| Message ID | 20240521204507.1288528-1-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix various overly aggressive protections in 2.45.1 and friends | expand |
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.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Much of what is tricky about these merges > happens outside conflict markers. Yes. In other words, what you do not see in these outputs can be leaving semantic conflicts unresolved. If we made a change to a location where there wasn't a textual conflict (i.e., making an evil merge) in order to adjust for a semantic differences (e.g., they added a callsite to a function whose signature we updated), any of "git show --cc", "git range-diff", and "git show --remerge-diff" would show. If you failed to do so and introduced a bug (and worse yet, unlike "oh, we now require one more argument to the function" that compilers would catch for us, there are differences that compilers would not notice), none of the three will show. > 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*]: > ... > 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). In short, you are agreeing that between "remerge-diff" and "range-diff" there is no difference in power to let you notice what is *not* there, and I am agreeing to your "what is *not* there is often more problematic", so we are on the same page. I do not think I am married to "remerge-diff" output, but it being the same form of the familiar "single-parent" patch, I personally find it far easier to read than "diff of diff". We may differ at that point, but it does not matter, as neither of our preferred format is adequate for the job. ;-)
diff --git a/builtin/clone.c b/builtin/clone.c remerge CONFLICT (content): Merge conflict in builtin/clone.c index 17d34efebd..399b2d3f42 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1447,15 +1447,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) free(unborn_head); free(dir); free(path); -<<<<<<< b9b439e0e3 (Git 2.40.2) free(repo_to_free); - free(template_dir_dup); -||||||| 47b6d90e91 - free(template_dir_dup); UNLEAK(repo); -======= - UNLEAK(repo); ->>>>>>> 9074ec92e7 (Revert "fetch/clone: detect dubious ownership of local repositories") junk_mode = JUNK_LEAVE_ALL; transport_ls_refs_options_release(&transport_ls_refs_options); diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh remerge CONFLICT (content): Merge conflict in t/t1800-hook.sh index edf0fa1334..3506f627b6 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,7 +177,6 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' -<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< b9b439e0e3 (Git 2.40.2) test_expect_success 'stdin to hooks' ' write_script .git/hooks/test-hook <<-\EOF && echo BEGIN stdin @@ -196,37 +195,4 @@ test_expect_success 'stdin to hooks' ' test_cmp expect actual ' -test_expect_success 'clone protections' ' - test_config core.hooksPath "$(pwd)/my-hooks" && - mkdir -p my-hooks && - write_script my-hooks/test-hook <<-\EOF && - echo Hook ran $1 - EOF - - git hook run test-hook 2>err && - grep "Hook ran" err && - test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ - git hook run test-hook 2>err && - grep "active .core.hooksPath" err && - ! grep "Hook ran" err -' - -|||||||||||||||||||||||||||||||| 47b6d90e91 -test_expect_success 'clone protections' ' - test_config core.hooksPath "$(pwd)/my-hooks" && - mkdir -p my-hooks && - write_script my-hooks/test-hook <<-\EOF && - echo Hook ran $1 - EOF - - git hook run test-hook 2>err && - grep "Hook ran" err && - test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ - git hook run test-hook 2>err && - grep "active .core.hooksPath" err && - ! grep "Hook ran" err -' - -================================ ->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 9074ec92e7 (Revert "fetch/clone: detect dubious ownership of local repositories") test_done