| Message ID | 20190821182941.12674-1-ben@wijen.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase.c: make sure the active branch isn't moved when autostashing | expand |
Dscho's review got me thinking about the rationale behind the 'HEAD is now at...' message. A 'stash push' is followed by a 'reset -q' but since 'stash create autostash' is not, we must do it ourselves. I guess the legacy implementation could have been 'reset --hard -q' which would have also prevented the 'HEAD is now at...' message. Ofcourse I'm happy to reinstate the message, but I'm convinced it doesn't add information, as with this commit the original branch is no longer moved and - as before - the autostash is re-applied after the rebase, leaving nothing to be guessed about. Thank you, Ben Wijen (1): rebase.c: make sure the active branch isn't moved when autostashing builtin/rebase.c | 18 ++++++------------ t/t3420-rebase-autostash.sh | 12 ++++++++---- 2 files changed, 14 insertions(+), 16 deletions(-)
Hi Ben, On Mon, 26 Aug 2019, Ben Wijen wrote: > Dscho's review got me thinking about the rationale behind the 'HEAD is now at...' > message. > > A 'stash push' is followed by a 'reset -q' but since 'stash create autostash' is > not, we must do it ourselves. I guess the legacy implementation could have been > 'reset --hard -q' which would have also prevented the 'HEAD is now at...' message. > > Ofcourse I'm happy to reinstate the message, but I'm convinced it doesn't add > information, as with this commit the original branch is no longer moved and > - as before - the autostash is re-applied after the rebase, leaving nothing > to be guessed about. 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. I would be a lot more comfortable with a bug fix that did *not* change the behavior, fast-tracking that to even maintenance branches. And leaving the behavior change to cook in `next` for a while. Of course, I am not Git's maintainer, if I were, I would insist on this more careful approach. Ciao, Johannes
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.
Junio C Hamano <gitster@pobox.com> writes: > 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. > ... 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 ;-)