Inline slop comments

At work, I occassionally have to review code that's generated by the text extruder du jour.

I really don't like it. The code might be technically correct, but it still tends to feel subtly wrong, in a way that's a bit hard to put my finger on, and the pull request descriptions tend to be overly verbose, to the point where my eyes glaze over and I loose focus after reading a few sentences. It's that obvious LLM writing style I think most of us have learned to recognize by now.

...and then there's the comments. The inline comments that some of those text extruders tend to extrude before each block of changed code. They kind of look like the example code from a textbook to introductory programmming.

There's something else that's been bugging me with those, however, and I have had a hard time pinpointing what it is. While the comments are generally not wrong, they tend to state basic facts about whatever language/framework/context the code lives in. It could be that it states something about the function used in the following block (even though that function in the standard library is well-documented), or maybe it mentions something extremely basic about how this particular flavor of CI-pipeline YAML works.

The thing that's been annoying me is that a lot of these inline comments only make sense in the context of the commit when they were added, and therefore they should have been a part of the commit message instead.

An example

I encountered this today. This a part of a workflow that acts on pull requests in some Erlang repositories, and if it detects changes in the rebar.config file, it checks whether the rebar.lock file also needs updating.

Exhibit A

The first offender is these two added lines:

       - uses: actions/checkout@v7
         with:
+          # The rebar.config check diffs from the merge base, which a shallow clone lacks.
+          fetch-depth: 0

The change itself, adding fetch-depth: 0 does make sense here, since we actually want to check against the merge-base rather than against the default branch, since this could be in a pull request targeting something other than the default branch.

The problem with this comment, is what I stated above: It kind of makes sense when looking at this pull request, but if this were to get merged as is, that comment will not mean anything for someone looking at the file later. It's pointless to have this comment here.

Exhibit B

A bunch of lines further down in the same file, we find this diff:

+            # Check if this branch changed rebar.config since it left the default branch.
+            # Three dots diff from the merge base: with two, a branch that has fallen
+            # behind looks like it changed every rebar.config edit made on main since.
+            if git diff --stat __rebar_compare__... | grep -q '^ *rebar.config '; then
-            if git diff --stat __rebar_compare__.. | grep -q '^ *rebar.config '; then

In this case, as well as the previous one, the code diff is acceptable. It makes more sense to diff from the merge base rather between two arbitrary commits.

Here is the relevant section of man git diff:

    git diff [<options>] <commit>..<commit> [--] [<path>...]
         This is synonymous to the earlier form (without the ..) for viewing the changes between two arbitrary <commit>. If <commit> on one side is omitted, it will have the same effect as using HEAD instead.

     git diff [<options>] <commit>...<commit> [--] [<path>...]
         This form is to view the changes on the branch containing and up to the second <commit>, starting at a common ancestor of both <commit>.  git diff A...B is equivalent to git diff $(git merge-base A B) B. You can omit any  one
         of <commit>, which has the same effect as using HEAD instead.

The comment above this code diff explains the same thing, and why it can be problematic to use .. instead of ... here. I agree with this comment, but I don't think it should live in the workflow file.

This should not be a comment, this is the commit message

Let's go over that again:

• It starts with a line explaining what the change does, except that it's arguably a little wrong¹.
• The rest of this comment explains why the change is necessary.

So: the comment on the second block has two sentences: one describing what is done, and one describing why.

To me, this screams "COMMIT MESSAGE!". This second diff block should be one comment, and the comment should be the commit message, although it can be shortened a bit more as well:

Check if rebar.config changed compared to merge-base

Three dots in git diff checks with the merge-base,
while two dots check between two arbitrary commits. 

The comment on the first block could also be the body of a commit message:

Fetch entire history when cloning

The rebar.config check diffs from the merge base, which a shallow clone lacks.

So, what do we learn from this?

Don't use LLMs to extrude code. But if you do, read through the output a few times extra, and think about whether all of the output is really necessary.

And for the love of everything that's holy, write the pull request description yourself, so your human peers don't get bored while attempting to read it!