Re: [PATCH 3/3] status: do not report errors in sequencer/todo

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Junio

Thanks for the comments

On 25/06/2019 21:44, Junio C Hamano wrote:
> "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

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

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

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