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

[Junio C Hamano <gitster@pobox.com>]

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

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

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