Git status parse error after v.2.22.0 upgrade

Git status parse error after v.2.22.0 upgrade

[Espen Antonsen]

Seeing an error after upgrading to git v2.22.0.

git status shows “error: could not parse ‘*a commit message from 2012*’”

Downgrading to v.2.21.0 does not yield this message for git status.

I can only replicate this in a private repo I have so far. Unable to share the repo but can debug if you need more info.

Re: Git status parse error after v.2.22.0 upgrade

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

Hi Espen,

On Tue, 11 Jun 2019, Espen Antonsen wrote:

> Seeing an error after upgrading to git v2.22.0.
>
> git status shows “error: could not parse ‘*a commit message from 2012*’”
>
> Downgrading to v.2.21.0 does not yield this message for git status.
>
> I can only replicate this in a private repo I have so far. Unable to
> share the repo but can debug if you need more info.

Sounds like a hook gone haywire. Find out more by setting `GIT_TRACE=1`
before repeating the command.

Ciao,
Johannes

Re: Git status parse error after v.2.22.0 upgrade

[Espen Antonsen]

I don't have any hooks, just the default sample scripts. GIT_TRACE doesn't show anything else.

On Tue, 11 Jun 2019, at 21:28, Johannes Schindelin wrote:
> Hi Espen,
> 
> On Tue, 11 Jun 2019, Espen Antonsen wrote:
> 
> > Seeing an error after upgrading to git v2.22.0.
> >
> > git status shows “error: could not parse ‘*a commit message from 2012*’”
> >
> > Downgrading to v.2.21.0 does not yield this message for git status.
> >
> > I can only replicate this in a private repo I have so far. Unable to
> > share the repo but can debug if you need more info.
> 
> Sounds like a hook gone haywire. Find out more by setting `GIT_TRACE=1`
> before repeating the command.
> 
> Ciao,
> Johannes
>

Re: Git status parse error after v.2.22.0 upgrade

[Johannes Schindelin]

Hi Espen,

On Wed, 12 Jun 2019, Espen Antonsen wrote:

> I don't have any hooks, just the default sample scripts. GIT_TRACE
> doesn't show anything else.

Okay, it was just a guess.

And here is another guess: unless you impart with more details about your
test case, I guess there is little anybody else can do to help you.

Ciao,
Johannes

Re: Git status parse error after v.2.22.0 upgrade

[aleksandrs]

Hello,

Managed to repro this problem on my private repo too.

Bisect points to b51a0fdc3822c2ef260f6d496b6df6d33b101e8a
However, I think the real culprit is 4a72486de97b5c6b0979b2b51e50c268bdb0d4f6, specifically `parse_insn_line` call.

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".

--
Regards,
Aleksandrs Ļedovskis

On Wed, Jun 12, 2019 at 08:47:27PM +0200, Johannes Schindelin wrote:
> Hi Espen,
> 
> On Wed, 12 Jun 2019, Espen Antonsen wrote:
> 
> > I don't have any hooks, just the default sample scripts. GIT_TRACE
> > doesn't show anything else.
> 
> Okay, it was just a guess.
> 
> And here is another guess: unless you impart with more details about your
> test case, I guess there is little anybody else can do to help you.
> 
> Ciao,
> Johannes

Re: Git status parse error after v.2.22.0 upgrade

[Junio C Hamano]

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".

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.

Thanks for a report.

Re: Git status parse error after v.2.22.0 upgrade

[Jeff King]

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".
> 
> 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? 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.

-Peff

Re: Git status parse error after v.2.22.0 upgrade

[SZEDER Gábor]

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".
> 
> 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

It may or may not be related, but...  Some weeks (months?) ago I run
into a situation where 'git rebase' didn't clean up after itself
following a well-timed ctrl-C, and got confused to the point that not
even a subsequent 'git rebase --abort' was able to rectify the
situation.

So, I wanted to rebase just a couple of commits to somewhere else, but
messed up the command's parameters, and it then tried to rebase a
couple hundred commits.  Upon noticing the unexpectedly large numbers
in the "Generating patches" progress line I hit ctrl-C, and then the
aborting 'git rebase' apparently left an incomplete/corrupted
'.git/rebase-apply/' directory behind.  Unfortunately I didn't think
about saving the precious corrupted state, and deleted that dir right
away...  only saved the transcript from the terminal later:

  ~/src/git (test-atexit)$ git rebase js/test-atexit 
  First, rewinding head to replay your work on top of it...
  Generating patches: 100% (509/509), done.
  ^C
  ~/src/git ((42e6cb0046...)|REBASE 1/509)$ gitk js/test-atexit 
  ~/src/git ((42e6cb0046...)|REBASE 1/509)$ git rebase --abort 
  error: could not read '.git/rebase-apply/head-name': No such file or directory
  ~/src/git ((42e6cb0046...)|REBASE 1/509)$ git reset --hard
  HEAD is now at 42e6cb0046 git-p4: use `test_atexit` to kill the daemon
  ~/src/git ((42e6cb0046...)|REBASE 1/509)$ rm -rf .git/re
  rebase-apply/    rebased-patches  refs/            
  ~/src/git ((42e6cb0046...)|REBASE 1/509)$ rm -rf .git/rebase-apply/
  ~/src/git ((42e6cb0046...))$ git checkout -

Re: Git status parse error after v.2.22.0 upgrade

[Aleksandrs Ļedovskis]

On Thu, Jun 13, 2019 at 09:05:16AM -0700, Junio C Hamano wrote:
> are you getting it when you run "git status" command???

Yes, the regular old porcelain `git-status(1)`.

$ git status
error: could not parse '<<full contents of .git/sequencer/todo here>>'
On branch <<some-branch>>
nothing to commit, working tree clean

--
Best,
Aleksandrs Ļedovskis

Re: Git status parse error after v.2.22.0 upgrade

[Phillip Wood]

On 13/06/2019 17:24, Jeff King wrote:
> 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.

>> 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.

> 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
> 

Re: Git status parse error after v.2.22.0 upgrade

[Jeff King]

On Thu, Jun 13, 2019 at 06:43:41PM +0100, Phillip Wood wrote:

> >>  (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.

OK, that makes a lot more sense to me. It did make me wonder if doing an
"e"dit on the final commit of a rebase followed by "reset HEAD^" would
clear the sequencer state. But looking at that commit, this kicks in
only for the cherry-pick and revert cases.

Thanks for explaining.

> 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.

IMHO that opens up a can of worms, because the reachability rules get
more complicated (so for example people trying to prune away unwanted
history get confused about what is still mentioning the objects). That's
a reasonable rare case. OTOH, so is mentioning commits in an ongoing
rebase that are not still reachable from the branch you are rebasing,
nor the HEAD reflog.

-Peff

Re: Git status parse error after v.2.22.0 upgrade

[Phillip Wood]

On 13/06/2019 18:43, Phillip Wood wrote:
> On 13/06/2019 17:24, Jeff King wrote:
>> On Thu, Jun 13, 2019 at 09:05:16AM -0700, Junio C Hamano wrote:
>>>
>>> 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.
> 
>> 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.

When I wrote that it hadn't dawned on me that if there is an error the 
status will not tell the user that a cherry-pick is in progress which is 
what we really want so they are alerted to the stale sequencer state. 
I've posted a series [1] to address this (sadly gitgitgadget wont let me 
post it on this thread).

Best Wishes

Phillip

[1] 
https://public-inbox.org/git/pull.275.git.gitgitgadget@gmail.com/T/#mf57a4ab95ba907fbf2d06ec64e9b676db158eace


> 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
>>
>