How can I keep track of which existing lines of code/text have passed some new review?

Question

Firstly, this is a really tough topic to do searches for because you get a wealth of information on “code review”, which is different from what I’m referring to here. That normally refers to reviewing new code that is being introduced into the codebase. That’s not what I’m interested in here.

The basic problem: You have a fairly large collection of textual files, some of them source code, and some plain text or Markdown files. You have it all under source control ^{(though perhaps only recently)}. Some characteristic or requirement of the project changes. Perhaps there’s a new security requirement, or perhaps you want to migrate to a different API. Perhaps you want to make sure documentation hasn’t gotten out of sync with the actual code, or perhaps some new idea has come along and you want to make sure all engineering/planning documentation takes it into account.

At that moment, conceptually, every line of code and every paragraph of text in all of the project’s files has in some sense become “invalid”, and someone will have to go through every class or every function or every line of code or every code comment or every section or every paragraph and “revalidate” them. Many elements will simply be found to be trivially “valid”, but some will have to be changed, and may trigger changes that are scattered through other elements and files.

There may be occasion to put this review aside, work on other things, and come back to it later.

The main challenge I’m concerned with here is: How do you keep track of which lines have and haven’t been validated according to the new requirement?

I'm looking for a solution that is agnostic to the type of work unit being reviewed. For instance, a solution that assumes that the review is being done on classes or functions wouldn't work, because external documentation will often be part of what needs review.

Possible Ideas

I’ve thought about how I might go about this with the tools I have presently. My projects of interest will tend to be versioned under Git, although asset-heavy projects will probably use Perforce instead. (I’ve been learning Git, still have to research Perforce.) Projects right now are mostly in either Visual Studio 2008 or 2019. Non-code text files tend to be edited outside these, typically using EmEditor, for instance.

Idea #1: Mark up in a branch

Basically, start a new branch dedicated to the review, and within that branch mark things up with code comments and the like to indicate what has been validated. This is nice and simple and can be done with my existing tools but does have some downsides, mostly connected with how copiously you insert these markings.

If you use them copiously:

• It’s more effort.
• It creates more divergence from the mainline which may make it harder to keep the branch synchronized. (You don't want to ever merge these marks into the mainline as they would simply clutter it up.)
• The marks could become distracting to the reviewer.

If you use them sparingly, you mostly avoid these problems but then it becomes error-prone because it’s harder to notice what has and hasn’t actually been validated.

Idea #2: Totally valid branch

Start with a completely empty branch (by git checkout --orphan and then unstage everything), and only admit pieces into this branch as they have been validated.

On one hand, this is an attractive approach because it’s less error-prone: you know everything in this branch was explicitly validated. It also avoids awkward mark-up, and you can use existing GUI tools such as git-gui to pick off individual lines to mark as valid, just by staging and committing them.

In Git I think it would be fairly easy to implement this by doing:

git switch some-review
git merge --no-commit main
git reset HEAD

At this point, your work tree will have the whole project in it, with anything that still needs review as unstaged changes. From here, you can just pick off some bit to review, and once it’s valid, stage and commit it.

This does have some drawbacks:

• You have to periodically do the above song and dance, because otherwise the branch is totally unbuildable and doesn’t even contain anything you still need to review. And if at any point you forget the --no-commit or the git reset HEAD, you’re going to make an annoying mess.
• You have to be careful not to accidentally stage things you haven’t actually validated, because there is really no other indication of whether you’ve reviewed it or not.
• It may push the limits of what Git can keep track of. As a pathological example, consider a single valid line at some random position in an otherwise unvalidated 500 line file. Git can stage this one line, but then on the next merge, it would have no idea where this line came from, so you’d be faced with an artificial merge conflict. It would likewise be impossible to straightforwardly cherry-pick this into the mainline. So, you’d have to validate in hunks large enough for Git to recognize the context, and this could also become more challenging if you’ve made changes to many of the lines involved. Worse, if the context you’re staging is not good enough, there is no warning of this until you go to do another merge and it fails.
• Due to the context problem, you’re probably better off to only make changes outside the review branch and then merge them back in, but this will become rather cumbersome.
• Merging mainline changes would be a bit cumbersome and error-prone. You can’t just do a normal merge because that would clobber your entire validation state. You have to do a --no-commit merge and then figure out what parts to stage based on what was already on the review branch or has otherwise been newly reviewed.
• Because large parts of the project tree are intentionally missing from the review branch, diffs will not differentiate between new mainline changes and stuff which has always been in mainline but which you just haven’t reviewed yet. This may make it harder to understand any parallel changes.
• git-gui performs very badly on text files that are not hard-wrapped, so when working on documentation, you’d have to jump back and forth between git-gui and some editor that can actually word wrap lines instead of having them scroll off the screen. This back-and-forth would be cumbersome and a bit error-prone.
• I‘m not sure if Perforce would permit a similar workflow and be able to track this even as well as Git could.

Question(s)

Is there any better way to do this, than my above two ideas? Either with my existing tools, or with some other type of tool?

Or, if these are my only options, what other factors might favour the one approach versus the other?

So as not to turn this into a shopping question, I’m not going to ask if there’s any specific piece of software that would be better suited than Git, Visual Studio, and EmEditor, but if there is some class of software other than revision control systems, IDEs, and text editors that would handle this better, then identifying that general class would be useful and then I can investigate the individual options on my own. (In the same sense as Q:”We keep clobbering each other’s edits,” A:”You need version control software.”)

Answer 1

There may be special programs for this, but I think this can be solved with most version control systems, without introducing a special tool. Extra tools may help solve the task quicker or smoother, but they could also introduce other, new problems into the development process (like the learning curve, an extra dependency from another 3rd party vendor, or some fuzz in the interaction with the current environment). So before going that route, it must be evaluated if their advantages over manual tracking really balance their costs.

#2 has IMHO way too many drawbacks, as you listed already in the question by yourself. It is overly complicated and error-prone, and that is exactly what you don't want.

If I were in your situation, I would probably try my luck with a variant of #1 which does not produce huge amount of clutter:

1. Use a kind of mark in the comment which is globally searchable, maybe by some unique keyword (or maybe a keyword combination - one for lines which have passed the review, one one where you want to come back later).

2. Try to work through the code file-by-file, and try to finish a file completely before reviewing the next one. When a source code file is completely "done", remove the inner marks from it, except one at the top which is indicating the review/rework is completed here, Finding those marks should be trivial because of 1. That should keep the number of "inner" marks small. Ideally, they are in only one file at a time, which should be manageable.

   This can also be done the other way round: start by adding a marker comment at the beginning of each file you want to review. Add comments as long as you work yourself through the file, and remove them completely when you are done with it. That way, a global search for all markers will give you a more straightforward progress indication - when there are no marker comments left, the review is complete.

I would also check rigorously if you may be able to refactor the code in the trunk first, before starting the actual the review, with the goal of making the review simpler.

• Maybe the part of the code base which need to be reviewed can be isolated / reduced to a layer or set of components, by making sure first that certain things are only happen in that layer or set of components?

• Maybe there are some source files to be reviewed which are pretty big and can be split-up into smaller ones beforehand, so the file-by-file approach for the review will work smoother?

Going though such a task will still require some discipline and bring some risk to introduce defects into working code, but that is true for other, extra-tool based approaches as well.

Answer 2

This isn’t what I’d call a “case closed” type answer, as it’s really not an ideal solution, but shares some experience I gained while attempting an actual review task (which is not totally done, but it seems should be ok to finish in this manner). It wasn’t an absolutely huge task, but I tried to be systematic, partly for the sake of figuring out how you might attack a larger task.

What Didn’t Work So Well

In principle, it would be clear, portable, and fairly simple to use various textual “inner” markers at least on the file or files you’re presently working on, and that approach is also conceptually nice in that the VCS can record and track how the review state changes. I came up with a scheme expanding @DocBrown’s idea, in which markers from inner scopes could “bubble up” through higher scopes, so as to eliminate markers from inner scopes as soon as possible. I also considered above/below markers, to further reduce the markers in cases where the review can proceed in textual order.

Unfortunately, in practice, I found that workflows involving markers inside the versioned texts anywhere near content you may want to change during the review to be very problematic, because often you’re constantly creating your own merge conflicts. In particular, problems I found were:

• Markers are very likely to cause pesky merge conflicts where you’re trying to make the actual changes in another branch and then merge back into the review branch. They only have to be near edited lines to cause conflicts.
• Worse, if you’ve cherry-picked stuff from the review branch into the mainline and then want to merge other stuff from the mainline into the review branch, markers on the previously cherry-picked stuff will almost always cause merge conflicts, because Git doesn’t consider cherry-picks in computing the merge base — it just thinks both sides changed differently. You could avoid this by doing or cherry-picking each commit on its own separate helper branch and then merging that into both the mainline and review branches to set a proper merge base, but this would be quite tedious where the review generates many commits.
• Cherry-picking changes off the review branch is a bit tedious and error-prone. In principle, git cherry or git log --cherry-pick ought to be able to help, but if you’ve merged the mainline into the review branch in the meantime, these commands won’t look for duplicates past the merge base, so you get a lot of false positives. You can do non-committed test merges into the mainline to figure out whether you’ve missed any cherries, but in practice I found this can get bogus conflicts.
• Markers clutter the work tree diffs where you’re trying to make the actual changes in the same branch. Separating marker changes from content changes for staging tends to be cumbersome.
• Where there are markers on the same lines as content, such as for list items in Markdown files, which don’t really like these on separate lines, it’s quite difficult in git-gui to tell whether a line has only changed the marker, or whether it has also changed content. The potential for error is high. This is mainly due to weaknesses of git-gui, and I need to look for better GUI clients for Git.
• Furthermore, Git sees such changes as basically opaque, even if the line’s actual content hasn’t changed, so the potential for severe merge conflicts becomes high.
• Placing and moving markers tends to involve significantly more scrolling, mouse clicks, and/or keystrokes than would be ideal.

Of course, some of these problems depend on the granularity and the structure of what you’re reviewing. If you can keep the markers far enough away from what you’re changing that they end up in separate diff hunks, then this procedure should be a fair bit less cumbersome. But this may not always be simple — the most logical and useful marker positions in my case did bump into changes quite often.

Also, some of these problems may in some cases be solved by using different VCS software altogether. In research for game repos, I recently ran into a system called Plastic SCM, which it looks like incidentally may be significantly smarter than Git for merges (my interest was more due to scalability). It’s able to merge heavy refactors with minimal intervention, by using language-aware merges, as well as looking at specific changes within each line. However, I haven’t tried it yet (mainly because the on‑premises server is limited to the most expensive tier).

What Worked Okay

By “okay” I mean this wasn’t anywhere near ideal, but you can kind of get by, at least if you can mostly contain yourself to one file at a time.

The basic idea is to, similar to @DocBrown’s suggestion, keep just a marker at the top of each reviewed or unreviewed file, work on one file at a time, and in some fashion keep track of progress within that one file. Where I had to diverge from that exact idea was in how to track the progress inside the current file. Instead of using textual markers as part of the file content, I used bookmarks within the Visual Studio IDE.

These are far from ideal, among other things because there’s only one type of bookmark. But with some ingenuity and brain memory, it’s possible to come up with a usable scheme. And bookmarks inherently don’t cause merge conflicts, which made them much more practical than marking up the text. It’s also quicker to set them than it is to fiddle with markers in the text.

There are of course some serious limitations:

• They’re usually limited to one user’s workspace, unless you want to check in the IDE’s private data to the review branch, and be careful not to work simultaneously. Probably best to have only one person per file.
• They’re rather ephemeral, so if you need to suspend the review for an extended period, you’ll either need to:
  • finish up the current file first,
  • bake the bookmarks into “inner” markers within the text, but get rid of these as soon as you pick back up, before any merging, or
  • save the progress indicators into some other form.

I found that in VS 2019, bookmarks worked better than expected, mainly because — and I never would’ve expected it, but discovered accidentally — if you modify a file outside VS while it’s open in VS, VS will automatically adjust the bookmark line numbers so that they stay on the same content.

So, if you want to merge in the upstream, you can do that without having your bookmarks jump all over.

I also found this feature quite handy for working on Markdown files. Modern versions of VS are dog slow, like as in, can’t even keep up with my typing. (But they do have lots of nifty features.) Between that and other editors being able to do things like WSYWIG editing of Markdown, I prefer to use other editors on plain text and Markdown files. Since the other editor and VS both monitor for outside changes, I can just switch back and forth easily between the two, provided I always save before switching. As I have them set up, they don’t even ask me about the reload if there are no unsaved changes.

YMMV depending on which IDE or editor’s bookmark feature you’re using.

Some important notes about using VS 2019’s bookmarks in this manner:

• This only works if the document is actually open in VS when the outside change is made to it; an outside change to a document that VS doesn’t have open at the time will not cause the bookmarks to update, which will probably lead to them being in the wrong places when you open it next.
• Even when VS does update the bookmark positions, it doesn't keep these changes after closing the file unless you then modify the file and then save it — just Save All without modified files doesn't do it. (This is probably a bug; the code that updates the bookmark positions on reload probably forgets to set a “dirty” flag on the in-memory .suo data.)
• If the line on which a bookmark is placed is deleted from outside of VS, when VS reloads the file per above, the bookmark will be deleted without warning.
• When opening files (via File->Open->File...) outside of any Solution, bookmarks can be used, but ordinarily they won't be saved. However, if you use the "Open a local folder" feature from the Start Window, or File->Open->Folder..., this will create a .vs subdirectory in the directory, and then bookmarks will be saved.

And...

This method was workable on the particular review task I used it on, but may have been problematic in more complicated cases. I’m certainly open to other ideas.

Answer 3

The first step would be to take inventory of all problems (list of different problems/code and where they are in the project). First in some part of the project, then over all.

If you are lucky a code checker like SonarLint might help.

If manual inspection needed, add a

// TODO technical debt, security, issue #1234

Already this allows checking incremental progress in a quantitative manner.

From this git version on all new changes for other aspects should refrain from touching the places (not even repair them themselves) and not introduce new security problems.

Until we solved our problems, we need to review the till then added code.

The review of added code can/could proceed as follows:

• For every other issue (ticket) a git feature/request/bug branch is created. After solved, a pull request is made for a peer review. Also checking our deficits.
• Then the code is merged back; it might be with new incursions // TODO ... to be registered in our list.

Solving the problem spots has its own dangers:

• Introducing help functions to replace the problematic code. Better take a bit larger context, and see if Don't-Repeat-Yourself can be implemented. This must simplify the code. And remove the number of problem spots. Sometimes however a help function is needed, ideally not in a new "utility" class.

The new code (actually as all new code) should be marked with the issue, why it was introduced as such, and to prevent what problem.

/**
 * ...
 * Prevents SQL injection as when the parameter is null,
 * a WHERE filtering is ommited, leaking information.
 * Jira Ticket: #1234.
 * It replaces <code>...</code> with ... .
 * ...
 */
public ...

These why-comments are worthwhile, and for code documentation it would be nice to have all these kind of issues listed in a wiki/word document so that documented technical and business logic have a correspondence in code and vice versa.

Addendum

In general I am not a friend of feature/bug branching and the mergure with peer review. It concentrates on making just minimal changes, and technical debt (bad code) keeps laying around. Good software has continuous code improvement on the side, as otherwise the system deteriorates. (I would not like to be blamed for unflexible coding standards.)

Answer 4

It seems to me that if this new security requirement is so important that it requires a review of every single line of code in the project, it would be important enough to do that work all at once, no? Or with a simple checklist of files?

And if it's not important enough to do that work all at once, I would recommend writing automated tests that exercise the new requirement, and gradually work in the rest of the code base.