Re: [PATCH 3/3] status: do not report errors in sequencer/todo
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 26, 2019, 08:57 UTC
- Message-ID
- <51d31257-7093-a0fa-9681-929b9fea059c@gmail.com>
- In-Reply-To
- <xmqqo92le6ns.fsf@gitster-ct.c.googlers.com>
Hi Junio
Thanks for the comments
On 25/06/2019 21:44, Junio C Hamano wrote:
Show 16 quoted lines
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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?Yes it would fail with error: invalid line <line-no>: <line> error: unusable instruction sheet: '.git/sequencer/todo' fatal: cherry-pick failed
> 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)?
I guess we could have a specific error return if get_oid() failed and suggest that then
Show 7 quoted lines
> 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?Probably. It is still possible for the user to run checkout in the middle of a cherry-pick and forget to finish it but if they're using a prompt with git support it should tell them that a cherry-pick is in progress as `git status` detects that it is.
Show 11 quoted lines
>> 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)?
mkdir() will fail with ENOTDIR which should be reported with error_errno() I think.
Best Wishes
Phillip
Show 28 quoted lines
> 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 ;-) > >> +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. > >> + 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 &&