Hi Junio
Thanks for the comments
On 25/06/2019 21:44, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
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 &&
