Reviewing merge commits, was Re: [rPATCH 13/12] Merge branch 'jc/fix-aggressive-protection-2.39'

Hi Junio,

On Tue, 21 May 2024, Junio C Hamano wrote:

Personally, I find this format quite hard to parse, and I am concerned that the focus on the merge conflicts may distract too much from verifying the correctness of the result. Much of what is tricky about these merges happens outside conflict markers.

If it was up to me to verify such fixes, short of using Git and validating the correctness by performing the merge independently instead of trying to accomplish the validation by looking at a plain-text mail, I would compare the diff of `maint-2.39` to the diff of `maint-2.40`. Something like this [*1*]:

  $ git range-diff \
    mbox:<(git diff v2.45.1...gitster/jc/fix-2.45.1-and-friends-for-2.39) \
    mbox:<(git diff v2.45.1...gitster/fixes/2.45.1/2.40)
  1:  00000000000 ! 1:  00000000000
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

          if (real_git_dir) {
       @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      -   free(unborn_head);
          free(dir);
          free(path);
      +   free(repo_to_free);
       -  free(template_dir_dup);
      -   UNLEAK(repo);
      ++  UNLEAK(repo);
          junk_mode = JUNK_LEAVE_ALL;

      +   transport_ls_refs_options_release(&transport_ls_refs_options);

        ## cache.h ##
       @@ cache.h: int copy_fd(int ifd, int ofd);
      @@ t/t1450-fsck.sh: test_expect_success 'fsck error on gitattributes with excessive
        test_done

        ## t/t1800-hook.sh ##
      -@@ t/t1800-hook.sh: test_expect_success 'git hook run a hook with a bad shebang' '
      +@@ t/t1800-hook.sh: test_expect_success 'stdin to hooks' '
          test_cmp expect actual
        '
It does get a bit more confusing with the diff between `maint-2.40` and
`maint-2.41` because the declaration of `do_files_match()` moved from
`cache.h` to `copy.h`, and the hunks removing that declaration are at
different locations in the diffs:

  $ git range-diff \
    mbox:<(git diff v2.45.1...gitster/fixes/2.45.1/2.40) \
    mbox:<(git diff v2.45.1...gitster/fixes/2.45.1/2.41)
  1:  00000000000 ! 1:  00000000000
      @@ Makefile: exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \

        ## builtin/clone.c ##
       @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      - 	int err = 0, complete_refs_before_fetch = 1;
        	int submodule_progress;
        	int filter_submodules = 0;
      + 	int hash_algo;
       -	const char *template_dir;
       -	char *template_dir_dup = NULL;

      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

        	transport_ls_refs_options_release(&transport_ls_refs_options);

      - ## cache.h ##
      -@@ cache.h: int copy_fd(int ifd, int ofd);
      - int copy_file(const char *dst, const char *src, int mode);
      - int copy_file_with_time(const char *dst, const char *src, int mode);
      -
      --/*
      -- * Compare the file mode and contents of two given files.
      -- *
      -- * If both files are actually symbolic links, the function returns 1 if the link
      -- * targets are identical or 0 if they are not.
      -- *
      -- * If any of the two files cannot be accessed or in case of read failures, this
      -- * function returns 0.
      -- *
      -- * If the file modes and contents are identical, the function returns 1,
      -- * otherwise it returns 0.
      -- */
      --int do_files_match(const char *path1, const char *path2);
      --
      - void write_or_die(int fd, const void *buf, size_t count);
      - void fsync_or_die(int fd, const char *);
      - int fsync_component(enum fsync_component component, int fd);
      -
        ## ci/install-dependencies.sh ##
       @@ ci/install-dependencies.sh: macos-*)
        	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
      @@ copy.c: int copy_file_with_time(const char *dst, const char *src, int mode)
       -	return ret;
       -}

      + ## copy.h ##
      +@@ copy.h: int copy_fd(int ifd, int ofd);
      + int copy_file(const char *dst, const char *src, int mode);
      + int copy_file_with_time(const char *dst, const char *src, int mode);
      +
      +-/*
      +- * Compare the file mode and contents of two given files.
      +- *
      +- * If both files are actually symbolic links, the function returns 1 if the link
      +- * targets are identical or 0 if they are not.
      +- *
      +- * If any of the two files cannot be accessed or in case of read failures, this
      +- * function returns 0.
      +- *
      +- * If the file modes and contents are identical, the function returns 1,
      +- * otherwise it returns 0.
      +- */
      +-int do_files_match(const char *path1, const char *path2);
      +-
      + #endif /* COPY_H */
      +
        ## fsck.c ##
       @@ fsck.c: static int fsck_tree(const struct object_id *tree_oid,
        				retval += report(options, tree_oid, OBJ_TREE,
      @@ git-send-email.perl: sub get_patch_subject {

        ## hook.c ##
       @@
      - #include "run-command.h"
      - #include "config.h"
      -
      + #include "strbuf.h"
      + #include "environment.h"
      + #include "setup.h"
      +-#include "copy.h"
      +-
       -static int identical_to_template_hook(const char *name, const char *path)
       -{
       -	const char *env = getenv("GIT_CLONE_TEMPLATE_DIR");
      @@ hook.c
       -	strbuf_release(&template_path);
       -	return ret;
       -}
      --
      +
        const char *find_hook(const char *name)
        {
      - 	static struct strbuf path = STRBUF_INIT;
       @@ hook.c: const char *find_hook(const char *name)
        		}
        		return NULL;
      @@ t/t1350-config-hooks-path.sh: test_expect_success 'git rev-parse --git-path hook
        test_done

        ## t/t1450-fsck.sh ##
      -@@ t/t1450-fsck.sh: test_expect_success 'fsck error on gitattributes with excessive size' '
      - 	test_cmp expected actual
      +@@ t/t1450-fsck.sh: test_expect_success 'fsck reports problems in main index without filename' '
      + 	test_cmp expect actual
        '

       -test_expect_success 'fsck warning on symlink target with excessive length' '

Another thing that makes the review of these diffs-of-diffs harder than necessary is the lack of color-coding in plain-text emails. When I look at the color-coded version, my eyes are immediately drawn away from the unimportant lines that start with a `-` or `+` but then continue in uncolored text that indicates context line changes only. Instead, my eyes shift immediately to the relevant parts, the blankets of red color.

Both the remerge-diff and the range-diff output do nothing, though, to help verifying that no-longer-needed `#include`s are removed (you can see that `#include "copy.h"` was removed from `hook.c`, but if that had been missed there would be no indicator thereof).

So while I find this output more useful than the remerge diffs, it is still far from ideal. I will therefore refrain from posting the range-diffs that correspond to the remaining `maint-*` merges.

Ciao, Johannes

Footnote *1*: This `mbox:` construct requires https://github.com/gitgitgadget/git/pull/1420 (or `js/range-diff-mbox`) to work as intended.