* Re: Git status parse error after v.2.22.0 upgrade
2019-06-13 16:05 ` Junio C Hamano
@ 2019-06-13 16:24 ` Jeff King
2019-06-13 17:43 ` Phillip Wood
2019-06-13 16:58 ` SZEDER Gábor
2019-06-13 17:11 ` Aleksandrs Ļedovskis
2 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2019-06-13 16:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: aleksandrs, Johannes Schindelin, Espen Antonsen, git
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Git status parse error after v.2.22.0 upgrade
2019-06-13 16:24 ` Jeff King
@ 2019-06-13 17:43 ` Phillip Wood
2019-06-13 19:00 ` Jeff King
2019-06-25 13:25 ` Phillip Wood
0 siblings, 2 replies; 12+ messages in thread
From: Phillip Wood @ 2019-06-13 17:43 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: aleksandrs, Johannes Schindelin, Espen Antonsen, git
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Git status parse error after v.2.22.0 upgrade
2019-06-13 17:43 ` Phillip Wood
@ 2019-06-13 19:00 ` Jeff King
2019-06-25 13:25 ` Phillip Wood
1 sibling, 0 replies; 12+ messages in thread
From: Jeff King @ 2019-06-13 19:00 UTC (permalink / raw)
To: Phillip Wood
Cc: Junio C Hamano, aleksandrs, Johannes Schindelin, Espen Antonsen,
git
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Git status parse error after v.2.22.0 upgrade
2019-06-13 17:43 ` Phillip Wood
2019-06-13 19:00 ` Jeff King
@ 2019-06-25 13:25 ` Phillip Wood
1 sibling, 0 replies; 12+ messages in thread
From: Phillip Wood @ 2019-06-25 13:25 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: aleksandrs, Johannes Schindelin, Espen Antonsen, git
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
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Git status parse error after v.2.22.0 upgrade
2019-06-13 16:05 ` Junio C Hamano
2019-06-13 16:24 ` Jeff King
@ 2019-06-13 16:58 ` SZEDER Gábor
2019-06-13 17:11 ` Aleksandrs Ļedovskis
2 siblings, 0 replies; 12+ messages in thread
From: SZEDER Gábor @ 2019-06-13 16:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: aleksandrs, Johannes Schindelin, Espen Antonsen, git
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 -
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Git status parse error after v.2.22.0 upgrade
2019-06-13 16:05 ` Junio C Hamano
2019-06-13 16:24 ` Jeff King
2019-06-13 16:58 ` SZEDER Gábor
@ 2019-06-13 17:11 ` Aleksandrs Ļedovskis
2 siblings, 0 replies; 12+ messages in thread
From: Aleksandrs Ļedovskis @ 2019-06-13 17:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Espen Antonsen, git
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
^ permalink raw reply [flat|nested] 12+ messages in thread