* [PATCH 0/1] warn about auto fast-forwarded submodules during merges @ 2018-05-10 18:26 Leif Middelschulte 2018-05-10 18:26 ` [PATCH 1/1] Warn about fast-forwarding of submodules during merge Leif Middelschulte 0 siblings, 1 reply; 15+ messages in thread From: Leif Middelschulte @ 2018-05-10 18:26 UTC (permalink / raw) To: git; +Cc: gitster, sandals, Leif Middelschulte From: Leif Middelschulte <Leif.Middelschulte@gmail.com> Warn the user during merges about automatically fast-forwarded submodules. This is just informational and does *not* change behavior otherwise. It is a follow up to Elijah Newren's suggestion[0] to provide the attached patch. [0] https://marc.info/?l=git&m=152544498723355&w=2 Leif Middelschulte (1): Warn about fast-forwarding of submodules during merge submodule.c | 2 ++ 1 file changed, 2 insertions(+) -- 2.15.1 (Apple Git-101) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/1] Warn about fast-forwarding of submodules during merge 2018-05-10 18:26 [PATCH 0/1] warn about auto fast-forwarded submodules during merges Leif Middelschulte @ 2018-05-10 18:26 ` Leif Middelschulte 2018-05-10 18:49 ` Stefan Beller 0 siblings, 1 reply; 15+ messages in thread From: Leif Middelschulte @ 2018-05-10 18:26 UTC (permalink / raw) To: git; +Cc: gitster, sandals, Leif Middelschulte From: Leif Middelschulte <Leif.Middelschulte@gmail.com> Warn the user about an automatically fast-forwarded submodule. The silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward merge for submodules", 2010-07-07)). Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com> --- submodule.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/submodule.c b/submodule.c index 74d35b257..0198a72e6 100644 --- a/submodule.c +++ b/submodule.c @@ -1817,10 +1817,12 @@ int merge_submodule(struct object_id *result, const char *path, /* Case #1: a is contained in b or vice versa */ if (in_merge_bases(commit_a, commit_b)) { oidcpy(result, b); + warning("Fast-forwarding submodule %s", path); return 1; } if (in_merge_bases(commit_b, commit_a)) { oidcpy(result, a); + warning("Fast-forwarding submodule %s", path); return 1; } -- 2.15.1 (Apple Git-101) ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] Warn about fast-forwarding of submodules during merge 2018-05-10 18:26 ` [PATCH 1/1] Warn about fast-forwarding of submodules during merge Leif Middelschulte @ 2018-05-10 18:49 ` Stefan Beller 2018-05-10 20:30 ` Leif Middelschulte 0 siblings, 1 reply; 15+ messages in thread From: Stefan Beller @ 2018-05-10 18:49 UTC (permalink / raw) To: Leif Middelschulte; +Cc: git, Junio C Hamano, sandals On Thu, May 10, 2018 at 11:26 AM, Leif Middelschulte <leif.middelschulte@gmail.com> wrote: > From: Leif Middelschulte <Leif.Middelschulte@gmail.com> Hi Leif! thanks for following up with a patch! > Warn the user about an automatically fast-forwarded submodule. The silent merge > behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward > merge for submodules", 2010-07-07)). > > Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com> > --- > submodule.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/submodule.c b/submodule.c > index 74d35b257..0198a72e6 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1817,10 +1817,12 @@ int merge_submodule(struct object_id *result, const char *path, > /* Case #1: a is contained in b or vice versa */ > if (in_merge_bases(commit_a, commit_b)) { > oidcpy(result, b); > + warning("Fast-forwarding submodule %s", path); > return 1; > } > if (in_merge_bases(commit_b, commit_a)) { > oidcpy(result, a); > + warning("Fast-forwarding submodule %s", path); > return 1; > } The code looks correct, however I think we can improve it. (Originally I was just wondering if stderr is the right output, which lead me to the thoughts below:) Looking through the code of merge-recursive.c, all the other merge outputs are done via 'output()' that is able to buffer up the output as well as handles the output for different verbosity settings. So I would think we should make the output() function available outside of merge-recursive.c. (and rename it to a be more concise and descriptive in the global namespace) and make use of it. Funnily we already have MERGE_WARNING in submodule.c which outputs information for all the other cases. I would think we ought to convert those to the output(), too. Thanks, Stefan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] Warn about fast-forwarding of submodules during merge 2018-05-10 18:49 ` Stefan Beller @ 2018-05-10 20:30 ` Leif Middelschulte 2018-05-10 21:19 ` [PATCH 0/2] Submodule merging: i18n, verbosity Stefan Beller 0 siblings, 1 reply; 15+ messages in thread From: Leif Middelschulte @ 2018-05-10 20:30 UTC (permalink / raw) To: Stefan Beller; +Cc: sandals, Junio C Hamano, git Hi Stefan, Am 10. Mai 2018 um 20:49:39, Stefan Beller (sbeller@google.com(mailto:sbeller@google.com)) schrieb: > On Thu, May 10, 2018 at 11:26 AM, Leif Middelschulte > wrote: > > From: Leif Middelschulte > > Hi Leif! > > thanks for following up with a patch! sure, thanks for the quick review. > > > Warn the user about an automatically fast-forwarded submodule. The silent merge > > behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward > > merge for submodules", 2010-07-07)). > > > > Signed-off-by: Leif Middelschulte > > --- > > submodule.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/submodule.c b/submodule.c > > index 74d35b257..0198a72e6 100644 > > --- a/submodule.c > > +++ b/submodule.c > > @@ -1817,10 +1817,12 @@ int merge_submodule(struct object_id *result, const char *path, > > /* Case #1: a is contained in b or vice versa */ > > if (in_merge_bases(commit_a, commit_b)) { > > oidcpy(result, b); > > + warning("Fast-forwarding submodule %s", path); > > return 1; > > } > > if (in_merge_bases(commit_b, commit_a)) { > > oidcpy(result, a); > > + warning("Fast-forwarding submodule %s", path); > > return 1; > > } > > The code looks correct, however I think we can improve it. > (Originally I was just wondering if stderr is the right output, > which lead me to the thoughts below:) I’ve had the same thoughts about stderr. However, I thought that using a log function named `warning` to warn the user would be the right choice. If anything, I thought, the warning function might need refactoring. > Looking through the code of merge-recursive.c, > all the other merge outputs are done via 'output()' > that is able to buffer up the output as well as handles > the output for different verbosity settings. > > So I would think we should make the output() function available > outside of merge-recursive.c. (and rename it to a be more concise > and descriptive in the global namespace) and make use of it. Sure, let me know what to use instead and I’ll update and resubmit the patch. > > Funnily we already have MERGE_WARNING in submodule.c > which outputs information for all the other cases. I would think > we ought to convert those to the output(), too. Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to merge submodule“. > > Thanks, > Stefan Thank you, Leif ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 0/2] Submodule merging: i18n, verbosity 2018-05-10 20:30 ` Leif Middelschulte @ 2018-05-10 21:19 ` Stefan Beller 2018-05-10 21:19 ` [PATCH 1/2] submodule.c: move submodule merging to merge-recursive.c Stefan Beller ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Stefan Beller @ 2018-05-10 21:19 UTC (permalink / raw) To: leif.middelschulte; +Cc: git, gitster, sandals, sbeller Leif wrote: > Sure, let me know what to use instead and I’ll update and resubmit the patch. > Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to > merge submodule“. I thought about replying and coming up with good reasons, but I wrote some patches instead. They can also be found at https://github.com/stefanbeller/git/tree/submodule_i18n_verbose I think these would be a good foundation for your patch as well, as you can use the output() function for the desired cases. Feel free to take these patches as part of your series or adapt (or be inspired by) as needed. Thanks, Stefan Stefan Beller (2): submodule.c: move submodule merging to merge-recursive.c merge-recursive: i18n submodule merge output and respect verbosity merge-recursive.c | 169 +++++++++++++++++++++++++++++++++++++++++++++- submodule.c | 168 +-------------------------------------------- submodule.h | 6 +- 3 files changed, 170 insertions(+), 173 deletions(-) -- 2.17.0.255.g8bfb7c0704 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] submodule.c: move submodule merging to merge-recursive.c 2018-05-10 21:19 ` [PATCH 0/2] Submodule merging: i18n, verbosity Stefan Beller @ 2018-05-10 21:19 ` Stefan Beller 2018-05-10 21:19 ` [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller 2018-05-11 0:04 ` [PATCH 0/2] Submodule merging: i18n, verbosity Elijah Newren 2 siblings, 0 replies; 15+ messages in thread From: Stefan Beller @ 2018-05-10 21:19 UTC (permalink / raw) To: leif.middelschulte; +Cc: git, gitster, sandals, sbeller In a later patch we want to improve submodule merging by using the output() function in merge-recursive.c for submodule merges to deliver a consistent UI to users. To do so we could either make the output() function globally available so we can use it in submodule.c#merge_submodule(), or we could integrate the submodule merging into the merging code. Choose the later as we generally want to move submodules closer into the core. Therefore we move any function related to merging submodules (merge_submodule(), find_first_merges() and print_commit) to merge-recursive.c. We'll keep add_submodule_odb() in submodule.c as it is used by other submodule functions. While at it, add a TODO note that we do not really like the function add_submodule_odb(). This commit is best viewed with --color-moved. Signed-off-by: Stefan Beller <sbeller@google.com> --- merge-recursive.c | 166 +++++++++++++++++++++++++++++++++++++++++++++ submodule.c | 168 +--------------------------------------------- submodule.h | 6 +- 3 files changed, 170 insertions(+), 170 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 0c0d48624da..700ba15bf88 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -23,6 +23,7 @@ #include "merge-recursive.h" #include "dir.h" #include "submodule.h" +#include "revision.h" struct path_hashmap_entry { struct hashmap_entry e; @@ -977,6 +978,171 @@ static int merge_3way(struct merge_options *o, return merge_status; } +static int find_first_merges(struct object_array *result, const char *path, + struct commit *a, struct commit *b) +{ + int i, j; + struct object_array merges = OBJECT_ARRAY_INIT; + struct commit *commit; + int contains_another; + + char merged_revision[42]; + const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path", + "--all", merged_revision, NULL }; + struct rev_info revs; + struct setup_revision_opt rev_opts; + + memset(result, 0, sizeof(struct object_array)); + memset(&rev_opts, 0, sizeof(rev_opts)); + + /* get all revisions that merge commit a */ + xsnprintf(merged_revision, sizeof(merged_revision), "^%s", + oid_to_hex(&a->object.oid)); + init_revisions(&revs, NULL); + rev_opts.submodule = path; + /* FIXME: can't handle linked worktrees in submodules yet */ + revs.single_worktree = path != NULL; + setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts); + + /* save all revisions from the above list that contain b */ + if (prepare_revision_walk(&revs)) + die("revision walk setup failed"); + while ((commit = get_revision(&revs)) != NULL) { + struct object *o = &(commit->object); + if (in_merge_bases(b, commit)) + add_object_array(o, NULL, &merges); + } + reset_revision_walk(); + + /* Now we've got all merges that contain a and b. Prune all + * merges that contain another found merge and save them in + * result. + */ + for (i = 0; i < merges.nr; i++) { + struct commit *m1 = (struct commit *) merges.objects[i].item; + + contains_another = 0; + for (j = 0; j < merges.nr; j++) { + struct commit *m2 = (struct commit *) merges.objects[j].item; + if (i != j && in_merge_bases(m2, m1)) { + contains_another = 1; + break; + } + } + + if (!contains_another) + add_object_array(merges.objects[i].item, NULL, result); + } + + object_array_clear(&merges); + return result->nr; +} + +static void print_commit(struct commit *commit) +{ + struct strbuf sb = STRBUF_INIT; + struct pretty_print_context ctx = {0}; + ctx.date_mode.type = DATE_NORMAL; + format_commit_message(commit, " %h: %m %s", &sb, &ctx); + fprintf(stderr, "%s\n", sb.buf); + strbuf_release(&sb); +} + +#define MERGE_WARNING(path, msg) \ + warning("Failed to merge submodule %s (%s)", path, msg); + +static int merge_submodule(struct object_id *result, const char *path, + const struct object_id *base, const struct object_id *a, + const struct object_id *b, int search) +{ + struct commit *commit_base, *commit_a, *commit_b; + int parent_count; + struct object_array merges; + + int i; + + /* store a in result in case we fail */ + oidcpy(result, a); + + /* we can not handle deletion conflicts */ + if (is_null_oid(base)) + return 0; + if (is_null_oid(a)) + return 0; + if (is_null_oid(b)) + return 0; + + if (add_submodule_odb(path)) { + MERGE_WARNING(path, "not checked out"); + return 0; + } + + if (!(commit_base = lookup_commit_reference(base)) || + !(commit_a = lookup_commit_reference(a)) || + !(commit_b = lookup_commit_reference(b))) { + MERGE_WARNING(path, "commits not present"); + return 0; + } + + /* check whether both changes are forward */ + if (!in_merge_bases(commit_base, commit_a) || + !in_merge_bases(commit_base, commit_b)) { + MERGE_WARNING(path, "commits don't follow merge-base"); + return 0; + } + + /* Case #1: a is contained in b or vice versa */ + if (in_merge_bases(commit_a, commit_b)) { + oidcpy(result, b); + return 1; + } + if (in_merge_bases(commit_b, commit_a)) { + oidcpy(result, a); + return 1; + } + + /* + * Case #2: There are one or more merges that contain a and b in + * the submodule. If there is only one, then present it as a + * suggestion to the user, but leave it marked unmerged so the + * user needs to confirm the resolution. + */ + + /* Skip the search if makes no sense to the calling context. */ + if (!search) + return 0; + + /* find commit which merges them */ + parent_count = find_first_merges(&merges, path, commit_a, commit_b); + switch (parent_count) { + case 0: + MERGE_WARNING(path, "merge following commits not found"); + break; + + case 1: + MERGE_WARNING(path, "not fast-forward"); + fprintf(stderr, "Found a possible merge resolution " + "for the submodule:\n"); + print_commit((struct commit *) merges.objects[0].item); + fprintf(stderr, + "If this is correct simply add it to the index " + "for example\n" + "by using:\n\n" + " git update-index --cacheinfo 160000 %s \"%s\"\n\n" + "which will accept this suggestion.\n", + oid_to_hex(&merges.objects[0].item->oid), path); + break; + + default: + MERGE_WARNING(path, "multiple merges found"); + for (i = 0; i < merges.nr; i++) + print_commit((struct commit *) merges.objects[i].item); + } + + object_array_clear(&merges); + return 0; +} + static int merge_file_1(struct merge_options *o, const struct diff_filespec *one, const struct diff_filespec *a, diff --git a/submodule.c b/submodule.c index 74d35b25779..654089b3647 100644 --- a/submodule.c +++ b/submodule.c @@ -153,7 +153,8 @@ void stage_updated_gitmodules(struct index_state *istate) die(_("staging updated .gitmodules failed")); } -static int add_submodule_odb(const char *path) +/* TODO: remove this function, use repo_submodule_init instead. */ +int add_submodule_odb(const char *path) { struct strbuf objects_directory = STRBUF_INIT; int ret = 0; @@ -1701,171 +1702,6 @@ int submodule_move_head(const char *path, return ret; } -static int find_first_merges(struct object_array *result, const char *path, - struct commit *a, struct commit *b) -{ - int i, j; - struct object_array merges = OBJECT_ARRAY_INIT; - struct commit *commit; - int contains_another; - - char merged_revision[42]; - const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path", - "--all", merged_revision, NULL }; - struct rev_info revs; - struct setup_revision_opt rev_opts; - - memset(result, 0, sizeof(struct object_array)); - memset(&rev_opts, 0, sizeof(rev_opts)); - - /* get all revisions that merge commit a */ - xsnprintf(merged_revision, sizeof(merged_revision), "^%s", - oid_to_hex(&a->object.oid)); - init_revisions(&revs, NULL); - rev_opts.submodule = path; - /* FIXME: can't handle linked worktrees in submodules yet */ - revs.single_worktree = path != NULL; - setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts); - - /* save all revisions from the above list that contain b */ - if (prepare_revision_walk(&revs)) - die("revision walk setup failed"); - while ((commit = get_revision(&revs)) != NULL) { - struct object *o = &(commit->object); - if (in_merge_bases(b, commit)) - add_object_array(o, NULL, &merges); - } - reset_revision_walk(); - - /* Now we've got all merges that contain a and b. Prune all - * merges that contain another found merge and save them in - * result. - */ - for (i = 0; i < merges.nr; i++) { - struct commit *m1 = (struct commit *) merges.objects[i].item; - - contains_another = 0; - for (j = 0; j < merges.nr; j++) { - struct commit *m2 = (struct commit *) merges.objects[j].item; - if (i != j && in_merge_bases(m2, m1)) { - contains_another = 1; - break; - } - } - - if (!contains_another) - add_object_array(merges.objects[i].item, NULL, result); - } - - object_array_clear(&merges); - return result->nr; -} - -static void print_commit(struct commit *commit) -{ - struct strbuf sb = STRBUF_INIT; - struct pretty_print_context ctx = {0}; - ctx.date_mode.type = DATE_NORMAL; - format_commit_message(commit, " %h: %m %s", &sb, &ctx); - fprintf(stderr, "%s\n", sb.buf); - strbuf_release(&sb); -} - -#define MERGE_WARNING(path, msg) \ - warning("Failed to merge submodule %s (%s)", path, msg); - -int merge_submodule(struct object_id *result, const char *path, - const struct object_id *base, const struct object_id *a, - const struct object_id *b, int search) -{ - struct commit *commit_base, *commit_a, *commit_b; - int parent_count; - struct object_array merges; - - int i; - - /* store a in result in case we fail */ - oidcpy(result, a); - - /* we can not handle deletion conflicts */ - if (is_null_oid(base)) - return 0; - if (is_null_oid(a)) - return 0; - if (is_null_oid(b)) - return 0; - - if (add_submodule_odb(path)) { - MERGE_WARNING(path, "not checked out"); - return 0; - } - - if (!(commit_base = lookup_commit_reference(base)) || - !(commit_a = lookup_commit_reference(a)) || - !(commit_b = lookup_commit_reference(b))) { - MERGE_WARNING(path, "commits not present"); - return 0; - } - - /* check whether both changes are forward */ - if (!in_merge_bases(commit_base, commit_a) || - !in_merge_bases(commit_base, commit_b)) { - MERGE_WARNING(path, "commits don't follow merge-base"); - return 0; - } - - /* Case #1: a is contained in b or vice versa */ - if (in_merge_bases(commit_a, commit_b)) { - oidcpy(result, b); - return 1; - } - if (in_merge_bases(commit_b, commit_a)) { - oidcpy(result, a); - return 1; - } - - /* - * Case #2: There are one or more merges that contain a and b in - * the submodule. If there is only one, then present it as a - * suggestion to the user, but leave it marked unmerged so the - * user needs to confirm the resolution. - */ - - /* Skip the search if makes no sense to the calling context. */ - if (!search) - return 0; - - /* find commit which merges them */ - parent_count = find_first_merges(&merges, path, commit_a, commit_b); - switch (parent_count) { - case 0: - MERGE_WARNING(path, "merge following commits not found"); - break; - - case 1: - MERGE_WARNING(path, "not fast-forward"); - fprintf(stderr, "Found a possible merge resolution " - "for the submodule:\n"); - print_commit((struct commit *) merges.objects[0].item); - fprintf(stderr, - "If this is correct simply add it to the index " - "for example\n" - "by using:\n\n" - " git update-index --cacheinfo 160000 %s \"%s\"\n\n" - "which will accept this suggestion.\n", - oid_to_hex(&merges.objects[0].item->oid), path); - break; - - default: - MERGE_WARNING(path, "multiple merges found"); - for (i = 0; i < merges.nr; i++) - print_commit((struct commit *) merges.objects[i].item); - } - - object_array_clear(&merges); - return 0; -} - /* * Embeds a single submodules git directory into the superprojects git dir, * non recursively. diff --git a/submodule.h b/submodule.h index e5526f6aaab..b96689ac0db 100644 --- a/submodule.h +++ b/submodule.h @@ -89,10 +89,8 @@ extern int submodule_uses_gitfile(const char *path); #define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<1) #define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2) extern int bad_to_remove_submodule(const char *path, unsigned flags); -extern int merge_submodule(struct object_id *result, const char *path, - const struct object_id *base, - const struct object_id *a, - const struct object_id *b, int search); + +int add_submodule_odb(const char *path); /* Checks if there are submodule changes in a..b. */ extern int submodule_touches_in_range(struct object_id *a, -- 2.17.0.255.g8bfb7c0704 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity 2018-05-10 21:19 ` [PATCH 0/2] Submodule merging: i18n, verbosity Stefan Beller 2018-05-10 21:19 ` [PATCH 1/2] submodule.c: move submodule merging to merge-recursive.c Stefan Beller @ 2018-05-10 21:19 ` Stefan Beller 2018-05-15 1:25 ` Elijah Newren 2018-05-11 0:04 ` [PATCH 0/2] Submodule merging: i18n, verbosity Elijah Newren 2 siblings, 1 reply; 15+ messages in thread From: Stefan Beller @ 2018-05-10 21:19 UTC (permalink / raw) To: leif.middelschulte; +Cc: git, gitster, sandals, sbeller The submodule merge code now uses the output() function that is used by all the rest of the merge-recursive-code. This allows for respecting internationalisation as well as the verbosity setting. Signed-off-by: Stefan Beller <sbeller@google.com> --- merge-recursive.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 700ba15bf88..a4b91d17f87 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1048,18 +1048,17 @@ static void print_commit(struct commit *commit) strbuf_release(&sb); } -#define MERGE_WARNING(path, msg) \ - warning("Failed to merge submodule %s (%s)", path, msg); - -static int merge_submodule(struct object_id *result, const char *path, +static int merge_submodule(struct merge_options *o, + struct object_id *result, const char *path, const struct object_id *base, const struct object_id *a, - const struct object_id *b, int search) + const struct object_id *b) { struct commit *commit_base, *commit_a, *commit_b; int parent_count; struct object_array merges; int i; + int search = !o->call_depth; /* store a in result in case we fail */ oidcpy(result, a); @@ -1073,21 +1072,21 @@ static int merge_submodule(struct object_id *result, const char *path, return 0; if (add_submodule_odb(path)) { - MERGE_WARNING(path, "not checked out"); + output(o, 1, _("Failed to merge submodule %s (not checked out)"), path); return 0; } if (!(commit_base = lookup_commit_reference(base)) || !(commit_a = lookup_commit_reference(a)) || !(commit_b = lookup_commit_reference(b))) { - MERGE_WARNING(path, "commits not present"); + output(o, 1, _("Failed to merge submodule %s (commits not present)"), path); return 0; } /* check whether both changes are forward */ if (!in_merge_bases(commit_base, commit_a) || !in_merge_bases(commit_base, commit_b)) { - MERGE_WARNING(path, "commits don't follow merge-base"); + output(o, 1, _("Failed to merge submodule %s (commits don't follow merge-base)"), path); return 0; } @@ -1116,25 +1115,24 @@ static int merge_submodule(struct object_id *result, const char *path, parent_count = find_first_merges(&merges, path, commit_a, commit_b); switch (parent_count) { case 0: - MERGE_WARNING(path, "merge following commits not found"); + output(o, 1, _("Failed to merge submodule %s (merge following commits not found)"), path); break; case 1: - MERGE_WARNING(path, "not fast-forward"); - fprintf(stderr, "Found a possible merge resolution " - "for the submodule:\n"); + output(o, 1, _("Failed to merge submodule %s (not fast-forward)"), path); + output(o, 1, _("Found a possible merge resolution for the submodule:\n")); print_commit((struct commit *) merges.objects[0].item); - fprintf(stderr, + output(o, 1, _( "If this is correct simply add it to the index " "for example\n" "by using:\n\n" " git update-index --cacheinfo 160000 %s \"%s\"\n\n" - "which will accept this suggestion.\n", + "which will accept this suggestion.\n"), oid_to_hex(&merges.objects[0].item->oid), path); break; default: - MERGE_WARNING(path, "multiple merges found"); + output(o, 1, _("Failed to merge submodule %s (multiple merges found)"), path); for (i = 0; i < merges.nr; i++) print_commit((struct commit *) merges.objects[i].item); } @@ -1205,12 +1203,11 @@ static int merge_file_1(struct merge_options *o, return ret; result->clean = (merge_status == 0); } else if (S_ISGITLINK(a->mode)) { - result->clean = merge_submodule(&result->oid, + result->clean = merge_submodule(o, &result->oid, one->path, &one->oid, &a->oid, - &b->oid, - !o->call_depth); + &b->oid); } else if (S_ISLNK(a->mode)) { switch (o->recursive_variant) { case MERGE_RECURSIVE_NORMAL: -- 2.17.0.255.g8bfb7c0704 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity 2018-05-10 21:19 ` [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller @ 2018-05-15 1:25 ` Elijah Newren 0 siblings, 0 replies; 15+ messages in thread From: Elijah Newren @ 2018-05-15 1:25 UTC (permalink / raw) To: Stefan Beller; +Cc: Leif Middelschulte, Git Mailing List, Junio C Hamano I know I said the patches looked okay earlier, but I just noticed something... On Thu, May 10, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> wrote: > case 1: > - MERGE_WARNING(path, "not fast-forward"); > - fprintf(stderr, "Found a possible merge resolution " > - "for the submodule:\n"); > + output(o, 1, _("Failed to merge submodule %s (not fast-forward)"), path); We allow folks to set GIT_MERGE_VERBOSITY to change how much output they get. A setting of 1 should only show conflicts or major warnings. 2 is the default and adds a few more messages (e.g. "Auto-merging $PATH", "Adding $PATH" for one-sided adds, etc.), higher levels show even more. Anyway this output message is correct to use level 1 since this is a conflict, but... > + output(o, 1, _("Found a possible merge resolution for the submodule:\n")); I think this should use level 2. > print_commit((struct commit *) merges.objects[0].item); > - fprintf(stderr, > + output(o, 1, _( > "If this is correct simply add it to the index " > "for example\n" > "by using:\n\n" > " git update-index --cacheinfo 160000 %s \"%s\"\n\n" >- "which will accept this suggestion.\n", >+ "which will accept this suggestion.\n"), > oid_to_hex(&merges.objects[0].item->oid), path); and so should this one (in fact, I'm tempted to say these last two should use level 3, but since it looks like a command users may have difficulty finding on their own, I'm okay with going with 2). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] Submodule merging: i18n, verbosity 2018-05-10 21:19 ` [PATCH 0/2] Submodule merging: i18n, verbosity Stefan Beller 2018-05-10 21:19 ` [PATCH 1/2] submodule.c: move submodule merging to merge-recursive.c Stefan Beller 2018-05-10 21:19 ` [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller @ 2018-05-11 0:04 ` Elijah Newren 2018-05-11 1:00 ` Stefan Beller 2 siblings, 1 reply; 15+ messages in thread From: Elijah Newren @ 2018-05-11 0:04 UTC (permalink / raw) To: Stefan Beller Cc: leif.middelschulte, Git Mailing List, Junio C Hamano, sandals On Thu, May 10, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> wrote: > Leif wrote: >> Sure, let me know what to use instead and I’ll update and resubmit the patch. >> Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to >> merge submodule“. > > I thought about replying and coming up with good reasons, but I wrote some > patches instead. > > They can also be found at https://github.com/stefanbeller/git/tree/submodule_i18n_verbose > > I think these would be a good foundation for your patch as well, as you can use the > output() function for the desired cases. > > Feel free to take these patches as part of your series or adapt > (or be inspired by) as needed. This is awesome. In addition to the good reasons you gave, switching merge_submodule() to use output() was one of several things on my todo list since I think it'd be needed for remerge-diffs (https://bugs.chromium.org/p/git/issues/detail?id=12) and might be useful for merges in bare repos; thanks for tackling it. Patches look good to me. Having Leif's patch on top of these two would be great. Elijah ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] Submodule merging: i18n, verbosity 2018-05-11 0:04 ` [PATCH 0/2] Submodule merging: i18n, verbosity Elijah Newren @ 2018-05-11 1:00 ` Stefan Beller 2018-05-14 20:57 ` [PATCH 0/1] rebased: inform about auto submodule ff during merge Leif Middelschulte 0 siblings, 1 reply; 15+ messages in thread From: Stefan Beller @ 2018-05-11 1:00 UTC (permalink / raw) To: Elijah Newren Cc: Leif Middelschulte, Git Mailing List, Junio C Hamano, sandals Hi Elijah, On Thu, May 10, 2018 at 5:04 PM, Elijah Newren <newren@gmail.com> wrote: > On Thu, May 10, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> wrote: >> Leif wrote: >>> Sure, let me know what to use instead and I’ll update and resubmit the patch. >>> Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to >>> merge submodule“. >> >> I thought about replying and coming up with good reasons, but I wrote some >> patches instead. >> >> They can also be found at https://github.com/stefanbeller/git/tree/submodule_i18n_verbose >> >> I think these would be a good foundation for your patch as well, as you can use the >> output() function for the desired cases. >> >> Feel free to take these patches as part of your series or adapt >> (or be inspired by) as needed. > > This is awesome. In addition to the good reasons you gave, switching > merge_submodule() to use output() was one of several things on my todo > list since I think it'd be needed for remerge-diffs > (https://bugs.chromium.org/p/git/issues/detail?id=12) and might be > useful for merges in bare repos; thanks for tackling it. Thanks for the encouraging words! The one nit I find on that series is that we need to rely on and export the add_submodule_odb function as I want to get rid of that function once the object store series has progressed far enough. > > Patches look good to me. Having Leif's patch on top of these two > would be great. ok, Let's go with that. Stefan ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 0/1] rebased: inform about auto submodule ff during merge 2018-05-11 1:00 ` Stefan Beller @ 2018-05-14 20:57 ` Leif Middelschulte 2018-05-14 20:57 ` [PATCH 1/1] Inform about fast-forwarding of submodules " Leif Middelschulte 0 siblings, 1 reply; 15+ messages in thread From: Leif Middelschulte @ 2018-05-14 20:57 UTC (permalink / raw) To: git; +Cc: gitster, Leif Middelschulte From: Leif Middelschulte <Leif.Middelschulte@gmail.com> This patch is in response to Stefan Beller's Commit 0357af480 ("merge-recursive: i18n submodule merge output and respect verbosity", 2018-05-10) and is based on the changes it provided. Leif Middelschulte (1): Inform about fast-forwarding of submodules during merge merge-recursive.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.15.1 (Apple Git-101) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/1] Inform about fast-forwarding of submodules during merge 2018-05-14 20:57 ` [PATCH 0/1] rebased: inform about auto submodule ff during merge Leif Middelschulte @ 2018-05-14 20:57 ` Leif Middelschulte 2018-05-15 0:41 ` Stefan Beller 2018-05-15 1:17 ` Elijah Newren 0 siblings, 2 replies; 15+ messages in thread From: Leif Middelschulte @ 2018-05-14 20:57 UTC (permalink / raw) To: git; +Cc: gitster, Leif Middelschulte From: Leif Middelschulte <Leif.Middelschulte@gmail.com> Inform the user about an automatically fast-forwarded submodule. The silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward merge for submodules", 2010-07-07)). Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com> --- merge-recursive.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/merge-recursive.c b/merge-recursive.c index a4b91d17f..4a03044d1 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1093,10 +1093,14 @@ static int merge_submodule(struct merge_options *o, /* Case #1: a is contained in b or vice versa */ if (in_merge_bases(commit_a, commit_b)) { oidcpy(result, b); + output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path); + output_commit_title(o, commit_b); return 1; } if (in_merge_bases(commit_b, commit_a)) { oidcpy(result, a); + output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit:"), path); + output_commit_title(o, commit_a); return 1; } -- 2.15.1 (Apple Git-101) ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge 2018-05-14 20:57 ` [PATCH 1/1] Inform about fast-forwarding of submodules " Leif Middelschulte @ 2018-05-15 0:41 ` Stefan Beller 2018-05-15 1:17 ` Elijah Newren 1 sibling, 0 replies; 15+ messages in thread From: Stefan Beller @ 2018-05-15 0:41 UTC (permalink / raw) To: Leif Middelschulte; +Cc: git, Junio C Hamano On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte <leif.middelschulte@gmail.com> wrote: > From: Leif Middelschulte <Leif.Middelschulte@gmail.com> > > Inform the user about an automatically fast-forwarded submodule. The silent merge > behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward > merge for submodules", 2010-07-07)). > > Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com> Thanks for following up with a patch. This looks good to me! Thanks, Stefan > --- > merge-recursive.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/merge-recursive.c b/merge-recursive.c > index a4b91d17f..4a03044d1 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -1093,10 +1093,14 @@ static int merge_submodule(struct merge_options *o, > /* Case #1: a is contained in b or vice versa */ > if (in_merge_bases(commit_a, commit_b)) { > oidcpy(result, b); > + output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path); > + output_commit_title(o, commit_b); > return 1; > } > if (in_merge_bases(commit_b, commit_a)) { > oidcpy(result, a); > + output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit:"), path); > + output_commit_title(o, commit_a); > return 1; > } > > -- > 2.15.1 (Apple Git-101) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge 2018-05-14 20:57 ` [PATCH 1/1] Inform about fast-forwarding of submodules " Leif Middelschulte 2018-05-15 0:41 ` Stefan Beller @ 2018-05-15 1:17 ` Elijah Newren 2018-05-15 7:34 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Elijah Newren @ 2018-05-15 1:17 UTC (permalink / raw) To: Leif Middelschulte; +Cc: Git Mailing List, Junio C Hamano Hi Leif, On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte <leif.middelschulte@gmail.com> wrote: Thanks for updating the patch on top of Stefan's series. :-) > /* Case #1: a is contained in b or vice versa */ > if (in_merge_bases(commit_a, commit_b)) { > oidcpy(result, b); > + output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path); > + output_commit_title(o, commit_b); Level 1 is for conflicts; I don't think this message should have higher priority than "Auto-merging $PATH" for normal files, so it needs to be 2 (or maybe 3, see below) rather than 1. (The default output level is 2, so it'd still be shown, but we do allow people to remove informational message and just get conflicts by setting GIT_MERGE_VERBOSITY to 1, or request extra information by setting it higher) Also, this two-line message seems somewhat verbose compared to the other messages in merge_submdoule(), and when compared to the simple "Auto-merging $PATH" we do for normal files. The multi-line nature of it particularly strikes me; the merge-recursive code has generally avoided multi-line messages even for conflicts. In comparison, your original patch just had ("Fast-forwarding submodule %s", path). Maybe you could "if (show(o, 3)) { output your current message } else { output the simpler message }" ? Or is this verbosity warranted for submodules at the default print level? I'm not a heavy user of submodules, so I may need to get others to weigh in on the verbosity and multi-line aspects, but I wanted to at least flag this as somewhat surprising to me. Elijah ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge 2018-05-15 1:17 ` Elijah Newren @ 2018-05-15 7:34 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2018-05-15 7:34 UTC (permalink / raw) To: Elijah Newren; +Cc: Leif Middelschulte, Git Mailing List Elijah Newren <newren@gmail.com> writes: > Hi Leif, > > On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte > <leif.middelschulte@gmail.com> wrote: > > Thanks for updating the patch on top of Stefan's series. :-) > >> /* Case #1: a is contained in b or vice versa */ >> if (in_merge_bases(commit_a, commit_b)) { >> oidcpy(result, b); >> + output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path); >> + output_commit_title(o, commit_b); > > Level 1 is for conflicts; I don't think this message should have > higher priority than "Auto-merging $PATH" for normal files, so it > needs to be 2 (or maybe 3, see below) rather than 1. (The default > output level is 2, so it'd still be shown, but we do allow people to > remove informational message and just get conflicts by setting > GIT_MERGE_VERBOSITY to 1, or request extra information by setting it > higher) > > Also, this two-line message seems somewhat verbose compared to the > other messages in merge_submdoule(), and when compared to the simple > "Auto-merging $PATH" we do for normal files. The multi-line nature of > it particularly strikes me; the merge-recursive code has generally > avoided multi-line messages even for conflicts. > > In comparison, your original patch just had ("Fast-forwarding > submodule %s", path). FWIW, I share both of your surprises. Between level 2 and 3, after skimming merge-recursive.c for existing use of output levels, I think the situation for non-submodule merges that is closest to these two cases the patch covers for submodules is probably the message given when content merge happened to end up with what we already had. It is part of a normal merge operation that is not a singificant event in the larger picture, yet it is rather rare and interesting when you are curious on events that occur infrequently. So a one-liner message as everybody else emitted at level 3 or more verbose would probably be a good balance with the remainder of the system, I would think. Thanks for a review. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-05-15 7:34 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-10 18:26 [PATCH 0/1] warn about auto fast-forwarded submodules during merges Leif Middelschulte 2018-05-10 18:26 ` [PATCH 1/1] Warn about fast-forwarding of submodules during merge Leif Middelschulte 2018-05-10 18:49 ` Stefan Beller 2018-05-10 20:30 ` Leif Middelschulte 2018-05-10 21:19 ` [PATCH 0/2] Submodule merging: i18n, verbosity Stefan Beller 2018-05-10 21:19 ` [PATCH 1/2] submodule.c: move submodule merging to merge-recursive.c Stefan Beller 2018-05-10 21:19 ` [PATCH 2/2] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller 2018-05-15 1:25 ` Elijah Newren 2018-05-11 0:04 ` [PATCH 0/2] Submodule merging: i18n, verbosity Elijah Newren 2018-05-11 1:00 ` Stefan Beller 2018-05-14 20:57 ` [PATCH 0/1] rebased: inform about auto submodule ff during merge Leif Middelschulte 2018-05-14 20:57 ` [PATCH 1/1] Inform about fast-forwarding of submodules " Leif Middelschulte 2018-05-15 0:41 ` Stefan Beller 2018-05-15 1:17 ` Elijah Newren 2018-05-15 7:34 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).