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, 15:34 UTC
Message-ID: <xmqqh8611eza.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1908281454070.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> FWIW I disagree with the decision to mingle a bug fix with a change of
> behavior. Resetting to the correct OID is of course the bug fix.
> Dropping the message is a change of behavior.

In general I strongly advocate that a patch should fix one thing and one thing well without breaking other things, so we are on the same page.

As I said in <xmqqftltqjy1.fsf@gitster-ct.c.googlers.com>, I think the message that is leaked from "reset --hard" was reporting an incorrect thing, iow, showing the message itself is another bug.

IIUC, the bug is twofold:

 - When --autostash creats a stash entry, the command attempts to
   reset the working tree and the tip of the current branch to where
   it should be (i.e. HEAD).  As we know, this attempt is faulty and
   resets to a wrong commit, not to HEAD.  This is the primary bug
   the patch under discussion fixes.

 - A message is given only when the above happens.  When rebasing
   from a clean working tree, we do not report "HEAD is now at..."
   at all.  And when autostash happens, the message is still not
   correct even after fixing to which commit we reset to.  "HEAD is
   now at ..." is misleading in that it implies that we changed to
   something else, but in reality, we have been on the same commit
   all the time since the command started, created a stash and wiped
   the working tree clean after doing so, when the message is given.
   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.

So, from the purist point of view, I see it may make sense to 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. We still need a follow up patch that removes the message to fix the other bug, perhaps with a follow-up to update the "first rewinding..." message, which is shown whether autostash kicks in or not, so that it reports which commit we are rebuilding the history.

Thans.