Re: [PATCH 3/3] status: do not report errors in sequencer/todo
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 25, 2019, 20:44 UTC
- Message-ID
- <xmqqo92le6ns.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <af4b823caac84899b5ac71da61af5ec00f88bb2f.1561457483.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 10 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> commit 4a72486de9 ("fix cherry-pick/revert status after commit",
> 2019-04-16) used parse_insn_line() to parse the first line of the todo
> list to check if it was a pick or revert. However if the todo list is
> left over from an old cherry-pick or revert and references a commit that
> no longer exists then parse_insn_line() prints an error message which is
> confusing for users [1]. Instead parse just the command name so that the
> user is alerted to the presence of stale sequencer state by status
> reporting that a cherry-pick or revert is in progress.I have to wonder what would happen when "git cherry-pick --continue" is given from such a stale state. It would have to fail as it would not know the shape of the change to be replayed, no?
Is it easy to detect and say "there is an abandoned state file from stale cherry-pick" and optionally offer to remove it (as we have no chance of continuing anyway)?
Or is it likely that such an effort would end up being wasted, as...
> Note that we should not be leaving stale sequencer state lying around
> (or at least not as often) after commit b07d9bfd17 ("commit/reset: try
> to clean up sequencer state", 2019-04-16)....this already happened?
> Also avoid printing an error message if for some reason the user has a > file called `sequencer` in $GIT_DIR.
I agree that it is not a good place for giving diagnostics, but getting ENOTDIR would be an indication that next time when the user wants to do a rebase, range pick or range revert, it would not work unless somebody removes that `sequencer` file, right? Are our users sufficiently covered on that front (e.g. giving "we cannot do this operation as $GIT_DIR/sequencer is in the way. please remove that file" would be OK, but silently removing and possibly losing info we didn't even look at may be bad)?
In any case, the "unable to open 'sequencer/todo'" you are removing with this patch was *not* about helping the issue above which is an unrelated tangent, so let me not be distracted by that ;-)
Show 11 quoted lines
> +test_expect_success 'status shows cherry-pick with invalid oid' ' > + mkdir .git/sequencer && > + test_write_lines "pick invalid-oid" >.git/sequencer/todo && > + git status --untracked-files=no >actual 2>err && > + git cherry-pick --quit && > + test_must_be_empty err && > + test_i18ncmp expected actual > +' > + > +test_expect_success 'status does not show error if .git/sequencer is a file' ' > + test_when_finished "rm .git/sequencer" &&
In short, I was wondering what happens if we lose this line, and then we later had a command that needs to use .git/sequencer/todo or something.
Show 8 quoted lines
> + test_write_lines hello >.git/sequencer && > + git status --untracked-files=no 2>err && > + test_must_be_empty err > +' > + > test_expect_success 'status showing detached at and from a tag' ' > test_commit atag tagging && > git checkout atag &&