Re: [PATCH] merge-submodule: reduce output verbosity
Hello,
Am 11. Juni 2018 um 20:04:19, Junio C Hamano
(gits...@pobox.com(mailto:gits...@pobox.com)) schrieb:
> Leif Middelschulte writes:
>
> > From: Leif Middelschulte
> >
> > The output shall behave more similar to ordinary file merges' output to
> > provide
> > a more consistent user experience.
> >
> > Signed-off-by: Leif Middelschulte
> > ---
> > merge-recursive.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Thanks, both. Very much appreciated.
You are welcome. Thank all of you, who participated in the discussions
about this topic, for your patience, advice, and guidance. I am sorry
it took me so long to reply and provide the patches.
Cheers,
Leif
>
> >
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index ac27abbd4..5eb907f46 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -1208,7 +1208,7 @@ static int merge_submodule(struct merge_options *o,
> > output(o, 3, _("Fast-forwarding submodule %s to the following commit:"),
> > path);
> > output_commit_title(o, commit_b);
> > } else if (show(o, 2))
> > - output(o, 2, _("Fast-forwarding submodule %s to %s"), path,
> > oid_to_hex(b));
> > + output(o, 2, _("Fast-forwarding submodule %s"), path);
> > else
> > ; /* no output */
> >
> > @@ -1220,7 +1220,7 @@ static int merge_submodule(struct merge_options *o,
> > output(o, 3, _("Fast-forwarding submodule %s to the following commit:"),
> > path);
> > output_commit_title(o, commit_a);
> > } else if (show(o, 2))
> > - output(o, 2, _("Fast-forwarding submodule %s to %s"), path,
> > oid_to_hex(a));
> > + output(o, 2, _("Fast-forwarding submodule %s"), path);
> > else
> > ; /* no output */
[PATCH] merge-submodule: reduce output verbosity
From: Leif Middelschulte
The output shall behave more similar to ordinary file merges' output to provide
a more consistent user experience.
Signed-off-by: Leif Middelschulte
---
merge-recursive.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index ac27abbd4..5eb907f46 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1208,7 +1208,7 @@ static int merge_submodule(struct merge_options *o,
output(o, 3, _("Fast-forwarding submodule %s to the
following commit:"), path);
output_commit_title(o, commit_b);
} else if (show(o, 2))
- output(o, 2, _("Fast-forwarding submodule %s to %s"),
path, oid_to_hex(b));
+ output(o, 2, _("Fast-forwarding submodule %s"), path);
else
; /* no output */
@@ -1220,7 +1220,7 @@ static int merge_submodule(struct merge_options *o,
output(o, 3, _("Fast-forwarding submodule %s to the
following commit:"), path);
output_commit_title(o, commit_a);
} else if (show(o, 2))
- output(o, 2, _("Fast-forwarding submodule %s to %s"),
path, oid_to_hex(a));
+ output(o, 2, _("Fast-forwarding submodule %s"), path);
else
; /* no output */
--
2.15.1 (Apple Git-101)
[PATCH 1/1] merge-recursive: give notice when submodule commit gets fast-forwarded
From: Leif Middelschulte
Since submodules are treated similarly to ordinary files (i.e. not as 'dumb'
pointers), an automatic merge should be mentioned if the user asks for it.
Just as it is mentioned for oridnary files.
Signed-off-by: Leif Middelschulte
---
merge-recursive.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..0990a135b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1093,10 +1093,20 @@ 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);
+ if (show(o, 2))
+ output(o, 2, _("Auto-merging %s"), path);
+ else
+ ; /* no output */
+
return 1;
}
if (in_merge_bases(commit_b, commit_a)) {
oidcpy(result, a);
+ if (show(o, 2))
+ output(o, 2, _("Auto-merging %s"), path);
+ else
+ ; /* no output */
+
return 1;
}
--
2.15.1 (Apple Git-101)
[PATCH v4 0/1] merge-recursive: give notice when submodule commit gets fast-forwarded
From: Leif Middelschulte The provided patch is in response to Elijah Newren's [0] and Junio Hamano's [1] comments on my prior patch regarding the reasoning and implementation of a user notification during (clean) merges of submodules. [0] https://public-inbox.org/git/xmqqo9hg7554@gitster-ct.c.googlers.com/#t [1] https://public-inbox.org/git/xmqqzi0t1waf@gitster-ct.c.googlers.com/ Leif Middelschulte (1): Inform about Auto-merging of submodules during merge merge-recursive.c | 10 ++ 1 file changed, 10 insertions(+) -- 2.15.1 (Apple Git-101)
[PATCH v3 0/1] rebased: inform about auto submodule ff
From: Leif Middelschulte <leif.middelschu...@gmail.com> This is a follow-up on Junio C Hamano's comments [0] and Stefan Beller's request [1] for a more explainatory/elaborate commit message. [0] https://public-inbox.org/git/xmqqk1s474vx@gitster-ct.c.googlers.com/ [1] https://public-inbox.org/git/20180517184008.25445-1-sbel...@google.com/ Leif Middelschulte (1): Inform about fast-forwarding of submodules during merge merge-recursive.c | 16 1 file changed, 16 insertions(+) -- 2.15.1 (Apple Git-101)
[PATCH 1/1] Inform about fast-forwarding of submodules during merge
From: Leif Middelschulte <leif.middelschu...@gmail.com>
Silent fast-forwarding might lead to inconveniences in cases where
submodules are expected to have a certain revision, because 'more recent'
(therefore fast-forwardable) versions might break behavior/contain regressions.
A use-case is the integration (merge) phase as part of the feature-centric
'git-flow' workflow [0]. I.e. a feature might be well-tested with a certain
submodule revision, but break because of regressions (or changes in general)
within an updated version of the sourced submodule.
This change tries to support the integrator by telling her about another
possible
source of unexpected behavior (differing submodule versions) she might see
during integration tests.
[0] http://nvie.com/posts/a-successful-git-branching-model/
Signed-off-by: Leif Middelschulte <leif.middelschu...@gmail.com>
---
merge-recursive.c | 16
1 file changed, 16 insertions(+)
diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..e2c99924d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1093,10 +1093,26 @@ 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);
+ if (show(o, 3)) {
+ output(o, 3, _("Fast-forwarding submodule %s to the
following commit:"), path);
+ output_commit_title(o, commit_b);
+ } else if (show(o, 2))
+ output(o, 2, _("Fast-forwarding submodule %s to %s"),
path, oid_to_hex(b));
+ else
+ ; /* no output */
+
return 1;
}
if (in_merge_bases(commit_b, commit_a)) {
oidcpy(result, a);
+ if (show(o, 3)) {
+ output(o, 3, _("Fast-forwarding submodule %s to the
following commit:"), path);
+ output_commit_title(o, commit_a);
+ } else if (show(o, 2))
+ output(o, 2, _("Fast-forwarding submodule %s to %s"),
path, oid_to_hex(a));
+ else
+ ; /* no output */
+
return 1;
}
--
2.15.1 (Apple Git-101)
[PATCH v2 0/1] rebased: inform about auto submodule ff
From: Leif Middelschulte <leif.middelschu...@gmail.com> This is a follow-up on Junio C Hamano's comments [0] and Stefan Beller's request [1] for a more explainatory/elaborate commit message. [0] https://public-inbox.org/git/xmqqk1s474vx@gitster-ct.c.googlers.com/ [1] https://public-inbox.org/git/20180517184008.25445-1-sbel...@google.com/ 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)
Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive
Hello Stefan,
thank you once again for your effort.
Am 15. Mai 2018 um 22:00:34, Stefan Beller
(sbel...@google.com(mailto:sbel...@google.com)) schrieb:
> This rerolls the two commits found at [1] with the feedback of Eliah
> and puts Leifs patch[2] on top, that I edited according to Eliahs feedback,
> but kept Leifs ownership.
>
> This has addressed all of Eliahs feedback AFAICT.
> You'll find a branch-diff below[3], which lacks
> the new patch of Leif in that series, but is part of the reroll?
>
> Leif, what do you think?
Seems great to me. Thank you for picking up and improving my changes :)
One Question though: Shouldn’t an enum (like
NOTES_MERGE_VERBOSITY_DEFAULT) be used instead of numbers?
Cheers,
Leif
>
> Thanks,
> Stefan
>
> [1] https://public-inbox.org/git/20180510211917.138518-1-sbel...@google.com/
> [2]
> https://public-inbox.org/git/20180514205737.21313-2-leif.middelschu...@gmail.com/
> [3] git branch-diff
> origin/master..origin/sb/submodule-merge-in-merge-recursive
> origin/master..HEAD >>-cover-letter.patch
>
> Leif Middelschulte (1):
> Inform about fast-forwarding of submodules during merge
>
> Stefan Beller (2):
> submodule.c: move submodule merging to merge-recursive.c
> merge-recursive: i18n submodule merge output and respect verbosity
>
> merge-recursive.c | 185 +-
> submodule.c | 168 +
> submodule.h | 6 +-
> 3 files changed, 186 insertions(+), 173 deletions(-)
>
> --
> 2.17.0.582.gccdcbd54c44.dirty
>
>
>
> 1: e022c7976ae ! 1: 3b638ccac64 submodule.c: move submodule merging to
> merge-recursive.c
> @@ -20,7 +20,6 @@
> This commit is best viewed with --color-moved.
>
> Signed-off-by: Stefan Beller
> - Signed-off-by: Junio C Hamano
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> --- a/merge-recursive.c
> 2: 2c02ece7e01 ! 2: eb43110df9d merge-recursive: i18n submodule merge output
> and respect verbosity
> @@ -7,7 +7,6 @@
> internationalisation as well as the verbosity setting.
>
> Signed-off-by: Stefan Beller
> - Signed-off-by: Junio C Hamano
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> --- a/merge-recursive.c
> @@ -73,10 +72,10 @@
> - 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"));
> ++ output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
> print_commit((struct commit *) merges.objects[0].item);
> - fprintf(stderr,
> -+ output(o, 1, _(
> ++ output(o, 2, _(
> "If this is correct simply add it to the index "
> "for example\n"
> "by using:\n\n"
> -: --- > 3: 4a3bc435023 Inform about fast-forwarding of submodules
> during merge
[PATCH 1/1] Inform about fast-forwarding of submodules during merge
From: Leif Middelschulte <leif.middelschu...@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.middelschu...@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)
[PATCH 0/1] rebased: inform about auto submodule ff during merge
From: Leif Middelschulte <leif.middelschu...@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)
Re: [PATCH 1/1] Warn about fast-forwarding of submodules during merge
Hi Stefan,
Am 10. Mai 2018 um 20:49:39, Stefan Beller
(sbel...@google.com(mailto:sbel...@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
[PATCH 1/1] Warn about fast-forwarding of submodules during merge
From: Leif Middelschulte <leif.middelschu...@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.middelschu...@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)
[PATCH 0/1] warn about auto fast-forwarded submodules during merges
From: Leif Middelschulte <leif.middelschu...@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=152544498723355=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)
