| Message ID | 777de92d9f166793cddbb383f497518a5dedb9f4.1640927044.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) | expand |
On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: >From: Elijah Newren <newren@gmail.com> > >When running `git merge-tree --real`, we previously would only return an >exit status reflecting the cleanness of a merge, and print out the >toplevel tree of the resulting merge. Merges also have informational >messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT >(file/directory)", etc.) In fact, when non-content conflicts occur >(such as file/directory, modify/delete, add/add with differing modes, >rename/rename (1to2), etc.), these informational messages are often the >only notification since these conflicts are not representable in the >contents of the file. > >Add a --messages option which names a file so that callers can request >these messages be recorded somewhere. > >Signed-off-by: Elijah Newren <newren@gmail.com> >--- > Documentation/git-merge-tree.txt | 6 ++++-- > builtin/merge-tree.c | 18 ++++++++++++++++-- > t/t4301-merge-tree-real.sh | 18 ++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > >diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt >index 5823938937f..4d5857b390b 100644 >--- a/Documentation/git-merge-tree.txt >+++ b/Documentation/git-merge-tree.txt >@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree > SYNOPSIS > -------- > [verse] >-'git merge-tree' --real <branch1> <branch2> >+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> > 'git merge-tree' <base-tree> <branch1> <branch2> > > DESCRIPTION >@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive > merge with rename detection. If the merge is clean, the exit status > will be `0`, and if the merge has conflicts, the exit status will be > `1`. The output will consist solely of the resulting toplevel tree >-(which may have files including conflict markers). >+(which may have files including conflict markers). With `--messages`, >+it will write any informational messages (such as "Auto-merging >+<path>" and conflict notices) to the given file. > > The second form is meant for backward compatibility and will only do a > trival merge. It reads three tree-ish, and outputs trivial merge >diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c >index c5757bed5bb..47deef0b199 100644 >--- a/builtin/merge-tree.c >+++ b/builtin/merge-tree.c >@@ -389,6 +389,7 @@ static int trivial_merge(const char *base, > > struct merge_tree_options { > int real; >+ char *messages_file; > }; > > static int real_merge(struct merge_tree_options *o, >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, > */ > > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); >+ >+ if (o->messages_file) { >+ FILE *fp = xfopen(o->messages_file, "w"); >+ merge_display_update_messages(&opt, &result, fp); >+ fclose(fp); I don't know enough about how merge-ort works internally, but it looks to me like at this point the merge already happened and we just didn't clean up (finalize) yet. It feels wrong to die() at this point just because we can't open messages_file. >+ } > printf("%s\n", oid_to_hex(&result.tree->object.oid)); >- merge_switch_to_result(&opt, NULL, &result, 0, 0); >+ >+ merge_finalize(&opt, &result); > return result.clean ? 0 : 1; > } > >@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > { > struct merge_tree_options o = { 0 }; > int expected_remaining_argc; >+ int original_argc; > > const char * const merge_tree_usage[] = { >- N_("git merge-tree --real <branch1> <branch2>"), >+ N_("git merge-tree --real [<options>] <branch1> <branch2>"), > N_("git merge-tree <base-tree> <branch1> <branch2>"), > NULL > }; > struct option mt_options[] = { > OPT_BOOL(0, "real", &o.real, > N_("do a real merge instead of a trivial merge")), >+ OPT_STRING(0, "messages", &o.messages_file, N_("file"), >+ N_("filename to write informational/conflict messages to")), > OPT_END() > }; > >@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > usage_with_options(merge_tree_usage, mt_options); > > /* Parse arguments */ >+ original_argc = argc; > argc = parse_options(argc, argv, prefix, mt_options, > merge_tree_usage, 0); >+ if (!o.real && original_argc < argc) >+ die(_("--real must be specified if any other options are")); > expected_remaining_argc = (o.real ? 2 : 3); > if (argc != expected_remaining_argc) > usage_with_options(merge_tree_usage, mt_options); >diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh >index 9fb617ccc7f..42218cdc019 100755 >--- a/t/t4301-merge-tree-real.sh >+++ b/t/t4301-merge-tree-real.sh >@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' ' > grep "^usage: git merge-tree" expect > ' > >+test_expect_success '--messages gives us the conflict notices and such' ' >+ test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 && >+ >+ # Expected results: >+ # "greeting" should merge with conflicts >+ # "numbers" should merge cleanly >+ # "whatever" has *both* a modify/delete and a file/directory conflict >+ cat <<-EOF >expect && >+ Auto-merging greeting >+ CONFLICT (content): Merge conflict in greeting >+ Auto-merging numbers >+ CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead. >+ CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree. >+ EOF >+ >+ test_cmp expect MSG_FILE >+' >+ > test_done >-- >gitgitgadget >
On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: >From: Elijah Newren <newren@gmail.com> > >When running `git merge-tree --real`, we previously would only return an >exit status reflecting the cleanness of a merge, and print out the >toplevel tree of the resulting merge. Merges also have informational >messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT >(file/directory)", etc.) In fact, when non-content conflicts occur >(such as file/directory, modify/delete, add/add with differing modes, >rename/rename (1to2), etc.), these informational messages are often the >only notification since these conflicts are not representable in the >contents of the file. > >Add a --messages option which names a file so that callers can request >these messages be recorded somewhere. > >Signed-off-by: Elijah Newren <newren@gmail.com> >--- > Documentation/git-merge-tree.txt | 6 ++++-- > builtin/merge-tree.c | 18 ++++++++++++++++-- > t/t4301-merge-tree-real.sh | 18 ++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > >diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt >index 5823938937f..4d5857b390b 100644 >--- a/Documentation/git-merge-tree.txt >+++ b/Documentation/git-merge-tree.txt >@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree > SYNOPSIS > -------- > [verse] >-'git merge-tree' --real <branch1> <branch2> >+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> > 'git merge-tree' <base-tree> <branch1> <branch2> > > DESCRIPTION >@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive > merge with rename detection. If the merge is clean, the exit status > will be `0`, and if the merge has conflicts, the exit status will be > `1`. The output will consist solely of the resulting toplevel tree >-(which may have files including conflict markers). >+(which may have files including conflict markers). With `--messages`, >+it will write any informational messages (such as "Auto-merging >+<path>" and conflict notices) to the given file. > > The second form is meant for backward compatibility and will only do a > trival merge. It reads three tree-ish, and outputs trivial merge >diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c >index c5757bed5bb..47deef0b199 100644 >--- a/builtin/merge-tree.c >+++ b/builtin/merge-tree.c >@@ -389,6 +389,7 @@ static int trivial_merge(const char *base, > > struct merge_tree_options { > int real; >+ char *messages_file; > }; > > static int real_merge(struct merge_tree_options *o, >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, > */ > > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); >+ >+ if (o->messages_file) { >+ FILE *fp = xfopen(o->messages_file, "w"); >+ merge_display_update_messages(&opt, &result, fp); >+ fclose(fp); >+ } Something else I just wondered. Can the user differentiate between the die() in xfopen() and a failed/unclean merge? Both just exit(1) don't they? > printf("%s\n", oid_to_hex(&result.tree->object.oid)); >- merge_switch_to_result(&opt, NULL, &result, 0, 0); >+ >+ merge_finalize(&opt, &result); > return result.clean ? 0 : 1; > } > >@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > { > struct merge_tree_options o = { 0 }; > int expected_remaining_argc; >+ int original_argc; > > const char * const merge_tree_usage[] = { >- N_("git merge-tree --real <branch1> <branch2>"), >+ N_("git merge-tree --real [<options>] <branch1> <branch2>"), > N_("git merge-tree <base-tree> <branch1> <branch2>"), > NULL > }; > struct option mt_options[] = { > OPT_BOOL(0, "real", &o.real, > N_("do a real merge instead of a trivial merge")), >+ OPT_STRING(0, "messages", &o.messages_file, N_("file"), >+ N_("filename to write informational/conflict messages to")), > OPT_END() > }; > >@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > usage_with_options(merge_tree_usage, mt_options); > > /* Parse arguments */ >+ original_argc = argc; > argc = parse_options(argc, argv, prefix, mt_options, > merge_tree_usage, 0); >+ if (!o.real && original_argc < argc) >+ die(_("--real must be specified if any other options are")); > expected_remaining_argc = (o.real ? 2 : 3); > if (argc != expected_remaining_argc) > usage_with_options(merge_tree_usage, mt_options); >diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh >index 9fb617ccc7f..42218cdc019 100755 >--- a/t/t4301-merge-tree-real.sh >+++ b/t/t4301-merge-tree-real.sh >@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' ' > grep "^usage: git merge-tree" expect > ' > >+test_expect_success '--messages gives us the conflict notices and such' ' >+ test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 && >+ >+ # Expected results: >+ # "greeting" should merge with conflicts >+ # "numbers" should merge cleanly >+ # "whatever" has *both* a modify/delete and a file/directory conflict >+ cat <<-EOF >expect && >+ Auto-merging greeting >+ CONFLICT (content): Merge conflict in greeting >+ Auto-merging numbers >+ CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead. >+ CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree. >+ EOF >+ >+ test_cmp expect MSG_FILE >+' >+ > test_done >-- >gitgitgadget >
On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote: > > On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: > >From: Elijah Newren <newren@gmail.com> > > > >When running `git merge-tree --real`, we previously would only return an > >exit status reflecting the cleanness of a merge, and print out the > >toplevel tree of the resulting merge. Merges also have informational > >messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT > >(file/directory)", etc.) In fact, when non-content conflicts occur > >(such as file/directory, modify/delete, add/add with differing modes, > >rename/rename (1to2), etc.), these informational messages are often the > >only notification since these conflicts are not representable in the > >contents of the file. > > > >Add a --messages option which names a file so that callers can request > >these messages be recorded somewhere. > > > >Signed-off-by: Elijah Newren <newren@gmail.com> > >--- > > Documentation/git-merge-tree.txt | 6 ++++-- > > builtin/merge-tree.c | 18 ++++++++++++++++-- > > t/t4301-merge-tree-real.sh | 18 ++++++++++++++++++ > > 3 files changed, 38 insertions(+), 4 deletions(-) > > > >diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt > >index 5823938937f..4d5857b390b 100644 > >--- a/Documentation/git-merge-tree.txt > >+++ b/Documentation/git-merge-tree.txt > >@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree > > SYNOPSIS > > -------- > > [verse] > >-'git merge-tree' --real <branch1> <branch2> > >+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> > > 'git merge-tree' <base-tree> <branch1> <branch2> > > > > DESCRIPTION > >@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive > > merge with rename detection. If the merge is clean, the exit status > > will be `0`, and if the merge has conflicts, the exit status will be > > `1`. The output will consist solely of the resulting toplevel tree > >-(which may have files including conflict markers). > >+(which may have files including conflict markers). With `--messages`, > >+it will write any informational messages (such as "Auto-merging > >+<path>" and conflict notices) to the given file. > > > > The second form is meant for backward compatibility and will only do a > > trival merge. It reads three tree-ish, and outputs trivial merge > >diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > >index c5757bed5bb..47deef0b199 100644 > >--- a/builtin/merge-tree.c > >+++ b/builtin/merge-tree.c > >@@ -389,6 +389,7 @@ static int trivial_merge(const char *base, > > > > struct merge_tree_options { > > int real; > >+ char *messages_file; > > }; > > > > static int real_merge(struct merge_tree_options *o, > >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, > > */ > > > > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); > >+ > >+ if (o->messages_file) { > >+ FILE *fp = xfopen(o->messages_file, "w"); > >+ merge_display_update_messages(&opt, &result, fp); > >+ fclose(fp); > > I don't know enough about how merge-ort works internally, but it looks to me > like at this point the merge already happened and we just didn't clean up > (finalize) yet. It feels wrong to die() at this point just because we can't > open messages_file. Yes, the merge already happened; there now exists a new toplevel tree (that nothing references). I'm not sure I understand what's wrong with die'ing here, though. I can't tell if you want to defer the die-ing until later, or just avoid the die-ing and return some kind of success despite failing to complete what the user requested. > > >+ } > > printf("%s\n", oid_to_hex(&result.tree->object.oid)); > >- merge_switch_to_result(&opt, NULL, &result, 0, 0); > >+ > >+ merge_finalize(&opt, &result); > > return result.clean ? 0 : 1; > > } > >
On Mon, Jan 3, 2022 at 4:35 AM Fabian Stelzer <fs@gigacodes.de> wrote: > > On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: > >From: Elijah Newren <newren@gmail.com> > > > >When running `git merge-tree --real`, we previously would only return an > >exit status reflecting the cleanness of a merge, and print out the > >toplevel tree of the resulting merge. Merges also have informational > >messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT > >(file/directory)", etc.) In fact, when non-content conflicts occur > >(such as file/directory, modify/delete, add/add with differing modes, > >rename/rename (1to2), etc.), these informational messages are often the > >only notification since these conflicts are not representable in the > >contents of the file. > > > >Add a --messages option which names a file so that callers can request > >these messages be recorded somewhere. > > > >Signed-off-by: Elijah Newren <newren@gmail.com> > >--- > > Documentation/git-merge-tree.txt | 6 ++++-- > > builtin/merge-tree.c | 18 ++++++++++++++++-- > > t/t4301-merge-tree-real.sh | 18 ++++++++++++++++++ > > 3 files changed, 38 insertions(+), 4 deletions(-) > > > >diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt > >index 5823938937f..4d5857b390b 100644 > >--- a/Documentation/git-merge-tree.txt > >+++ b/Documentation/git-merge-tree.txt > >@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree > > SYNOPSIS > > -------- > > [verse] > >-'git merge-tree' --real <branch1> <branch2> > >+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> > > 'git merge-tree' <base-tree> <branch1> <branch2> > > > > DESCRIPTION > >@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive > > merge with rename detection. If the merge is clean, the exit status > > will be `0`, and if the merge has conflicts, the exit status will be > > `1`. The output will consist solely of the resulting toplevel tree > >-(which may have files including conflict markers). > >+(which may have files including conflict markers). With `--messages`, > >+it will write any informational messages (such as "Auto-merging > >+<path>" and conflict notices) to the given file. > > > > The second form is meant for backward compatibility and will only do a > > trival merge. It reads three tree-ish, and outputs trivial merge > >diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > >index c5757bed5bb..47deef0b199 100644 > >--- a/builtin/merge-tree.c > >+++ b/builtin/merge-tree.c > >@@ -389,6 +389,7 @@ static int trivial_merge(const char *base, > > > > struct merge_tree_options { > > int real; > >+ char *messages_file; > > }; > > > > static int real_merge(struct merge_tree_options *o, > >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, > > */ > > > > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); > >+ > >+ if (o->messages_file) { > >+ FILE *fp = xfopen(o->messages_file, "w"); > >+ merge_display_update_messages(&opt, &result, fp); > >+ fclose(fp); > >+ } > > Something else I just wondered. Can the user differentiate between the die() > in xfopen() and a failed/unclean merge? > Both just exit(1) don't they? xfopen() calls die_errno(), which calls die_routine(), which will be pointing at die_builtin() since we don't change it in builtin/merge-tree.c, and die_builtin() calls exit(128). So, a different error code. But good question...perhaps I should mention exit codes other than 0 and 1 in the documentation of merge-tree for other failures. > > > printf("%s\n", oid_to_hex(&result.tree->object.oid)); > >- merge_switch_to_result(&opt, NULL, &result, 0, 0); > >+ > >+ merge_finalize(&opt, &result); > > return result.clean ? 0 : 1; > > } > > > >@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > > { > > struct merge_tree_options o = { 0 }; > > int expected_remaining_argc; > >+ int original_argc; > > > > const char * const merge_tree_usage[] = { > >- N_("git merge-tree --real <branch1> <branch2>"), > >+ N_("git merge-tree --real [<options>] <branch1> <branch2>"), > > N_("git merge-tree <base-tree> <branch1> <branch2>"), > > NULL > > }; > > struct option mt_options[] = { > > OPT_BOOL(0, "real", &o.real, > > N_("do a real merge instead of a trivial merge")), > >+ OPT_STRING(0, "messages", &o.messages_file, N_("file"), > >+ N_("filename to write informational/conflict messages to")), > > OPT_END() > > }; > > > >@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > > usage_with_options(merge_tree_usage, mt_options); > > > > /* Parse arguments */ > >+ original_argc = argc; > > argc = parse_options(argc, argv, prefix, mt_options, > > merge_tree_usage, 0); > >+ if (!o.real && original_argc < argc) > >+ die(_("--real must be specified if any other options are")); > > expected_remaining_argc = (o.real ? 2 : 3); > > if (argc != expected_remaining_argc) > > usage_with_options(merge_tree_usage, mt_options); > >diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh > >index 9fb617ccc7f..42218cdc019 100755 > >--- a/t/t4301-merge-tree-real.sh > >+++ b/t/t4301-merge-tree-real.sh > >@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' ' > > grep "^usage: git merge-tree" expect > > ' > > > >+test_expect_success '--messages gives us the conflict notices and such' ' > >+ test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 && > >+ > >+ # Expected results: > >+ # "greeting" should merge with conflicts > >+ # "numbers" should merge cleanly > >+ # "whatever" has *both* a modify/delete and a file/directory conflict > >+ cat <<-EOF >expect && > >+ Auto-merging greeting > >+ CONFLICT (content): Merge conflict in greeting > >+ Auto-merging numbers > >+ CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead. > >+ CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree. > >+ EOF > >+ > >+ test_cmp expect MSG_FILE > >+' > >+ > > test_done > >-- > >gitgitgadget > >
On 03.01.2022 08:51, Elijah Newren wrote: >On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote: >> >> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: >> >From: Elijah Newren <newren@gmail.com> [...] >> > >> > static int real_merge(struct merge_tree_options *o, >> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, >> > */ >> > >> > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); >> >+ >> >+ if (o->messages_file) { >> >+ FILE *fp = xfopen(o->messages_file, "w"); >> >+ merge_display_update_messages(&opt, &result, fp); >> >+ fclose(fp); >> >> I don't know enough about how merge-ort works internally, but it looks to me >> like at this point the merge already happened and we just didn't clean up >> (finalize) yet. It feels wrong to die() at this point just because we can't >> open messages_file. > >Yes, the merge already happened; there now exists a new toplevel tree >(that nothing references). I'm not sure I understand what's wrong >with die'ing here, though. I can't tell if you want to defer the >die-ing until later, or just avoid the die-ing and return some kind of >success despite failing to complete what the user requested. > I think i would prefer the merge operation to abort before actually merging when not being able to write its logfile. Otherwise we possibly do a whole lot of work that`s inaccessible afterwards isn't it? (since we don`t print the hash) Thanks for your work on this feature. I think this could open a lot of new possibilities.
On Mon, Jan 3, 2022 at 9:23 AM Fabian Stelzer <fs@gigacodes.de> wrote: > > On 03.01.2022 08:51, Elijah Newren wrote: > >On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote: > >> > >> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: > >> >From: Elijah Newren <newren@gmail.com> > [...] > >> > > >> > static int real_merge(struct merge_tree_options *o, > >> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, > >> > */ > >> > > >> > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); > >> >+ > >> >+ if (o->messages_file) { > >> >+ FILE *fp = xfopen(o->messages_file, "w"); > >> >+ merge_display_update_messages(&opt, &result, fp); > >> >+ fclose(fp); > >> > >> I don't know enough about how merge-ort works internally, but it looks to me > >> like at this point the merge already happened and we just didn't clean up > >> (finalize) yet. It feels wrong to die() at this point just because we can't > >> open messages_file. > > > >Yes, the merge already happened; there now exists a new toplevel tree > >(that nothing references). I'm not sure I understand what's wrong > >with die'ing here, though. I can't tell if you want to defer the > >die-ing until later, or just avoid the die-ing and return some kind of > >success despite failing to complete what the user requested. > > > > I think i would prefer the merge operation to abort before actually merging > when not being able to write its logfile. Otherwise we possibly do a whole > lot of work that`s inaccessible afterwards isn't it? (since we don`t print > the hash) I see where you're coming from, but I don't see this as worth worrying about. For two reasons: (1) I'm not sure I buy the "whole lot of work" concern. merge-ort is pretty snappy. For a simple example of rebasing a single patch in linux.git across a branch with 28000 renames, I get 176 milliseconds for merge_incore_nonrecursive(). Granted, linux.git is pretty small in terms of number of files, but Stolee did some measurements a while back on the Microsoft repos with millions of files at HEAD. For those, for a trivial merge he saw merge_incore_recursive() complete in 2 milliseconds, and for a trivial rebase he saw merge_incore_nonrecursive() complete in 4 milliseconds (See https://lore.kernel.org/git/CABPp-BHO7bZ3H7A=E9TudhvBoNfwPvRiDMm8S9kq3mYeSXrpXw@mail.gmail.com/). So huge numbers of files pose much less of a problem than lots of interesting work like renames, and merge-ort is pretty fast in either case. Sure, if we were talking about traditional merge-recursive which would have taken 150000 milliseconds on the same single patch in linux.git testcase (due to the 28000 renames), then we might worry more about not letting work get tossed, but at only 176 milliseconds even with a crazy number of renames, it's just not worth worrying about. (2) Even if there is a lot of computation, I don't see why this error path merits extra coding work to salvage the computation somehow By way of comparison, a regular `git merge` will abort after completing the same amount of merge work (i.e. after creating a new tree) when the user has a dirty working tree involving a path that would need to be updated by the merge operation. And that is not a bug; it's a requirement -- we cannot first check if the user has dirtied such a path before performing the merge because it's impossible to do so accurately in the face of renames. merge-recursive tried to do that and had early aborts that fell in the false-positive category and some that fell in the false-negative category. It was impossible to fix the false-positives and false-negatives without either (a) disallowing ever doing a merge with a dirty working tree under any conditions (a backwards compatibility break), or (b) waiting to do the notification of dirty-files-in-the-way until after the merge tree has been computed. I wasn't about to break that feature, so merge-ort had to delay error notifications instead. Now, the dirty-file-in-the-way condition is for a very common case (either for users who intentionally like keeping dirty changes around and doing merges but the branch they are merging happens to touch a file they didn't know about, or users who just forgot that they had local modifications). In contrast, this case here is for when we cannot open a file for writing -- with the filename explicitly just specified by the user. So, I'd rather keep the code nice and simple as it currently stands. > Thanks for your work on this feature. I think this could open a lot of new > possibilities. I hope people do interesting things with it, and with the server-side commit replaying I'm working on as well.
On 03.01.2022 11:46, Elijah Newren wrote: >On Mon, Jan 3, 2022 at 9:23 AM Fabian Stelzer <fs@gigacodes.de> wrote: >> >> On 03.01.2022 08:51, Elijah Newren wrote: >> >On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote: >> >> >> >> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote: >> >> >From: Elijah Newren <newren@gmail.com> >> [...] >> >> > >> >> > static int real_merge(struct merge_tree_options *o, >> >> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, >> >> > */ >> >> > >> >> > merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); >> >> >+ >> >> >+ if (o->messages_file) { >> >> >+ FILE *fp = xfopen(o->messages_file, "w"); >> >> >+ merge_display_update_messages(&opt, &result, fp); >> >> >+ fclose(fp); >> >> >> >> I don't know enough about how merge-ort works internally, but it looks to me >> >> like at this point the merge already happened and we just didn't clean up >> >> (finalize) yet. It feels wrong to die() at this point just because we can't >> >> open messages_file. >> > >> >Yes, the merge already happened; there now exists a new toplevel tree >> >(that nothing references). I'm not sure I understand what's wrong >> >with die'ing here, though. I can't tell if you want to defer the >> >die-ing until later, or just avoid the die-ing and return some kind of >> >success despite failing to complete what the user requested. >> > >> >> I think i would prefer the merge operation to abort before actually merging >> when not being able to write its logfile. Otherwise we possibly do a whole >> lot of work that`s inaccessible afterwards isn't it? (since we don`t print >> the hash) > >I see where you're coming from, but I don't see this as worth worrying >about. For two reasons: > >(1) I'm not sure I buy the "whole lot of work" concern. > >merge-ort is pretty snappy. For a simple example of rebasing a single >patch in linux.git across a branch with 28000 renames, I get 176 >milliseconds for merge_incore_nonrecursive(). Granted, linux.git is >pretty small in terms of number of files, but Stolee did some >measurements a while back on the Microsoft repos with millions of >files at HEAD. For those, for a trivial merge he saw >merge_incore_recursive() complete in 2 milliseconds, and for a trivial >rebase he saw merge_incore_nonrecursive() complete in 4 milliseconds >(See https://lore.kernel.org/git/CABPp-BHO7bZ3H7A=E9TudhvBoNfwPvRiDMm8S9kq3mYeSXrpXw@mail.gmail.com/). >So huge numbers of files pose much less of a problem than lots of >interesting work like renames, and merge-ort is pretty fast in either >case. Sure, if we were talking about traditional merge-recursive >which would have taken 150000 milliseconds on the same single patch in >linux.git testcase (due to the 28000 renames), then we might worry >more about not letting work get tossed, but at only 176 milliseconds >even with a crazy number of renames, it's just not worth worrying >about. I guess I'm just not used to how good ort is yet. I was still expecting cases like the merge-recursive ones :) > >(2) Even if there is a lot of computation, I don't see why this error >path merits extra coding work to salvage the computation somehow > >By way of comparison, a regular `git merge` will abort after >completing the same amount of merge work (i.e. after creating a new >tree) when the user has a dirty working tree involving a path that >would need to be updated by the merge operation. And that is not a >bug; it's a requirement -- we cannot first check if the user has >dirtied such a path before performing the merge because it's >impossible to do so accurately in the face of renames. >merge-recursive tried to do that and had early aborts that fell in the >false-positive category and some that fell in the false-negative >category. It was impossible to fix the false-positives and >false-negatives without either (a) disallowing ever doing a merge with >a dirty working tree under any conditions (a backwards compatibility >break), or (b) waiting to do the notification of >dirty-files-in-the-way until after the merge tree has been computed. >I wasn't about to break that feature, so merge-ort had to delay error >notifications instead. Completely understandable for the regular merge. In this case we are not touching the index/working tree at all though so I don't quite see how this is comparable. > >Now, the dirty-file-in-the-way condition is for a very common case >(either for users who intentionally like keeping dirty changes around >and doing merges but the branch they are merging happens to touch a >file they didn't know about, or users who just forgot that they had >local modifications). In contrast, this case here is for when we >cannot open a file for writing -- with the filename explicitly just >specified by the user. > > >So, I'd rather keep the code nice and simple as it currently stands. Especially since the extra work penalty is so miniscule i agree. Thanks > >> Thanks for your work on this feature. I think this could open a lot of new >> possibilities. > >I hope people do interesting things with it, and with the server-side >commit replaying I'm working on as well.
diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt index 5823938937f..4d5857b390b 100644 --- a/Documentation/git-merge-tree.txt +++ b/Documentation/git-merge-tree.txt @@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree SYNOPSIS -------- [verse] -'git merge-tree' --real <branch1> <branch2> +'git merge-tree' --real [--messages=<file>] <branch1> <branch2> 'git merge-tree' <base-tree> <branch1> <branch2> DESCRIPTION @@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive merge with rename detection. If the merge is clean, the exit status will be `0`, and if the merge has conflicts, the exit status will be `1`. The output will consist solely of the resulting toplevel tree -(which may have files including conflict markers). +(which may have files including conflict markers). With `--messages`, +it will write any informational messages (such as "Auto-merging +<path>" and conflict notices) to the given file. The second form is meant for backward compatibility and will only do a trival merge. It reads three tree-ish, and outputs trivial merge diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index c5757bed5bb..47deef0b199 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -389,6 +389,7 @@ static int trivial_merge(const char *base, struct merge_tree_options { int real; + char *messages_file; }; static int real_merge(struct merge_tree_options *o, @@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o, */ merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); + + if (o->messages_file) { + FILE *fp = xfopen(o->messages_file, "w"); + merge_display_update_messages(&opt, &result, fp); + fclose(fp); + } printf("%s\n", oid_to_hex(&result.tree->object.oid)); - merge_switch_to_result(&opt, NULL, &result, 0, 0); + + merge_finalize(&opt, &result); return result.clean ? 0 : 1; } @@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) { struct merge_tree_options o = { 0 }; int expected_remaining_argc; + int original_argc; const char * const merge_tree_usage[] = { - N_("git merge-tree --real <branch1> <branch2>"), + N_("git merge-tree --real [<options>] <branch1> <branch2>"), N_("git merge-tree <base-tree> <branch1> <branch2>"), NULL }; struct option mt_options[] = { OPT_BOOL(0, "real", &o.real, N_("do a real merge instead of a trivial merge")), + OPT_STRING(0, "messages", &o.messages_file, N_("file"), + N_("filename to write informational/conflict messages to")), OPT_END() }; @@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) usage_with_options(merge_tree_usage, mt_options); /* Parse arguments */ + original_argc = argc; argc = parse_options(argc, argv, prefix, mt_options, merge_tree_usage, 0); + if (!o.real && original_argc < argc) + die(_("--real must be specified if any other options are")); expected_remaining_argc = (o.real ? 2 : 3); if (argc != expected_remaining_argc) usage_with_options(merge_tree_usage, mt_options); diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh index 9fb617ccc7f..42218cdc019 100755 --- a/t/t4301-merge-tree-real.sh +++ b/t/t4301-merge-tree-real.sh @@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' ' grep "^usage: git merge-tree" expect ' +test_expect_success '--messages gives us the conflict notices and such' ' + test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 && + + # Expected results: + # "greeting" should merge with conflicts + # "numbers" should merge cleanly + # "whatever" has *both* a modify/delete and a file/directory conflict + cat <<-EOF >expect && + Auto-merging greeting + CONFLICT (content): Merge conflict in greeting + Auto-merging numbers + CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead. + CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree. + EOF + + test_cmp expect MSG_FILE +' + test_done