Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Junio C Hamano]

Elijah Newren  writes:

> Thanks for continuing to push on this.  This looks good so far (to
> me), but I was also hoping to see the analogy between these messages
> and "Auto-merging $FILE" for regular files mentioned.  Both Junio[1]
> and I[2] pointed out this similarity, and I think this
> similarity/analogy is useful additional motivation for making this
> change.

... meaning that it should be discussed and named as the primary
reason why this change is a good idea?

Re-reading what Leif wrote in the first paragraph, I tend to think
that "the more recent version may break us" Leif gives is not a
particularly convincing one.  After all, if we did not change the
commit bound at a submodule since we forked, while they changed it
to something else (either old or new), even though our changes may
have been fully tested with the version of the submodule we have
been testing with, it may break with the version the merged branch
has been using.  Such an update is cleanly and silently resolved at
the tree-level three-way merge, but the risk of breakage is no
different to the case this patch adds new notices to.

More importantly, the same "the changes we made may get broken by
changes in areas that are textually unrelated they made" will happen
without submodules.  Content-level three-way merges that resolves
cleanly at the textual level may need to get semantic adjustment.
Do we treat clean 3-way content merges as suspicious and give a
similar warning?  That smells like madness.

But as you said, we give "Auto-merging $FILE" notice to clean 3-way
merge at the content-level for normal files, and there is no good
reason why we should not do the same for submodules when one
fast-forwards to the other, which is an analogue to the
content-level 3-way merge where one branch's version is a superset
of the other ones.  And that is quite a convincing reason why a new
"Auto-merging $SUBMODULE" notice is a good idea.

> ...
> Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
> on these two lines feels out of place.  Users can just look at the
> submodule to see what it was updated to.  In a sea of output from
> merging, this extra detail feels like noise for the standard use-case,
> unless I'm misunderstanding how submodules are special.

Now you meantion it, that part of the message does look more like a
debugging aid than a feature that helps actual end-users.  After
all, if our side did not change the commit recorded for the
submodule while their side changed, we do not report the result of
such a tree-level three-way merge that takes what commit they had at
their tip.

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Elijah Newren]

Hi Leif,

On Fri, May 18, 2018 at 12:48 PM, Leif Middelschulte
 wrote:
> From: Leif Middelschulte 
>
> 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.

Thanks for continuing to push on this.  This looks good so far (to
me), but I was also hoping to see the analogy between these messages
and "Auto-merging $FILE" for regular files mentioned.  Both Junio[1]
and I[2] pointed out this similarity, and I think this
similarity/analogy is useful additional motivation for making this
change.

[1] https://public-inbox.org/git/xmqqo9hg7554@gitster-ct.c.googlers.com/
[2] 
https://public-inbox.org/git/CABPp-BGaibCPWuCnaX5Af=sv-2zvyhncupt+-pkxhdfjbg_...@mail.gmail.com/


> +   } else if (show(o, 2))
> +   output(o, 2, _("Fast-forwarding submodule %s to %s"), 
> path, oid_to_hex(b));
...
> +   } else if (show(o, 2))
> +   output(o, 2, _("Fast-forwarding submodule %s to %s"), 
> path, oid_to_hex(a));

Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
on these two lines feels out of place.  Users can just look at the
submodule to see what it was updated to.  In a sea of output from
merging, this extra detail feels like noise for the standard use-case,
unless I'm misunderstanding how submodules are special.  Junio also
commented on this in the same email referenced above (at [1]).  Is
there a reason this is an important piece of the message for you to be
shown at the standard merge verbosity?

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Junio C Hamano]

Elijah Newren  writes:

> Hi Leif,
>
> On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
>  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.

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Elijah Newren]

Hi Leif,

On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
 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

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Stefan Beller]

On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
 wrote:
> From: Leif Middelschulte 
>
> 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 

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)
>