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
