Re: [RFC PATCH v2 0/2] bisect: add a single command for editing logs

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2017, 05:35 UTC
Message-ID: <xmqqshd6ub8f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <cover.1511200589.git.adam@dinwoodie.org>

Adam Dinwoodie <adam@dinwoodie.org> writes:

Show 6 quoted lines

> - It's possible to start a bisect session with a command like `git
>   bisect @ @~10`.  This will lead to the bisect log including the `@`
>   and `@~10` literally, and the interpretation of those values changes
>   depending on the current HEAD.  As a result, if you do a `git bisect
>   edit` after starting a bisect like that, but don't actually edit the
>   file, you'll nonetheless be in a different state.

This is a tangent, but for writing to the general public, please do spell out HEAD, not the line noise synonym "@" that confuses readers.

Show 7 quoted lines

>   I can see a few ways of coping with that:
>
>   1. Change the existing `git bisect start` behaviour to run arguments
>      through `git rev-parse` before recording them.  It appears `git
>      bisect good` et al. already do that, but it is a change in
>      behaviour that I guess could impact badly on other people using
>      `git bisect log`-based workflows.

The issue is not just HEAD but also for anything fruid, i.e. the name of a branch, a search result ":/pattern", etc., and if we want to allow restarting a previously failed bisect session from a midpoint, we should be recording things in absolute terms as early as possible. I'd think it was an oversight the "log" thing did not do so.

>   2. Do a full `git bisect reset` before replaying the log, so the
>      revisions will be parsed in the same way as they were originally.
>      I'd be slightly sad about that, as it seems an unnecessary
>      inefficiency, but it may well be the simplest approach.

It is not just inefficient, but would require there is no a local change; I thought that the current system allows you to have a local modification to a path that is not involved in the bisect session and losing that property would be sad.

> - There aren't yet any tests or documentation changes; I wanted to get
>   commentary on the initial code changes before I spent time on those
>   parts.

There are some chicken-and-egg around this area. For some changes, without a doc update and test addition, it is harder to judge if a reviewer can agree with the proposed change, as there is only a high level description "we allow editing" and the lowest level changes to the actual code, without anything in between that describes the guiding principle and design decision that lead to the patch.

I'll need to see if the changes in the patch is clear/trivial enough to see where you are trying to go to see if it is the case for this patch, though, so read the above paragraph as a general guideline.