Re: Git status parse error after v.2.22.0 upgrade

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 13, 2019, 17:43 UTC
Message-ID: <8981e6c8-eb20-fde7-2b4c-1fbf2057caef@gmail.com>
In-Reply-To: <20190613162432.GA12128@sigill.intra.peff.net>

On 13/06/2019 17:24, Jeff King wrote:

Show 8 quoted lines

> On Thu, Jun 13, 2019 at 09:05:16AM -0700, Junio C Hamano wrote:
> 
>> aleksandrs@ledovskis.lv writes:
>>
>>> My repo indeed contains a ".git/sequencer/todo" file which
>>> contains references to commits long-gone (i.e., rebased).
>>> Renaming or deleting this file stops whines about "error: could
>>> not parse".

I think this bug report is a result of 4a72486de9 ("fix cherry-pick/revert status after commit", 2019-04-16) which tries to read the sequencer todo file if it is present and there is no CHERRY_PICK_HEAD/REVERT_HEAD. Before that if the user committed a conflict resolution in the middle of a sequence of picks then status would forget that there was a cherry-pick in progress. It just reuses the code that cherry-pick uses to parse the first todo list item which expects the commits to exist.

Show 20 quoted lines

>> Interesting.  So in short, when the repository has leftover
>> sequencer state file that is not in use, "git status parse" thing
>> (whatever it is---are you getting it when you run "git status"
>> command???)---is not careful enough to notice that it does not
>> matter even if that leftover file is unusable.
>>
>> Two issues "the sequencer" folks may want to address are
>>
>>  (1) make the one that reads an irrelevant/stale 'todo' file more
>>      careful to ignore errors in such a file;
>>
>>  (2) make the sequencer machinery more careful to clean up after it
>>      is done or it is aborted  (for example, "git reset --hard"
>>      could remove these state files preemptively even when a rebase
>>      is not in progress, I would think).
>>
>> I think we already had some patches toward the latter recently.
> 
> Maybe I am not understanding it correctly, but do you mean in (2) that
> "git reset --hard" would always clear sequencer state?

I think the commit Junio is referring to is b07d9bfd17 ("commit/reset: try to clean up sequencer state", 2019-04-16) which will only remove the sequencer directory if it stops after the pick was the last one in the series. The idea is that if cherry-pick stops for a conflict resolution on the last pick user commits the result directly or run reset without running `cherry-pick --continue` afterwards the sequencer state gets cleaned up properly.

Show 7 quoted lines

> That seems
> undesirable to me, as I often use "git reset" to move the head around
> during a rebase. (e.g., when using "rebase -i" to split apart I commit,
> I stop on that commit, "git reset" back to the parent, and then
> selectively "add -p" the various parts).
> 
> Direction (1) seems quite sensible to me, though.

Now that we try harder to clean up the sequencer state I wonder if that would just cover up bugs where the state has not been removed when it should have been. That can lead to unpleasant problems if the user aborts a single revert/cherry-pick when there is stale sequencer state around as it rewinds HEAD to the commit when the stale cherry-pick/revert was started as explained in the message to b07d9bfd17 ("commit/reset: try to clean up sequencer state", 2019-04-16)

If we do want to do something then maybe teaching gc not to collect commits listed in .git/sequencer/todo and .git/rebase-merge/git-rebase-todo would be useful.

Best Wishes

Phillip

> -Peff
>