From: Phillip Wood Date: Wed, 26 Jun 2019 08:57:58 GMT Subject: Re: [PATCH 3/3] status: do not report errors in sequencer/todo Message-ID: <51d31257-7093-a0fa-9681-929b9fea059c@gmail.com> In-Reply-To: Hi Junio Thanks for the comments On 25/06/2019 21:44, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood >> >> 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 : 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 &&