From: Elijah Newren <newren@gmail.com>
Date: Fri, 18 May 2018 21:25:26 GMT
Subject: Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge
Message-ID: <CABPp-BGGe3r_QiC5264xkj0cp2Vu6WoLQZeDGEJi4eOpUW9z3g@mail.gmail.com>
In-Reply-To: <20180518194802.28355-2-leif.middelschulte@gmail.com>

Hi Leif,

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

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.fsf@gitster-ct.c.googlers.com/
[2] https://public-inbox.org/git/CABPp-BGaibCPWuCnaX5Af=sv-2zvyhNcupT+-PkxHDfJBg_Vbw@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?