Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2019, 16:03 UTC
Message-ID: <xmqq8srd1dld.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqh8611eza.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 7 quoted lines

> IIUC, the bug is twofold:
> ...
>  - A message is given only when the above happens.  When rebasing
> ...
>    That "reset --hard" is done only to clean the index and the
>    working tree and talking about "HEAD is now..." is a bug in its
>    context.

Actually, this "latter" bug can further be split into two

 * The "HEAD is now" is given only when autostash feature needs to
   clean the working tree, and we have never moved HEAD anyway.

 * The message does not indicate what we are rebuilding on top of.

and dealt with separately, so with that in mind the step that would follow the first patch, i.e.

Show 5 quoted lines

> ... update
> this patch to add logic to give a pointless and misleading "HEAD is
> now at..." message so that we will report the location of HEAD where
> the "rebase --autostash" command started at, to fix only the first
> bug.

may become different. The fact that the "HEAD is now..." is given only when autostash actually happens _might_ be taken as a feature by some users---the location of HEAD reported by the message is irrelevant to them (we know that as a fact---we have been reporting a wrong commit all along anyway), but the single-bit "we got a message" is a signal that "--autostash" had something valuable to save.

So the second step may be to replace the "HEAD is now..." message we add back (relative to Ben's patch under discussion) to the first patch with a more direct "stashed away your local changes" message (perhaps with diffstat??? I do not care about the details, as we are talking about resurrecting one single useful bit of information and extending it futher is beyond the scope of this analysis).

And the last point, i.e. "First, rewinding head to replay your work..." does not give enough information to be truly useful, is a totally separate bug (that Ben's patch does not even mention or attempt to address), so we can leave it out of this analysis, too.

So, yeah, if we are to spend extra effort to polish Ben's patch further while keeping the "fix things without making unnecessary changes", I think the approach that takes least amount of effort may not to make the code manually say "Head is at ...", but to add a new message to report that autostash happened. That fixes two bugs (i.e. the original bug, and the "we autostashed" bit is reported in a roundabout and misleading way via "HEAD is now at ...") in a single patch ;-)