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

From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Ben Wijen <ben@wijen.net>,
	git@vger.kernel.org, Pratik Karki <predatoramigo@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
Date: Wed, 28 Aug 2019 08:34:01 -0700	[thread overview]
Message-ID: <xmqqh8611eza.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1908281454070.46@tvgsbejvaqbjf.bet> (Johannes Schindelin's message of "Wed, 28 Aug 2019 14:56:03 +0200 (CEST)")

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.