Hi Junio,
On Tue, 21 May 2024, Junio C Hamano wrote:
> If this format looks reasonable to folks as a way to review the result
> of merging up fixes, I'll follow up with "patches" for more recent
> maintenance tracks.
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.
