patchrebase -i: do not update "done" when rescheduling command

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As the sequencer executes todo commands it appends them to .git/rebase-merge/done. This file is used by "git status" to show the recently executed commands. Unfortunately when a command is rescheduled the command preceding it is erroneously appended to the "done" file. This means that when rebase stops after rescheduling "pick B" the "done" file contains

	pick A
	pick B
	pick A

instead of

	pick A
	pick B

Fix this by not updating the "done" file when adding a rescheduled command back into the "todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file.

Note that the rescheduled command will still be appended to the "done" file again when it is successfully executed. Arguably it would be better not to do that but fixing it would be more involved.

Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
    rebase -i: do not update "done" when rescheduling command
    
    This fixes the output of "git status" when rebase stops for a
    rescheduled command.
    
    Stefan - thanks for reporting this, hopefully it will be easy enough to
    update lazygit to check the last command in the done file as well as the
    second to last command for older versions of git.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1492

 sequencer.c                   | 15 +++++++--------
 t/t3404-rebase-interactive.sh | 27 +++++++++++++++++----------
 t/t3430-rebase-merges.sh      | 22 ++++++++++++++++------
 3 files changed, 40 insertions(+), 24 deletions(-)

On 19.03.23 15:48, Phillip Wood via GitGitGadget wrote:

Thanks for the fix. Yes, it should be easy to adapt lazygit to the new behavior.

-Stefan

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Here it may not be clear what you meant with the verb "reschedule" to those who weren't closely following the previous discussion that led to this fix.

Is it the same as "the command attempted to execute a step, couldn't complete it (e.g. due to conflicts), and gave control to the end user until they say 'git rebase --continue'"? What cases, other than interrupted step due to conflicts, involve "rescheduling"?

And without quite understanding what "reschedule" refers to, it is unclear why it is even arguable---it is perfectly sensible that a command that is rescheduled (hence not yet done) would not be sent to 'done'. If a command that was once rescheduled (hence it wasn't finished initially) gets finished now, shouldn't it be sent to 'done'? It is unclear why is it better not to.

OK, all callers to save_todo() are in pick_commits() that knows what the value of "reschedule" is, and it is passed down to this helper ...

... and the change here is quite straight-forward. With reschedule, we do not advance because by definition we haven't finished the step yet. OK.

I wonder why we pass a hardcoded 0 here---shouldn't the value match the local variable 'reschedule'? here?

The same question for the other two callers, but I admit that when the second one is called, the local variable "reschedule" is not set...

... yet we call the helper with reschedule set to 1.  Puzzled.

At this point, reschedule is set and passing it instead of 1 would be OK.

Thanks.

Hi Junio

Thanks for your comments

On 20/03/2023 17:46, Junio C Hamano wrote:

I'll expand the commit message to explain that if we cannot pick a commit because it would overwrite untracked files then we add the command back into the todo list and give control to the user until they say 'git rebase --continue'. Hopefully they'll have removed the problematic files and we try to pick the commit again it will succeed. We do the same if an exec command fails and --reschedule-failed-exec was given. For conflicts we don't add the command back into the todo list because the cherry-pick has happened the user "just" needs to fix the conflicts before continuing to the next command.

The command is only successfully executed once but may end up in 'done' multiple times. While that means we can see which commands ended up being rescheduled I'm not sure it is very useful and think really we're just cluttering the 'done' file with failed attempts.

The rescheduling code is a bit of a mess as rescheduling commands that pick a commit does not use the "reschedule" variable and is handled separately to other commands like "reset", "merge" and "exec" which do use the "reschedule" varibale. I did try and add a preparatory step to fix that but failed to find a good way of doing so. The reason I went with hardcoded parameters is that for each call the purpose is fixed and as you noticed the "reschedule" variable is only used for rescheduling "reset", "merge" and "exec". I could expand the commit message or do you think a couple of code comments be more helpful?

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Sorry, but you utterly confused me. I thought the point of this change was to avoid such a failed attempt to be recorded in "done", and if that is the case, we (1) do not record any failing attempts, (2) record the successful execution, and (3) will not re-attempt once it is successful. And if all of these three hold, we wont clutter 'done' with failed attempts at all, no?

I see. It may be a sign, taken together with the fact that I found that it was very hard---even after reading the patch twice---to understand the verb "reschedule" in the proposed commit log to explain the change, that the concept of "reschedule" in this codepath may not be clearly capturing what it is attempting to do in the first place.

Yeah, at least it sounds like the code deserves a "NEEDSWORK: this is messy in such and such way and we need to clean it up to make it understandable" comment somehow.

Thanks.

Hi Junio

On 24/03/2023 15:49, Junio C Hamano wrote:

Perhaps I should have not have added that last paragraph to the commit message.

No. This change fixes a bug where when we add the failed command back into "git-rebase-todo" we accidentally add the previous command to the "done" file. If "pick A" succeeds and "pick B" fails because it would overwrite an untracked file then currently when the rebase stops after the failed "done" will contain

	pick A
	pick B
	pick A

When it should contain

	pick A
	pick B

i.e. the last line should be the last command we tried to execute.

unfortunately "done" is updated just before we try and execute the command so all the failing attempts are recorded. I'm not trying to change that in this patch, I mentioned it in the commit message as a suggestion for a future improvement.

Yes, unfortunately that's not how it works at the moment.

I'll try and come up with some better wording (if you have any suggestions please let me know). What's happening is that just before we try and execute a command it it removed from "git-rebase-todo" and added to "done". If the command then fails because it would overwrite an untracked file we need to add it back into "git-rebase-todo" before handing control to the user to remove the offending files. When the user runs "git rebase --continue" we'll try and execute the command again. It is the adding the command back into "git-rebase-todo" so that it is executed by "git rebase --continue" that "reschedule" was intended to capture.

The basic problem is that rebase updates "git-rebase-todo" and "done" before it has successfully executed the command (cherry-pick and revert only remove a command from their todo list after it is executed successfully). I fear it may be too late to change that now though.

I'll have another think about how we could clean it up, if that fails I'll add a code comment. I'll be offline next week so I'll re-roll after that.

Best Wishes

Phillip

Hi Phillip,

On Sun, 19 Mar 2023, Phillip Wood via GitGitGadget wrote:

I am purposefully not yet focusing on the patch, as I have a concern about the reasoning above.

When a command fails that needs to be rescheduled, I actually _like_ that there is a record in `done` about said command. It is very much like a `pick` that failed (but was not rescheduled) and was then `--skip`ed: it still shows up on `done`.

I do understand the concern that the rescheduled command now shows up in both `done` and `git-rebase-todo` (which is very different from the failed `pick` command that would show up _only_ in `git-rebase-todo`). So maybe we can find some compromise, e.g. adding a commented-out line to `done` à la:

	# rescheduled: pick A

What do you think?

Ciao, Johannes

Hi Dscho

Sorry for not replying sooner.

On 27/03/2023 08:04, Johannes Schindelin wrote:

We still do that after this patch. What changes is that when "pick B" fails we don't add "pick A" to the "done" file when "pick B" is added back into "git-rebase-todo"

If a commit is rescheduled we still end up with multiple entries in the "done". In the example above if "pick B" fails the first time it is executed and then succeeds on the second attempt "done" will contain

	pick A
	pick B
	pick B

It might be nice to mark it as rescheduled as you suggest but this series focuses on removing the incorrect entry from the "done" file, not de-duplicating the "done" entities when a command fails.

Best Wishes

Phillip

Hi Phillip,

On Thu, 3 Aug 2023, Phillip Wood wrote:

After having had plenty of time to let this issue simmer in the back of my head, and after reviewing the latest iteration of the patch series, I am no longer concerned, as it now sounds more logical to me, too, that rescheduled commands don't show up in `done`.

Thank you, Johannes

This series fixes several bugs in the way we handle a commit cannot be picked because it would overwrite an untracked file.

 * after a failed pick "git rebase --continue" will happily commit any
   staged changes even though no commit was picked.

 * the commit of the failed pick is recorded as rewritten even though no
   commit was picked.

 * the "done" file used by "git status" to show the recently executed
   commands contains an incorrect entry.

Thanks for the comments on V1, this series has now grown somewhat. Previously I was worried that refactoring would change the behavior, but having thought about it the current behavior is wrong and should be changed.

Changes since V1:

Rebased onto master to avoid a conflict with ab/remove-implicit-use-of-the-repository

 * Patches 1-3 are new preparatory changes
 * Patches 4 & 5 are new and fix the first two issues listed above.
 * Patch 6 is the old patch 1 which has been rebased and the commit message
   reworded. It fixes the last issues listed above.

Phillip Wood (6):
  rebase -i: move unlink() calls
  rebase -i: remove patch file after conflict resolution
  sequencer: factor out part of pick_commits()
  rebase --continue: refuse to commit after failed command
  rebase: fix rewritten list for failed pick
  rebase -i: fix adding failed command to the todo list

 sequencer.c                   | 170 ++++++++++++++++++----------------
 t/t3404-rebase-interactive.sh |  49 +++++++---
 t/t3430-rebase-merges.sh      |  35 +++++--
 t/t5407-post-rewrite-hook.sh  |  11 +++
 4 files changed, 165 insertions(+), 100 deletions(-)

base-commit: 9c6990cca24301ae8f82bf6291049667a0aef14b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1492

Range-diff vs v1:

 -:  ----------- > 1:  3dfb2c6903b rebase -i: move unlink() calls
 -:  ----------- > 2:  227aea031b5 rebase -i: remove patch file after conflict resolution
 -:  ----------- > 3:  31bb644e769 sequencer: factor out part of pick_commits()
 -:  ----------- > 4:  9356d14b09a rebase --continue: refuse to commit after failed command
 -:  ----------- > 5:  f8e64c1b631 rebase: fix rewritten list for failed pick
 1:  dc51a7499bc ! 6:  a836b049b90 rebase -i: do not update "done" when rescheduling command
     @@ Metadata
      Author: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## Commit message ##
     -    rebase -i: do not update "done" when rescheduling command
     +    rebase -i: fix adding failed command to the todo list
      
     -    As the sequencer executes todo commands it appends them to
     -    .git/rebase-merge/done. This file is used by "git status" to show the
     -    recently executed commands. Unfortunately when a command is rescheduled
     +    When rebasing commands are moved from the todo list in "git-rebase-todo"
     +    to the "done" file (which is used by "git status" to show the recently
     +    executed commands) just before they are executed. This means that if a
     +    command fails because it would overwrite an untracked file it has to be
     +    added back into the todo list before the rebase stops for the user to
     +    fix the problem.
     +
     +    Unfortunately when a failed command is added back into the todo list
          the command preceding it is erroneously appended to the "done" file.
     -    This means that when rebase stops after rescheduling "pick B" the "done"
     +    This means that when rebase stops after "pick B" fails the "done"
          file contains
      
                  pick A
     @@ Commit message
                  pick A
                  pick B
      
     -    Fix this by not updating the "done" file when adding a rescheduled
     -    command back into the "todo" file. A couple of the existing tests are
     +    Fix this by not updating the "done" file when adding a failed command
     +    back into the "git-rebase-todo" file. A couple of the existing tests are
          modified to improve their coverage as none of them trigger this bug or
          check the "done" file.
      
     -    Note that the rescheduled command will still be appended to the "done"
     -    file again when it is successfully executed. Arguably it would be better
     -    not to do that but fixing it would be more involved.
     -
          Reported-by: Stefan Haller <lists@haller-berlin.de>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ sequencer.c: static int pick_commits(struct repository *r,
       		int check_todo = 0;
       
      -		if (save_todo(todo_list, opts))
     -+		if (save_todo(todo_list, opts, 0))
     ++		if (save_todo(todo_list, opts, reschedule))
       			return -1;
       		if (is_rebase_i(opts)) {
       			if (item->command != TODO_COMMENT) {
     -@@ sequencer.c: static int pick_commits(struct repository *r,
     - 							    todo_list->current),
     - 				       get_item_line(todo_list,
     - 						     todo_list->current));
     --				todo_list->current--;
     --				if (save_todo(todo_list, opts))
     -+				if (save_todo(todo_list, opts, 1))
     - 					return -1;
     - 			}
     - 			if (item->command == TODO_EDIT) {
      @@ sequencer.c: static int pick_commits(struct repository *r,
       			       get_item_line_length(todo_list,
       						    todo_list->current),
       			       get_item_line(todo_list, todo_list->current));
      -			todo_list->current--;
      -			if (save_todo(todo_list, opts))
     -+			if (save_todo(todo_list, opts, 1))
     ++			if (save_todo(todo_list, opts, reschedule))
       				return -1;
       			if (item->commit)
     - 				return error_with_patch(r,
     + 				write_rebase_head(&item->commit->object.oid);
      
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' '
      +	head -n3 todo >expect &&
      +	test_cmp expect .git/rebase-merge/done &&
      +	rm file2 &&
     + 	test_path_is_missing .git/rebase-merge/author-script &&
     + 	test_path_is_missing .git/rebase-merge/patch &&
     + 	test_path_is_missing .git/MERGE_MSG &&
     +@@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
     + 	grep "error: you have staged changes in your working tree" err &&
     + 	git reset --hard HEAD &&
       	git rebase --continue &&
      -	test_cmp_rev HEAD I
      +	test_cmp_rev HEAD D &&

From: Phillip Wood <phillip.wood@dunelm.org.uk>

At the start of each iteration the loop that picks commits removes state files from the previous pick. However some of these are only written if there are conflicts so only need to be removed before starting the loop, not in each iteration.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

I do not doubt your reasoning is correct, but could you explain this a bit better?

I think the reason why others, e.g. author-script, need to be removed on every iteration is because the previous iteration that called do_pick_commit() can come back successfully after calling write_author_script(), and we would want to clear the deck before going into the next iteration, so I can guess that you meant by "if there are conflicts" that the loop will not iterate to the next step after conflicts happened (and these files like "amend" and "stopped-sha" may have been written)? The latter, i.e. the loop will not iterate any further, is the more direct reason to justify this change, I think, and it would help readers of "git log" to say so, instead of forcing them to infer "are conflicts" imply "hence loop will stop".

Is this a pure clean-up, or will there be behaviour change? I do not think there is with this patch alone, but does this change make future steps easier to understand or something?

IOW, the proposed log message may explain why this is not a wrong change to make, but it is unclear why this is a good change we want to have in this part of the series.

Thanks.

On 21/04/2023 18:22, Junio C Hamano wrote:

Yes, that's right. I'll expand the commit message when I re-roll.

It is a pure cleanup. I noticed it when adding the unlink() call in the next patch, it doesn't really have anything to do with the subject of this series, but I felt it was worth doing as a preparation for the next patch.

Best Wishes

Phillip

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When rebase stops for the user to resolve conflicts it writes a patch for the conflicting commit to .git/rebase-merge/patch. This file should be deleted when the rebase continues. As the path is now used in two different places rebase_path_patch() is added and used to obtain the path for the patch.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Could you describe the reason why this file "should" be deleted a bit better? Once the user edits the files in the working tree and tell "git rebase" with the "--continue" option that they finished helping the command, and the command creates a commit out of the resolution left by the user in the working tree and in the index, the patch may no longer is needed, so I can understand if this were "this file can be deleted"---in other words, again, this explains why such a change would not be a wrong change that hurts the users, but it does not explain why we want such a change very well. Is there a reason why a left-over patch file is a bad thing (perhaps causing end-user confusion upon seeing such a patch that apparently is for a much earlier step in the rebase in progress? If so, that might be a good justification to say we "should").

OK.

The patch text itself looks good in the sense that it correctly implements what the proposed log message claims it "should".

Thanks.

On 21/04/2023 20:01, Junio C Hamano wrote:

Yes that's the reason - we don't want a stale patch when we stop for another reason, I'll improve the commit message.

Best Wishes

Phillip

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

I was checking to see if we could remove buf or whether we are reusing it for unrelated reasons (which is a common Git-ism). We can't remove it because we reuse it, however...

this buf.buf was supposed to be the value we populated earlier - this should be rebase_path_patch() instead.

As an aside, I have a mild distaste the Git-ism of reusing "struct strbuf buf" - using a variable for just a single purpose and naming it as such makes these sorts of errors much easier to spot. That isn't something we need to fix here, I'm just venting a little :)

On 21/06/2023 21:14, Glen Choo wrote:

I had a look at that and we're using it to construct a path that we should obtain by calling rebase_path_message() - I'll add a fix when I re-roll.

Oh, well spotted, thanks for pointing that out.

I agree it can get confusing. We occasionally forget to call strbuf_reset() before reusing the buffer (my first contribution to git fixed such a case in 4ab867b8fc8 (rebase -i: fix reflog message, 2017-05-18)), or forget to remove a call to reset the buffer that is no-longer necessary when refactoring. However it does save quite a few calls to malloc()/free() in the rebase/sequencer code.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Wow, a patch from April commented in June and responded in July ;-).

I'll salvage the topic from the "will discard" bin and mark it again as "Expecting a reroll" in my draft of the next "What's cooking" report.

Thanks.

On 14/07/2023 17:51, Junio C Hamano wrote:

Thanks, sorry it has taken so long, I've been struggling to find time to work on this but hopefully will have a new version ready in the next couple of weeks

Best Wishes

Phillip

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This is simplifies a change in a later commit. If a pick fails we now return the error at then end of the loop body rather than returning early but there is no change in behavior.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 129 ++++++++++++++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 60 deletions(-)

On Fri, Apr 21, 2023 at 11:11 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

s/This is/This/

s/then end/the end/

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

The new pick_one_commit() function is pretty much verbatim copy/move from inside the "any command below SQUASH" block, and in the original code, the block returned with an error whenever res is not 0, with one exception that TODO_EDIT would return with 0 if there is no error (but still with a patch).

The new code that calls pick_one_commit() helper lets this exception case to return from the function in the "any command below SQUASH" block, but everything else falls through *and* eventually at the end of the outer block there is

	if (res)
		return res;

that makes us return from the function.

But there are now a few other things done after the if/else if/else cascade, namely

 * there is an extra "if (reschedule)" and "else if (rebase-i) etc"
   logic.

 * the todo_list->current counter is incremented

are done BEFORE that "if res is not zero return". I am not sure we can safely claim "there is no change in behaviour".

Am I missing something?

Thanks.

Hi Junio

On 21/04/2023 20:31, Junio C Hamano wrote:

There are two blocks that might be entered. One guarded by "if (reschedule)" - this is not entered because reschedlue is always zero when picking a commit. The other is guarded by "else if (is_rebase_i(opts) && check_todo && !res)" and so will not be entered when we want to return an error because "res" is non-zero in that case.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Perhaps the proposed log message can be updated to mention these to save time from "git log" readers?

The todo_list->current counter gets incremented before leaving the function now, but all three callers of pick_commits() immediately call todo_list_release(), the value, whether it is incremented or not, is discarded without getting looked at.

So it is not a noop, but the difference in behaviour does not become externally observable?

Thanks.

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a commit cannot be picked because it would overwrite an untracked file then "git rebase --continue" should refuse to commit any staged changes as the commit was not picked. Do this by using the existing check for a missing author script in run_git_commit() which prevents "rebase --continue" from committing staged changes after failed exec commands.

When fast-forwarding it is not necessary to write the author script as we're reusing an existing commit, not creating a new one. If a fast-forwarded commit is modified by an "edit" or "reword" command then the modification is committed with "git commit --amend" which reuses the author of the commit being amended so the author script is not needed. baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) changed run_git_commit() to allow a missing author script when rewording a commit. This changes extends that to allow a missing author script whenever the commit is being amended.

If we're not fast-forwarding then we must remove the author script if the pick fails.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 10 +++++-----
 t/t3404-rebase-interactive.sh |  8 ++++++++
 t/t3430-rebase-merges.sh      |  4 +++-
 3 files changed, 16 insertions(+), 6 deletions(-)

On Fri, Apr 21, 2023 at 11:12 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

s/changes/change/

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

It makes perfect sense to refuse blindly committing. But this makes me wonder what the procedure for the user to recover. In the simplest case, the untracked file may be expendable and the user would wish to easily redo the step after removing it? Like

	$ git rebase
	... stops with "merging will overwrite untracked 'foo'" ...
	$ git rebase --continue
	... refuses thanks to the fix in this step ...
	$ rm foo
	... now what?  "git rebase --redo"?  "git rebase --continue"?
	

Depending on the recovery procedure, this may or may not be a wise design decision, even though it may be the quickest way to implement it. If the recovery procedure involves redoing the failed step from scratch (i.e. "rm foo && git reset --hard && git rebase" would try to restart by replaying the failed step anew), then loss of author script file has no downside. If we want to salvage as much as what was done in the initial attempt, maybe not.

I'd prefer a comma before "it is not" here.

OK.

It is unclear to me what "This changes extends" refers to. Could you rephrase?

The changes described in these three paragraphs are about efforts that were made needed only because the approach chosen to stop "continue" from continuing in this situation happens to be to remove the author script. If it were done differently (perhaps by adding another flag file that "continue" pays attention to), none of them may be necessary?

I agree that this is the right location to add new code to stop "continue" from moving forward. I do not know if the "unlink the author script" is the right choice of such a new code, though.

Coming back to my first point, perhaps adding an advice() after this error() would help end users who see this error to learn what to do to move forward?

Before the pre-context is an attempt to run "git rebase -i" to replay a commit that adds file6, stop the sequence by marking a step as "edit" to take control back. Then we create file6 manually in the working tree and make sure that "git rebase --continue" fails in the pre-context we can see here.

The recovery I asked about earlier is done with "rm file6" in this case.

This is testing the implementation. The failed "continue" would have removed the file thanks to the change we saw earlier.

Then we make some edit, and try to "--continue". Why should this fail? Is it because the earlier "rebase --continue" that failed did not replay the original commit due to untracked file and the user needs to redo the step in its entirety before the working tree becomes ready to take any further changes?

And this "reset --hard" is another thing in the recovery procedure the user needs to take (the other one being the removal of file6 we have seen earlier). After that, "rebase --continue" will replay the step that was interrupted by the untracked file6 that was in the working tree. OK.

We of course do not want to become overly "helpful" and run "reset --hard" ourselves when we issue the "could not even attempt to merge" message, but when we step back and see what the user wanted to do, this is still not entirely satisfactory, is it?

My understanding of what the user wanted to do (and let's pretend that creation of file6 in the middle was merely for test writer's convenience and in the real scenario of what the user wanted to do, there was the file left untracked from the beginning before the rebase started) is to redo the A---F---I chain, making a bit of change to F when it is recreated, and then replay I on top of the result of tweaked F. But instead of allowing them to edit F by doing some modification to file1, we ended up forcing the user to discard the edit made to file1 with "reset --hard", and "continue" replayed I on top of the replayed F that "edit" did not have a chance to modify.

Of course, the user can now go back to A and replay F and I on top, essentially redoing the "rebase -i" with FAKE_LINES="edit 1 2" and this time, because untracked file6 is gone, it may work better and F may allow to be tweaked. But then it does not look any better than saying "git rebase --abort" before doing the final "continue", which will also allow the user to redo the whole thing from scratch again.

So, I dunno.

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

For someone unfamiliar with "git rebase" code, I think it is easy enough to gather that "rebase --continue" will refuse to accept staged changes if the author script is missing, so we are reusing that mechanism to achieve our desired effect. It's not obvious whether this might have unintended consequences (Are we reusing something unrelated for an unintended purpose?) or what alternatives exist (Is sequencer.c so complex that there isn't another way to do this?). It would have been helpful for me to see how these considerations factored into your decision.

As I understand it, the author script can now be missing in other circumstances, so we have to adjust the rest of the machinery to handle that case? If so, this seems to suggest that there are some unintended consequences.

Checking that the path is missing seems like testing implementation details. If so, I would prefer to remove this assertion here and elsewhere.

This seems reasonable.

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When rebasing commands are moved from the todo list in "git-rebase-todo" to the "done" file just before they are executed. This means that if a command fails because it would overwrite an untracked file it has to be added back into the todo list before the rebase stops for the user to fix the problem. Unfortunately the way this is done results in the failed pick being recorded as rewritten.

Fix this by not calling error_with_patch() for failed commands. The pick has failed so there is nothing to commit and therefore we do not want to set up the message file for committing staged changes when the rebase continues. This change means we no-longer write a patch for the failed command or display the error message printed by error_with_patch(). As the command has failed the patch isn't really useful in that case and REBASE_HEAD is still written so the user can inspect the commit associated with the failed command. Unless the user has disabled it we print an advice message that is more helpful than the message from error_with_patch(). If the advice is disabled the user will still see the messages from the merge machinery detailing the problem.

To simplify writing REBASE_HEAD in this case pick_one_commit() is modified to avoid duplicating the code that adds the failed command back into the todo list.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 19 +++++++------------
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 t/t3430-rebase-merges.sh      | 11 ++++++++---
 t/t5407-post-rewrite-hook.sh  | 11 +++++++++++
 4 files changed, 38 insertions(+), 15 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

I could not make the connection from the described problem to the proposed solution. In particular, I couldn't tell what about "the way this is done" that causes the incorrect behavior (e.g. are we failing to clean up something? are we writing the wrong set of metadata?).

So unfortunately , I wasn't sure how this solution would fix the problem, and I didn't dive too deeply into this patch.

This also seems to be testing implementation details, and if so, it would be worth removing them.

Hi Glen

On 21/06/2023 21:49, Glen Choo wrote:

Yes, on reflection that first paragraph is not very helpful. I've updated it to

git rebase keeps a list that maps the OID of each commit before it was rebased to the OID of the equivalent commit after the rebase. This list is used to drive the "post-rewrite" hook that is called at the end of a successful rebase. When a rebase stops for the user to resolve merge conflicts the OID of the commit being picked is written to ".git/rebase-merge/stopped-sha1" and when the rebase is continued that OID is added to the list of rewritten commits. Unfortunately when a commit cannot be picked because it would overwrite an untracked file we still write the "stopped-sha1" file and so when the rebase is continued the commit is added into the list of rewritten commits even though it has not been picked yet.

Hopefully that is more helpful

With the exception of the "patch" file which exists solely for the benefit of the user this is testing an invariant of the implementation which isn't ideal. I'm worried that removing these checks will mask some subtle regression in the future. I think it is unlikely that the names of these files will change in the future as we try to avoid changes that would cause a rebase to fail if git is upgraded while it has stopped for the user to resolve conflicts. I did think about whether we could add some BUG() statements to sequencer.c instead. Unfortunately I don't think it is that easy for the sequencer to know when these files should be missing without relying on the logic that we are tying to test.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Ah, yes that is much easier to visualise and understand. Thanks so much.

Unfortunately, it's been a while since I reviewed this patch, so forgive me if I'm rusty. So you're saying that this test is about checking invariants that we want to preserve between Git versions. I think that's a reasonable goal - I am slightly skeptical of whether we should be doing that ad-hoc like this, but I don't feel strongly about it.

IIRC, there was an earlier patch would be different from an where we tested that author-script is missing, but what we really want is for the pick to stop. Is the same thing happening here? E.g. is 'testing for missing stopped-sha' a stand-in for 'testing that the rewritten list is correct'? If so, it would be nice to test that specifically, but if that's infeasible, a clarifying comment will probably suffice.

Hi Glen

On 25/07/2023 17:46, Glen Choo wrote:

Not really. One of the reasons why testing the implementation rather than the user observable behavior is a bad idea is that when the implementation is changed the test is likely to start failing or keep passing without checking anything useful. I was trying to say that in this case we're unlikely to change this aspect of the implementation because it would be tricky to do so without inconveniencing users who upgrade git while rebase is stopped for a conflict resolution and so it is unlikely that this test will be affected by future changes to the implementation.

Yes this patch adds a test to t5407-post-rewrite-hook.sh to do that but it only checks a failing "pick" command. The reason I think it is useful to add these test_path_is_missing checks is that they are checking failing "squash" and "merge" commands as well. Maybe I should just bite the bullet see how tricky it is to extend the post-rewrite-hook test to cover those cases as well.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Ah, I see the difference. I think that's it's fair to assume that the names of the files will be fairly stable, though this series has made it clear to me that what each file does and when it is written is quite under-documented, and I wouldn't be surprised to see some of that change if we start to try to explain the inner workings to ourselves.

Yes, that would probably be a good idea. Maybe if we combined them into a test helper that checks all of "pick", "squash" and "merge", which also has the added benefit of being able to hide implementation details in case we decide to change them.

Hi Glen

On 26/07/2023 18:48, Glen Choo wrote:

That is certainly true

I agree. In the end I've removed the state file checks from the tests in favor of adding explicit checks that we refuse to commit staged changes and adding more cases to the test for the "post-rewrite" hook. I think it would probably be useful to add some assertions to the sequencer in a future series. We can assert things like "if this state file exists then so should these other ones" and "if this state file does not exist then these others should not" without relying on the logic in the sequencer. The sequencer assertions wouldn't know if the message file should exist but know what other files should exist if it does and the tests for committing staged changes then effectively check if the message file should exist.

Thanks for your comments on this series, I'll send a re-roll next week

Best Wishes

Phillip

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When rebasing commands are moved from the todo list in "git-rebase-todo" to the "done" file (which is used by "git status" to show the recently executed commands) just before they are executed. This means that if a command fails because it would overwrite an untracked file it has to be added back into the todo list before the rebase stops for the user to fix the problem.

Unfortunately when a failed command is added back into the todo list the command preceding it is erroneously appended to the "done" file. This means that when rebase stops after "pick B" fails the "done" file contains

	pick A
	pick B
	pick A

instead of

	pick A
	pick B

Fix this by not updating the "done" file when adding a failed command back into the "git-rebase-todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file.

Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 12 ++++++------
 t/t3404-rebase-interactive.sh | 27 +++++++++++++++++----------
 t/t3430-rebase-merges.sh      | 22 ++++++++++++++++------
 3 files changed, 39 insertions(+), 22 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

It's very interesting that an _earlier_ line would get appended on the _other_ side, I'd expect that even if lines get duplicated, their relative order would be preserved. I was not able to trace the sequence of events that got us here, so I could not make a connection from that to the solution. I didn't dig deeply into this patch either, so I will refrain from commenting on the solution.

I didn't look into how the test works, but I confirmed that it tests the exact scenario described in the commit message.

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

So much has changed since the original one, I did not even recognise this was a redoing of the "rescheduled step" topic (which I do not think I have kept in 'seen'). Building on top of the more recent 'master' like you did here is very much appreciated.

Hi Phillip!

We picked up this series during Review Club. You can browse the notes at

  https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1

but we'll post any substantial feedback back to the mailing list anyway.

Firstly, I have to acknowledge that this series appears to be geared towards to reviewers who are already familiar with the rebase machinery and don't require a lot of context on the changes. None of the Review Club attendees were familiar with it, so we had trouble following along certain patches, but we might not have been the intended audience anyway.

I can leave comments from that perspective, i.e. what would have been useful from _if_ this were written for folks who weren't already familiar with the rebase code, which could be useful if we were trying to train people to reviewer sequencer.c. However, I don't think this warrants substantial changes if the series is already clear to the intended audience.

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

This sounds like a safer default for most users, but I worry that there are seasoned users relying on the old behavior.

Sounds like a reasonable fix.

This also sounds reasonable, but from Johannes' upthread response, this sounds like this isn't universally agreed upon.

Perhaps the underlying issue is that the behavior of "git rebase --continue", and the todo/done lists is underspecified (in public and internal documentation), so we end up having to reestablish what the 'correct' behavior is (which will probably end up being just a matter of personal taste). This isn't strictly necessary for the series, but it would be nice for us to establish what the correct behavior _should be_ (even if we aren't there yet) and use that to guide future fixes.

This series fixes several bugs in the way we handle a commit cannot be picked because it would overwrite an untracked file.

 * after a failed pick "git rebase --continue" will happily commit any
   staged changes even though no commit was picked.

 * the commit of the failed pick is recorded as rewritten even though no
   commit was picked.

 * the "done" file used by "git status" to show the recently executed
   commands contains an incorrect entry.

Thanks to Eric, Glen and Junio for their comments on v2. Here are the changes since v2:

Patch 1 - Reworded the commit message.

Patch 2 - Reworded the commit message, added a test and fixed error message pointed out by Glen.

Patch 3 - New cleanup.

Patch 4 - Reworded the commit message, now only increments todo_list->current if there is no error.

Patch 5 - Swapped with next patch. Reworded the commit message, stopped testing implementation (suggested by Glen). Expanded post-rewrite hook test.

Patch 6 - Reworded the commit message, now uses the message file rather than the author script to check if "rebase --continue" should commit staged changes. Junio suggested using a separate file for this but I think that would end up being more involved as we'd need to be careful about creating and removing it.

Patch 7 - Reworded the commit message.

Thanks for the comments on V1, this series has now grown somewhat. Previously I was worried that refactoring would change the behavior, but having thought about it the current behavior is wrong and should be changed.

Changes since V1:

Rebased onto master to avoid a conflict with ab/remove-implicit-use-of-the-repository

 * Patches 1-3 are new preparatory changes
 * Patches 4 & 5 are new and fix the first two issues listed above.
 * Patch 6 is the old patch 1 which has been rebased and the commit message
   reworded. It fixes the last issues listed above.

Phillip Wood (7):
  rebase -i: move unlink() calls
  rebase -i: remove patch file after conflict resolution
  sequencer: use rebase_path_message()
  sequencer: factor out part of pick_commits()
  rebase: fix rewritten list for failed pick
  rebase --continue: refuse to commit after failed command
  rebase -i: fix adding failed command to the todo list

 sequencer.c                   | 179 ++++++++++++++++++----------------
 t/t3404-rebase-interactive.sh |  53 +++++++---
 t/t3418-rebase-continue.sh    |  18 ++++
 t/t3430-rebase-merges.sh      |  30 ++++--
 t/t5407-post-rewrite-hook.sh  |  48 +++++++++
 5 files changed, 225 insertions(+), 103 deletions(-)

base-commit: a80be152923a46f04a06bade7bcc72870e46ca09
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1492

Range-diff vs v2:

 1:  3dfb2c6903b ! 1:  1ab1ad2ef07 rebase -i: move unlink() calls
     @@ Commit message
      
          At the start of each iteration the loop that picks commits removes
          state files from the previous pick. However some of these are only
     -    written if there are conflicts so only need to be removed before
     -    starting the loop, not in each iteration.
     +    written if there are conflicts and so we break out of the loop after
     +    writing them. Therefore they only need to be removed when the rebase
     +    continues, not in each iteration.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
 2:  227aea031b5 ! 2:  e2a758eb4a5 rebase -i: remove patch file after conflict resolution
     @@ Metadata
       ## Commit message ##
          rebase -i: remove patch file after conflict resolution
      
     -    When rebase stops for the user to resolve conflicts it writes a patch
     -    for the conflicting commit to .git/rebase-merge/patch. This file
     -    should be deleted when the rebase continues. As the path is now used
     -    in two different places rebase_path_patch() is added and used to
     -    obtain the path for the patch.
     +    When a rebase stops for the user to resolve conflicts it writes a patch
     +    for the conflicting commit to .git/rebase-merge/patch. This file has
     +    been written since the introduction of "git-rebase-interactive.sh" in
     +    1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the
     +    idea was to enable the user inspect the conflicting commit in the same
     +    way as they could for the patch based rebase. This file should be
     +    deleted when the rebase continues as if the rebase stops for a failed
     +    "exec" command or a "break" command it is confusing to the user if there
     +    is a stale patch lying around from an unrelated command. As the path is
     +    now used in two different places rebase_path_patch() is added and used
     +    to obtain the path for the patch.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ sequencer.c: static int make_patch(struct repository *r,
      +	log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w");
       	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
       	if (!log_tree_opt.diffopt.file)
     - 		res |= error_errno(_("could not open '%s'"), buf.buf);
     +-		res |= error_errno(_("could not open '%s'"), buf.buf);
     ++		res |= error_errno(_("could not open '%s'"),
     ++				   rebase_path_patch());
     + 	else {
     + 		res |= log_tree_commit(&log_tree_opt, commit);
     + 		fclose(log_tree_opt.diffopt.file);
     + 	}
     +-	strbuf_reset(&buf);
     + 
     + 	strbuf_addf(&buf, "%s/message", get_dir(opts));
     + 	if (!file_exists(buf.buf)) {
      @@ sequencer.c: static int pick_commits(struct repository *r,
       	unlink(rebase_path_message());
       	unlink(rebase_path_stopped_sha());
     @@ sequencer.c: static int pick_commits(struct repository *r,
       
       	while (todo_list->current < todo_list->nr) {
       		struct todo_item *item = todo_list->items + todo_list->current;
     +
     + ## t/t3418-rebase-continue.sh ##
     +@@ t/t3418-rebase-continue.sh: test_expect_success 'the todo command "break" works' '
     + 	test_path_is_file execed
     + '
     + 
     ++test_expect_success 'patch file is removed before break command' '
     ++	test_when_finished "git rebase --abort" &&
     ++	cat >todo <<-\EOF &&
     ++	pick commit-new-file-F2-on-topic-branch
     ++	break
     ++	EOF
     ++
     ++	(
     ++		set_replace_editor todo &&
     ++		test_must_fail git rebase -i --onto commit-new-file-F2 HEAD
     ++	) &&
     ++	test_path_is_file .git/rebase-merge/patch &&
     ++	echo 22>F2 &&
     ++	git add F2 &&
     ++	git rebase --continue &&
     ++	test_path_is_missing .git/rebase-merge/patch
     ++'
     ++
     + test_expect_success '--reschedule-failed-exec' '
     + 	test_when_finished "git rebase --abort" &&
     + 	test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
 -:  ----------- > 3:  8f6c0e40567 sequencer: use rebase_path_message()
 3:  31bb644e769 ! 4:  a1fad70f4b9 sequencer: factor out part of pick_commits()
     @@ Metadata
       ## Commit message ##
          sequencer: factor out part of pick_commits()
      
     -    This is simplifies a change in a later commit. If a pick fails we now
     -    return the error at then end of the loop body rather than returning
     -    early but there is no change in behavior.
     +    This simplifies the next commit. If a pick fails we now return the error
     +    at the end of the loop body rather than returning early, a successful
     +    "edit" command continues to return early. There are three things to
     +    check to ensure that removing the early return for an error does not
     +    change the behavior of the code:
     +
     +    (1) We could enter the block guarded by "if (reschedule)". This block
     +        is not entered because "reschedlue" is always zero when picking a
     +        commit.
     +
     +    (2) We could enter the block guarded by
     +        "else if (is_rebase_i(opts) &&  check_todo && !res)". This block is
     +        not entered when returning an error because "res" is non-zero in
     +        that case.
     +
     +    (3) todo_list->current could be incremented before returning. That is
     +        avoided by moving the increment which is of course a potential
     +        change in behavior itself. The move is safe because none of the
     +        callers look at todo_list after this function returns. Moving the
     +        increment makes it clear we only want to advance the current item
     +        if the command was successful.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ sequencer.c: static int pick_commits(struct repository *r,
       		} else if (item->command == TODO_EXEC) {
       			char *end_of_arg = (char *)(arg + item->arg_len);
       			int saved = *end_of_arg;
     +@@ sequencer.c: static int pick_commits(struct repository *r,
     + 			return -1;
     + 		}
     + 
     +-		todo_list->current++;
     + 		if (res)
     + 			return res;
     ++
     ++		todo_list->current++;
     + 	}
     + 
     + 	if (is_rebase_i(opts)) {
 5:  f8e64c1b631 ! 5:  df401945866 rebase: fix rewritten list for failed pick
     @@ Metadata
       ## Commit message ##
          rebase: fix rewritten list for failed pick
      
     -    When rebasing commands are moved from the todo list in "git-rebase-todo"
     -    to the "done" file just before they are executed. This means that if a
     -    command fails because it would overwrite an untracked file it has to be
     -    added back into the todo list before the rebase stops for the user to
     -    fix the problem. Unfortunately the way this is done results in the
     -    failed pick being recorded as rewritten.
     +    git rebase keeps a list that maps the OID of each commit before it was
     +    rebased to the OID of the equivalent commit after the rebase.  This list
     +    is used to drive the "post-rewrite" hook that is called at the end of a
     +    successful rebase. When a rebase stops for the user to resolve merge
     +    conflicts the OID of the commit being picked is written to
     +    ".git/rebase-merge/stopped-sha". Then when the rebase is continued that
     +    OID is added to the list of rewritten commits. Unfortunately if a commit
     +    cannot be picked because it would overwrite an untracked file we still
     +    write the "stopped-sha1" file. This means that when the rebase is
     +    continued the commit is added into the list of rewritten commits even
     +    though it has not been picked yet.
      
          Fix this by not calling error_with_patch() for failed commands. The pick
          has failed so there is nothing to commit and therefore we do not want to
     -    set up the message file for committing staged changes when the rebase
     +    set up the state files for committing staged changes when the rebase
          continues. This change means we no-longer write a patch for the failed
          command or display the error message printed by error_with_patch(). As
     -    the command has failed the patch isn't really useful in that case and
     -    REBASE_HEAD is still written so the user can inspect the commit
     -    associated with the failed command. Unless the user has disabled it we
     -    print an advice message that is more helpful than the message from
     -    error_with_patch(). If the advice is disabled the user will still see
     -    the messages from the merge machinery detailing the problem.
     +    the command has failed the patch isn't really useful and in any case the
     +    user can inspect the commit associated with the failed command by
     +    inspecting REBASE_HEAD. Unless the user has disabled it we already print
     +    an advice message that is more helpful than the message from
     +    error_with_patch() which the user will still see. Even if the advice is
     +    disabled the user will see the messages from the merge machinery
     +    detailing the problem.
      
          To simplify writing REBASE_HEAD in this case pick_one_commit() is
     -    modified to avoid duplicating the code that adds the failed command back
     -    into the todo list.
     +    modified to avoid duplicating the code that adds the failed command
     +    back into the todo list.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## sequencer.c ##
      @@ sequencer.c: static int do_merge(struct repository *r,
     + 	if (ret < 0) {
       		error(_("could not even attempt to merge '%.*s'"),
       		      merge_arg_len, arg);
     - 		unlink(rebase_path_author_script());
      +		unlink(git_path_merge_msg(r));
       		goto leave_merge;
       	}
     @@ sequencer.c: static int pick_commits(struct repository *r,
      
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
     + 	>file6 &&
     + 	test_must_fail git rebase --continue &&
       	test_cmp_rev HEAD F &&
     ++	test_cmp_rev REBASE_HEAD I &&
       	rm file6 &&
     - 	test_path_is_missing .git/rebase-merge/author-script &&
      +	test_path_is_missing .git/rebase-merge/patch &&
     -+	test_path_is_missing .git/MERGE_MSG &&
     -+	test_path_is_missing .git/rebase-merge/message &&
     -+	test_path_is_missing .git/rebase-merge/stopped-sha &&
     - 	echo changed >file1 &&
     - 	git add file1 &&
     - 	test_must_fail git rebase --continue 2>err &&
     + 	git rebase --continue &&
     + 	test_cmp_rev HEAD I
     + '
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
     + 	>file6 &&
     + 	test_must_fail git rebase --continue &&
       	test_cmp_rev HEAD F &&
     ++	test_cmp_rev REBASE_HEAD I &&
       	rm file6 &&
     - 	test_path_is_missing .git/rebase-merge/author-script &&
      +	test_path_is_missing .git/rebase-merge/patch &&
     -+	test_path_is_missing .git/MERGE_MSG &&
     -+	test_path_is_missing .git/rebase-merge/message &&
     -+	test_path_is_missing .git/rebase-merge/stopped-sha &&
       	git rebase --continue &&
       	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
       	git reset --hard original-branch2
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
     + 	>file6 &&
     + 	test_must_fail git rebase --continue &&
       	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
     ++	test_cmp_rev REBASE_HEAD I &&
       	rm file6 &&
     - 	test_path_is_missing .git/rebase-merge/author-script &&
      +	test_path_is_missing .git/rebase-merge/patch &&
     -+	test_path_is_missing .git/MERGE_MSG &&
     -+	test_path_is_missing .git/rebase-merge/message &&
     -+	test_path_is_missing .git/rebase-merge/stopped-sha &&
       	git rebase --continue &&
       	test $(git cat-file commit HEAD | sed -ne \$p) = I
       '
      
       ## t/t3430-rebase-merges.sh ##
      @@ t/t3430-rebase-merges.sh: test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
     + 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
     + 	test_tick &&
       	test_must_fail git rebase -ir HEAD &&
     ++	test_cmp_rev REBASE_HEAD H^0 &&
       	grep "^merge -C .* G$" .git/rebase-merge/done &&
       	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
      -	test_path_is_file .git/rebase-merge/patch &&
      +	test_path_is_missing .git/rebase-merge/patch &&
     - 	test_path_is_missing .git/rebase-merge/author-script &&
     -+	test_path_is_missing .git/MERGE_MSG &&
     -+	test_path_is_missing .git/rebase-merge/message &&
     -+	test_path_is_missing .git/rebase-merge/stopped-sha &&
       
       	: fail because of merge conflict &&
      -	rm G.t .git/rebase-merge/patch &&
       	git reset --hard conflicting-G &&
       	test_must_fail git rebase --continue &&
       	! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
     - 	test_path_is_file .git/rebase-merge/patch &&
     --	test_path_is_file .git/rebase-merge/author-script
     -+	test_path_is_file .git/rebase-merge/author-script &&
     -+	test_path_is_file .git/MERGE_MSG &&
     -+	test_path_is_file .git/rebase-merge/message &&
     -+	test_path_is_file .git/rebase-merge/stopped-sha
     - '
     - 
     - test_expect_success 'failed `merge <branch>` does not crash' '
      
       ## t/t5407-post-rewrite-hook.sh ##
     +@@ t/t5407-post-rewrite-hook.sh: test_expect_success 'setup' '
     + 	git checkout A^0 &&
     + 	test_commit E bar E &&
     + 	test_commit F foo F &&
     ++	git checkout B &&
     ++	git merge E &&
     ++	git tag merge-E &&
     ++	test_commit G G &&
     ++	test_commit H H &&
     ++	test_commit I I &&
     + 	git checkout main &&
     + 
     + 	test_hook --setup post-rewrite <<-EOF
      @@ t/t5407-post-rewrite-hook.sh: test_fail_interactive_rebase () {
       	)
       }
       
      +test_expect_success 'git rebase with failed pick' '
     -+	test_fail_interactive_rebase "exec_>bar pick 1" --onto C A E &&
     ++	clear_hook_input &&
     ++	cat >todo <<-\EOF &&
     ++	exec >bar
     ++	merge -C merge-E E
     ++	exec >G
     ++	pick G
     ++	exec >H 2>I
     ++	pick H
     ++	fixup I
     ++	EOF
     ++
     ++	(
     ++		set_replace_editor todo &&
     ++		test_must_fail git rebase -i D D 2>err
     ++	) &&
     ++	grep "would be overwritten" err &&
      +	rm bar &&
     ++
     ++	test_must_fail git rebase --continue 2>err &&
     ++	grep "would be overwritten" err &&
     ++	rm G &&
     ++
     ++	test_must_fail git rebase --continue 2>err &&
     ++	grep "would be overwritten" err &&
     ++	rm H &&
     ++
     ++	test_must_fail git rebase --continue 2>err &&
     ++	grep "would be overwritten" err &&
     ++	rm I &&
     ++
      +	git rebase --continue &&
      +	echo rebase >expected.args &&
      +	cat >expected.data <<-EOF &&
     -+	$(git rev-parse E) $(git rev-parse HEAD)
     ++	$(git rev-parse merge-E) $(git rev-parse HEAD~2)
     ++	$(git rev-parse G) $(git rev-parse HEAD~1)
     ++	$(git rev-parse H) $(git rev-parse HEAD)
     ++	$(git rev-parse I) $(git rev-parse HEAD)
      +	EOF
      +	verify_hook_input
      +'
 4:  9356d14b09a ! 6:  2ed7cbe5fff rebase --continue: refuse to commit after failed command
     @@ Commit message
      
          If a commit cannot be picked because it would overwrite an untracked
          file then "git rebase --continue" should refuse to commit any staged
     -    changes as the commit was not picked. Do this by using the existing
     -    check for a missing author script in run_git_commit() which prevents
     -    "rebase --continue" from committing staged changes after failed exec
     -    commands.
     +    changes as the commit was not picked. This is implemented by refusing to
     +    commit if the message file is missing. The message file is chosen for
     +    this check because it is only written when "git rebase" stops for the
     +    user to resolve merge conflicts.
      
     -    When fast-forwarding it is not necessary to write the author script as
     -    we're reusing an existing commit, not creating a new one. If a
     -    fast-forwarded commit is modified by an "edit" or "reword" command then
     -    the modification is committed with "git commit --amend" which reuses the
     -    author of the commit being amended so the author script is not needed.
     -    baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding,
     -    2021-08-20) changed run_git_commit() to allow a missing author script
     -    when rewording a commit. This changes extends that to allow a missing
     -    author script whenever the commit is being amended.
     +    Existing commands that refuse to commit staged changes when continuing
     +    such as a failed "exec" rely on checking for the absence of the author
     +    script in run_git_commit(). This prevents the staged changes from being
     +    committed but prints
      
     -    If we're not fast-forwarding then we must remove the author script if
     -    the pick fails.
     +        error: could not open '.git/rebase-merge/author-script' for
     +        reading
     +
     +    before the message about not being able to commit. This is confusing to
     +    users and so checking for the message file instead improves the user
     +    experience. The existing test for refusing to commit after a failed exec
     +    is updated to check that we do not print the error message about a
     +    missing author script anymore.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## sequencer.c ##
     -@@ sequencer.c: static int run_git_commit(const char *defmsg,
     +@@ sequencer.c: static int commit_staged_changes(struct repository *r,
       
     - 	if (is_rebase_i(opts) &&
     - 	    ((opts->committer_date_is_author_date && !opts->ignore_date) ||
     --	     !(!defmsg && (flags & AMEND_MSG))) &&
     -+	     !(flags & AMEND_MSG)) &&
     - 	    read_env_script(&cmd.env)) {
     - 		const char *gpg_opt = gpg_sign_opt_quoted(opts);
     + 	is_clean = !has_uncommitted_changes(r, 0);
       
     -@@ sequencer.c: static int do_pick_commit(struct repository *r,
     - 	if (opts->allow_ff && !is_fixup(command) &&
     - 	    ((parent && oideq(&parent->object.oid, &head)) ||
     - 	     (!parent && unborn))) {
     --		if (is_rebase_i(opts))
     --			write_author_script(msg.message);
     - 		res = fast_forward_to(r, &commit->object.oid, &head, unborn,
     - 			opts);
     - 		if (res || command != TODO_REWORD)
     -@@ sequencer.c: static int do_pick_commit(struct repository *r,
     - 		 command == TODO_REVERT) {
     - 		res = do_recursive_merge(r, base, next, base_label, next_label,
     - 					 &head, &msgbuf, opts);
     --		if (res < 0)
     -+		if (res < 0) {
     -+			unlink(rebase_path_author_script());
     - 			goto leave;
     --
     -+		}
     - 		res |= write_message(msgbuf.buf, msgbuf.len,
     - 				     git_path_merge_msg(r), 0);
     - 	} else {
     -@@ sequencer.c: static int do_merge(struct repository *r,
     - 	if (ret < 0) {
     - 		error(_("could not even attempt to merge '%.*s'"),
     - 		      merge_arg_len, arg);
     -+		unlink(rebase_path_author_script());
     - 		goto leave_merge;
     - 	}
     - 	/*
     ++	if (!is_clean && !file_exists(rebase_path_message())) {
     ++		const char *gpg_opt = gpg_sign_opt_quoted(opts);
     ++
     ++		return error(_(staged_changes_advice), gpg_opt, gpg_opt);
     ++	}
     + 	if (file_exists(rebase_path_amend())) {
     + 		struct strbuf rev = STRBUF_INIT;
     + 		struct object_id head, to_amend;
      
       ## t/t3404-rebase-interactive.sh ##
     +@@ t/t3404-rebase-interactive.sh: test_expect_success 'clean error after failed "exec"' '
     + 	echo "edited again" > file7 &&
     + 	git add file7 &&
     + 	test_must_fail git rebase --continue 2>error &&
     +-	test_i18ngrep "you have staged changes in your working tree" error
     ++	test_i18ngrep "you have staged changes in your working tree" error &&
     ++	test_i18ngrep ! "could not open.*for reading" error
     + '
     + 
     + test_expect_success 'rebase a detached HEAD' '
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
     - 	test_must_fail git rebase --continue &&
     - 	test_cmp_rev HEAD F &&
     + 	test_cmp_rev REBASE_HEAD I &&
       	rm file6 &&
     -+	test_path_is_missing .git/rebase-merge/author-script &&
     + 	test_path_is_missing .git/rebase-merge/patch &&
      +	echo changed >file1 &&
      +	git add file1 &&
      +	test_must_fail git rebase --continue 2>err &&
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overw
       	test_cmp_rev HEAD I
       '
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
     - 	test_must_fail git rebase --continue &&
     - 	test_cmp_rev HEAD F &&
     + 	test_cmp_rev REBASE_HEAD I &&
       	rm file6 &&
     -+	test_path_is_missing .git/rebase-merge/author-script &&
     + 	test_path_is_missing .git/rebase-merge/patch &&
     ++	echo changed >file1 &&
     ++	git add file1 &&
     ++	test_must_fail git rebase --continue 2>err &&
     ++	grep "error: you have staged changes in your working tree" err &&
     ++	git reset --hard HEAD &&
       	git rebase --continue &&
       	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
       	git reset --hard original-branch2
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
     - 	test_must_fail git rebase --continue &&
     - 	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
     + 	test_cmp_rev REBASE_HEAD I &&
       	rm file6 &&
     -+	test_path_is_missing .git/rebase-merge/author-script &&
     + 	test_path_is_missing .git/rebase-merge/patch &&
     ++	echo changed >file1 &&
     ++	git add file1 &&
     ++	test_must_fail git rebase --continue 2>err &&
     ++	grep "error: you have staged changes in your working tree" err &&
     ++	git reset --hard HEAD &&
       	git rebase --continue &&
       	test $(git cat-file commit HEAD | sed -ne \$p) = I
       '
     @@ t/t3430-rebase-merges.sh
      @@ t/t3430-rebase-merges.sh: test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
       	grep "^merge -C .* G$" .git/rebase-merge/done &&
       	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
     - 	test_path_is_file .git/rebase-merge/patch &&
     -+	test_path_is_missing .git/rebase-merge/author-script &&
     + 	test_path_is_missing .git/rebase-merge/patch &&
     ++	echo changed >file1 &&
     ++	git add file1 &&
     ++	test_must_fail git rebase --continue 2>err &&
     ++	grep "error: you have staged changes in your working tree" err &&
       
       	: fail because of merge conflict &&
     - 	rm G.t .git/rebase-merge/patch &&
       	git reset --hard conflicting-G &&
     - 	test_must_fail git rebase --continue &&
     - 	! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
     --	test_path_is_file .git/rebase-merge/patch
     -+	test_path_is_file .git/rebase-merge/patch &&
     -+	test_path_is_file .git/rebase-merge/author-script
     - '
     - 
     - test_expect_success 'failed `merge <branch>` does not crash' '
 6:  a836b049b90 ! 7:  bbe0afde512 rebase -i: fix adding failed command to the todo list
     @@ Commit message
          added back into the todo list before the rebase stops for the user to
          fix the problem.
      
     -    Unfortunately when a failed command is added back into the todo list
     -    the command preceding it is erroneously appended to the "done" file.
     -    This means that when rebase stops after "pick B" fails the "done"
     -    file contains
     +    Unfortunately when a failed command is added back into the todo list the
     +    command preceding it is erroneously appended to the "done" file.  This
     +    means that when rebase stops after "pick B" fails the "done" file
     +    contains
      
                  pick A
                  pick B
     @@ Commit message
                  pick A
                  pick B
      
     -    Fix this by not updating the "done" file when adding a failed command
     +    This happens because save_todo() updates the "done" file with the
     +    previous command whenever "git-rebase-todo" is updated. When we add the
     +    failed pick back into "git-rebase-todo" we do not want to update
     +    "done". Fix this by adding a "reschedule" parameter to save_todo() which
     +    prevents the "done" file from being updated when adding a failed command
          back into the "git-rebase-todo" file. A couple of the existing tests are
          modified to improve their coverage as none of them trigger this bug or
          check the "done" file.
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' '
      -	>file6 &&
      -	test_must_fail git rebase --continue &&
      -	test_cmp_rev HEAD F &&
     +-	test_cmp_rev REBASE_HEAD I &&
      -	rm file6 &&
      +	test_cmp_rev HEAD B &&
     ++	test_cmp_rev REBASE_HEAD C &&
      +	head -n3 todo >expect &&
      +	test_cmp expect .git/rebase-merge/done &&
      +	rm file2 &&
     - 	test_path_is_missing .git/rebase-merge/author-script &&
       	test_path_is_missing .git/rebase-merge/patch &&
     - 	test_path_is_missing .git/MERGE_MSG &&
     + 	echo changed >file1 &&
     + 	git add file1 &&
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
       	grep "error: you have staged changes in your working tree" err &&
       	git reset --hard HEAD &&

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When a rebase stops for the user to resolve conflicts it writes a patch for the conflicting commit to .git/rebase-merge/patch. This file has been written since the introduction of "git-rebase-interactive.sh" in 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the idea was to enable the user inspect the conflicting commit in the same way as they could for the patch based rebase. This file should be deleted when the rebase continues as if the rebase stops for a failed "exec" command or a "break" command it is confusing to the user if there is a stale patch lying around from an unrelated command. As the path is now used in two different places rebase_path_patch() is added and used to obtain the path for the patch.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                | 13 +++++++++----
 t/t3418-rebase-continue.sh | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Unlike the previous step, this describes the change in end-user observable behaviour *and* asserts that it is an intended change.

Very good.

The get_dir() function gives different paths, between "rebase-merge" (for "rebase -i") and "sequencer" (for everything else), and that is the parent directory of "/patch" output make_patch() uses.

error_with_patch() is the only caller of make_patch(), and error_with_patch() is called by

 error_failed_squash() - called from pick_commits()
 pick_commits() - when TODO_EDIT stops the sequence, or
		  a non fix-up insn failed when is_rebase_i(), or
		  a merge insn failed, or
		  a reschedule happened.

Are we sure that it is the right thing to do to hardcode "rebase-merge/patch"? Unless "rebase -i" is the only thing that calls pick_commits() and reaches error_with_patch() to cause make_patch() to be called, this changes the behaviour for cases the tests added by this patch do not cover, doesn't it?

I would feel safer if this change is accompanied by something like

On 01/08/2023 18:23, Junio C Hamano wrote:

There is no intended change in the observable behavior in the previous patch.

Good point - the patch file is only ever created by "rebase -i". I'll add the assertion you suggest below.

Best Wishes

Phillip

 >

From: Phillip Wood <phillip.wood@dunelm.org.uk>

At the start of each iteration the loop that picks commits removes state files from the previous pick. However some of these are only written if there are conflicts and so we break out of the loop after writing them. Therefore they only need to be removed when the rebase continues, not in each iteration.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

"They only need to be removed before the loop" assumes that the SOLE purpose of the removal is to give the next iteration a clean slate to work with, but is that really the case? The original unlink's is followed by "if TODO_BREAK, break out of the loop", presumably to give control back to the end-user. So three files that were not available to the user after "break" are now suddenly visible to them.

Perhaps that is the effect the series wanted to have. Or it could be an unintended side effect that may be a regression. Or perhaps the visibility of these three files (but not others?) is considered an implementation detail no users should ever depend on.

It is hard to tell from the above description and the patch text which is the case. Care to enlighten?

Thanks.

On 01/08/2023 18:22, Junio C Hamano wrote:

The files will never exist when the "if TODO_BREAK" is executed because we've removed them before entering the loop and as I tried and seemly failed to explain in the commit message they are only created when we're about to break out of the loop.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Specifically, they are not created when we voluntarily leave the loop via TODO_BREAK. They are created when we leave the loop via the other exit paths (e.g. path_message may be created from MERGE_MSG in error_with_patch() but the control flow to reach error_with_patch() in the loop would break out of the loop without ever reaching the TODO_BREAK codepath).

Or something like that?  I didn't follow thru the other two files.

OK. I am slow to read and understand a patch from more than 3 months ago X-<; sorry for the confusion.

Thanks.

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Rather than constructing the path in a struct strbuf use the ready made function to get the path name instead. This was the last remaining use of the strbuf so remove it as well.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

The same comment about "get_dir() vs hardcoded rebase-merge" applies here, I think. And the same (1) assertion to ensure that we are in "rebase -i" when make_patch() is called should give us a sufficient safety valve, or (2) instead of hardcoding rebase_path_message(), call it sequencer_path_message() and switch at runtime behaving the same way as get_dir(opts) based version, would also work.

Thanks.

On 01/08/2023 18:23, Junio C Hamano wrote:

Agreed

I think that would me misleading because cherry-pick/revert do not create that file - they rely on "git commit" reading .git/MERGE_MSG

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Fair enough.  

Abusing the MERGE_MSG this way probably came from the "if 'commit' picks up whatever is left inside MERGE_MSG when it is run, reusing it for this operation (even though it clearly is not a 'merge') would be a way to do with the least effort, even if it does not make sense for those who will be reading the code 3 years from now on" kind of hackery. The file probably outgrew its name and we might want to rename it to a more appropriate name (it is "we gave control back to the user to help us resolve a mess in the working tree, and here is the message to be used when the user is done"; the "mess" no longer is limited to conflicts created during a "merge").

But it would be a major headache if end-user tools are relying on it, so it is not likely to happen anytime soon.

So, moving to hardcoded "rebase-merge", as long as we make sure make_patch() will only callable by "rebase -i" (and not something like "cherry-pick -i" people will wish to add in the future), I am fine with such a design.

Thanks.

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This simplifies the next commit. If a pick fails we now return the error at the end of the loop body rather than returning early, a successful "edit" command continues to return early. There are three things to check to ensure that removing the early return for an error does not change the behavior of the code:

(1) We could enter the block guarded by "if (reschedule)". This block
    is not entered because "reschedlue" is always zero when picking a
    commit.

(2) We could enter the block guarded by
    "else if (is_rebase_i(opts) &&  check_todo && !res)". This block is
    not entered when returning an error because "res" is non-zero in
    that case.

(3) todo_list->current could be incremented before returning. That is
    avoided by moving the increment which is of course a potential
    change in behavior itself. The move is safe because none of the
    callers look at todo_list after this function returns. Moving the
    increment makes it clear we only want to advance the current item
    if the command was successful.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 132 ++++++++++++++++++++++++++++------------------------
 1 file changed, 71 insertions(+), 61 deletions(-)

Hi Phillip,

On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:

Well argued, and it matches exactly what the patch says.

FWIW I had to look at the patch locally, using

$ git show --color-moved --color-moved-ws=allow-indentation-change \
	a1fad70f4b920252d84b5b5578b1b9b0a7bba7ca

to convince myself that the code was extracted into `pick_one_commit()` as verbatim as possible. There are a couple of (benign) differences: a `&` was removed since `check_todo` is now a pointer, and some wrapping differences that leave the logic unchanged.

So here is my ACK.

Thank you, Johannes

From: Phillip Wood <phillip.wood@dunelm.org.uk>

git rebase keeps a list that maps the OID of each commit before it was rebased to the OID of the equivalent commit after the rebase. This list is used to drive the "post-rewrite" hook that is called at the end of a successful rebase. When a rebase stops for the user to resolve merge conflicts the OID of the commit being picked is written to ".git/rebase-merge/stopped-sha". Then when the rebase is continued that OID is added to the list of rewritten commits. Unfortunately if a commit cannot be picked because it would overwrite an untracked file we still write the "stopped-sha1" file. This means that when the rebase is continued the commit is added into the list of rewritten commits even though it has not been picked yet.

Fix this by not calling error_with_patch() for failed commands. The pick has failed so there is nothing to commit and therefore we do not want to set up the state files for committing staged changes when the rebase continues. This change means we no-longer write a patch for the failed command or display the error message printed by error_with_patch(). As the command has failed the patch isn't really useful and in any case the user can inspect the commit associated with the failed command by inspecting REBASE_HEAD. Unless the user has disabled it we already print an advice message that is more helpful than the message from error_with_patch() which the user will still see. Even if the advice is disabled the user will see the messages from the merge machinery detailing the problem.

To simplify writing REBASE_HEAD in this case pick_one_commit() is modified to avoid duplicating the code that adds the failed command back into the todo list.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 19 +++++---------
 t/t3404-rebase-interactive.sh |  6 +++++
 t/t3430-rebase-merges.sh      |  4 +--
 t/t5407-post-rewrite-hook.sh  | 48 +++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 14 deletions(-)

Hi Phillip,

On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:

This motivates the change well, and answered all but one of the questions I had about it, being:

Why is it okay to remove this decrement?

Here is why: The code that calls `save_todo()` in the `if (reschedule)` block of the loop of `pick_commits()` _duplicates_ the logic that is removed here, including the advice and the decrementing of `current`.

I had to instrument the code before and after this patch to figure this out, as I had missed the fact that the now-remaining code also decremented the `current` attribute.

So: All is good with this patch. If you'd like to amend the commit message
accordingly, I would not be opposed, but I could now live equally as
easily without it.

I'd like to point out how delighted I am about this careful test case:

Here is my ACK.

Thank you, Johannes

Hi Dscho

On 23/08/2023 09:55, Johannes Schindelin wrote:

I'll try and add something to the commit message when I re-roll

Thanks

Phillip

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a commit cannot be picked because it would overwrite an untracked file then "git rebase --continue" should refuse to commit any staged changes as the commit was not picked. This is implemented by refusing to commit if the message file is missing. The message file is chosen for this check because it is only written when "git rebase" stops for the user to resolve merge conflicts.

Existing commands that refuse to commit staged changes when continuing such as a failed "exec" rely on checking for the absence of the author script in run_git_commit(). This prevents the staged changes from being committed but prints

    error: could not open '.git/rebase-merge/author-script' for
    reading

before the message about not being able to commit. This is confusing to users and so checking for the message file instead improves the user experience. The existing test for refusing to commit after a failed exec is updated to check that we do not print the error message about a missing author script anymore.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   |  5 +++++
 t/t3404-rebase-interactive.sh | 18 +++++++++++++++++-
 t/t3430-rebase-merges.sh      |  4 ++++
 3 files changed, 26 insertions(+), 1 deletion(-)

Hi Phillip,

On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:

I am delighted to see an improvement of the user experience!

However, I could imagine that users would still be confused when seeing the advice about staged changes, even if nothing was staged at all.

Could you introduce a new advice message specifically for the case where untracked files are in the way and prevent changes from being applied?

Thank you, Johannes

P.S.: To save both you and me time, here is my ACK for patch 7/7 (actually, the entire patch series, but _maybe_ you want to change "impove" -> "improve" in the cover letter's subject) ;-)

Hi Dscho

On 23/08/2023 10:01, Johannes Schindelin wrote:

If nothing is staged then this message wont trigger because is_clean will be false.

We have an advice message now that is printed when the rebase stops in that case. The message here is printed when the user runs "rebase --continue" with staged changes and we're not expecting to commit anything because the commit couldn't be picked or we're containing from a break command or bad exec/label/reset etc.

Thanks for taking the time to read through the patches and for you comments. I'll fix the typo when I re-roll

Best Wishes

Phillip

Hi Phillip,

On Mon, 4 Sep 2023, Phillip Wood wrote:

Ah. I managed to get confused by the first sentence of the commit message already. You clearly talk about "any staged changes". As in "*iff* there are any staged changes". Which I missed.

A further contributing factor for my misunderstading was the slightly convoluted logic where `is_clean` is set to true if there are _not_ any uncommitted changes, and then we ask if `is_clean` is _not_ true. Reminds me of Smullyan's Knights & Knaves [*1*].

With your patch, there are now four users of the `is_clean` value, and all but one of them ask for the negated value.

It's not really the responsibility of this patch series, but I could imagine that it would be nicer to future readers if a patch was added that would invert the meaning of that variable and rename it to `needs_committing`. At least to me, that would make the intention of the code eminently clearer.

Ciao, Johannes

Footnote *1*: https://en.wikipedia.org/wiki/Knights_and_Knaves

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Excellent observation. That strongly argues for the flipping of polarity, i.e. many people want to know "is it unclean/dirty?". It is funny that the name of the helper function where the value comes from, i.e. has_uncommitted_changes(), has the desired polarity.

While I agree, after reading the code, that it would make it easier to follow to flip the polarity of the variable, I would advise against renaming from state based naming (is it dirty?) to action based naming (must we commit?), *if* the variable is checked to sometimes see if we has something that we _could_ commit, while some other times to see if we _must_ commit before we can let the user proceed.

"Does the index hold some changes to be committed?" is better question than "Must we commit?" or "Could we commit?" to derive the name of this variable from if that is the case, I would think.

Hi Johannes

On 05/09/2023 12:17, Johannes Schindelin wrote:

I agree 'is_clean' is confusing (I have the same problem with merge_recursive() and friends where a return value of zero means that there were conflicts)

Inverting and renaming 'is_clean' is a good idea, I might leave it to a follow up series though.

Best Wishes

Phillip

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When rebasing commands are moved from the todo list in "git-rebase-todo" to the "done" file (which is used by "git status" to show the recently executed commands) just before they are executed. This means that if a command fails because it would overwrite an untracked file it has to be added back into the todo list before the rebase stops for the user to fix the problem.

Unfortunately when a failed command is added back into the todo list the command preceding it is erroneously appended to the "done" file. This means that when rebase stops after "pick B" fails the "done" file contains

	pick A
	pick B
	pick A

instead of

	pick A
	pick B

This happens because save_todo() updates the "done" file with the previous command whenever "git-rebase-todo" is updated. When we add the failed pick back into "git-rebase-todo" we do not want to update "done". Fix this by adding a "reschedule" parameter to save_todo() which prevents the "done" file from being updated when adding a failed command back into the "git-rebase-todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file.

Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 12 ++++++------
 t/t3404-rebase-interactive.sh | 29 ++++++++++++++++++-----------
 t/t3430-rebase-merges.sh      | 22 ++++++++++++++++------
 3 files changed, 40 insertions(+), 23 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

I'd really prefer to see the latter half of the series reviewed (both for the design and its implementation) by those who have more stake in the sequencer code than myself.

I just noticed that we have a question on the last step left unanswered since the very initial iteration.

cf. https://lore.kernel.org/git/f05deb00-1bcd-9e05-739f-6a30d6d8cf3b@gmx.de/

Thanks.

On 02/08/2023 23:10, Junio C Hamano wrote:

That would certainly be nice.

Thanks, I'd forgotten about that.

Best Wishes

Phillip

On 02/08/2023 23:10, Junio C Hamano wrote:

Dscho do you think you will have time to look at the last four patches after 2.42.0 is released?

Many Thanks

Phillip

Hi Phillip!

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Thanks for sending this version, and apologies for not getting to it sooner (I tried a few times, but it was hard to reconstruct the context around something as complicated as sequencer.c..). Unfortunately, I don't think I will be able to chime in on subsequent rounds.

I found the updated commit messages much easier to understand, and the change to no longer test implementation is also very welcome, so overall, I think this is a marked improvement over the previous version.

Like Junio, I'm not familiar enough with sequencer or its 'expected behavior' to feel comfortable LGTM-ing the later patches.

On 07/08/2023 21:16, Glen Choo wrote:

Thanks again for you comments on the last round, they were really helpful in improving the commit messages.

Thanks, I'm glad the messages are easier to understand now

Yes, I'm hoping Dscho will have time to take a look at them once 2.42.0 is out.

Best Wishes

Phillip

This series fixes several bugs in the way we handle a commit cannot be picked because it would overwrite an untracked file.

 * after a failed pick "git rebase --continue" will happily commit any
   staged changes even though no commit was picked.

 * the commit of the failed pick is recorded as rewritten even though no
   commit was picked.

 * the "done" file used by "git status" to show the recently executed
   commands contains an incorrect entry.

Thanks to Eric, Glen and Junio for their comments on v2. Here are the changes since v2:

Patch 1 - Reworded the commit message.

Patch 2 - Reworded the commit message, added a test and fixed error message pointed out by Glen.

Patch 3 - New cleanup.

Patch 4 - Reworded the commit message, now only increments todo_list->current if there is no error.

Patch 5 - Swapped with next patch. Reworded the commit message, stopped testing implementation (suggested by Glen). Expanded post-rewrite hook test.

Patch 6 - Reworded the commit message, now uses the message file rather than the author script to check if "rebase --continue" should commit staged changes. Junio suggested using a separate file for this but I think that would end up being more involved as we'd need to be careful about creating and removing it.

Patch 7 - Reworded the commit message.

Thanks for the comments on V1, this series has now grown somewhat. Previously I was worried that refactoring would change the behavior, but having thought about it the current behavior is wrong and should be changed.

Changes since V1:

Rebased onto master to avoid a conflict with ab/remove-implicit-use-of-the-repository

 * Patches 1-3 are new preparatory changes
 * Patches 4 & 5 are new and fix the first two issues listed above.
 * Patch 6 is the old patch 1 which has been rebased and the commit message
   reworded. It fixes the last issues listed above.

Phillip Wood (7):
  rebase -i: move unlink() calls
  rebase -i: remove patch file after conflict resolution
  sequencer: use rebase_path_message()
  sequencer: factor out part of pick_commits()
  rebase: fix rewritten list for failed pick
  rebase --continue: refuse to commit after failed command
  rebase -i: fix adding failed command to the todo list

 sequencer.c                   | 182 ++++++++++++++++++----------------
 t/t3404-rebase-interactive.sh |  53 +++++++---
 t/t3418-rebase-continue.sh    |  18 ++++
 t/t3430-rebase-merges.sh      |  30 ++++--
 t/t5407-post-rewrite-hook.sh  |  48 +++++++++
 5 files changed, 228 insertions(+), 103 deletions(-)

base-commit: a80be152923a46f04a06bade7bcc72870e46ca09
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1492

Range-diff vs v3:

 1:  1ab1ad2ef07 ! 1:  ae4f873b3d0 rebase -i: move unlink() calls
     @@ Metadata
       ## Commit message ##
          rebase -i: move unlink() calls
      
     -    At the start of each iteration the loop that picks commits removes
     -    state files from the previous pick. However some of these are only
     -    written if there are conflicts and so we break out of the loop after
     -    writing them. Therefore they only need to be removed when the rebase
     -    continues, not in each iteration.
     +    At the start of each iteration the loop that picks commits removes the
     +    state files from the previous pick. However some of these files are only
     +    written if there are conflicts in which case we exit the loop before the
     +    end of the loop body. Therefore they only need to be removed when the
     +    rebase continues, not at the start of each iteration.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
 2:  e2a758eb4a5 ! 2:  f540ed1d607 rebase -i: remove patch file after conflict resolution
     @@ Commit message
          now used in two different places rebase_path_patch() is added and used
          to obtain the path for the patch.
      
     +    To construct the path write_patch() previously used get_dir() which
     +    returns different paths depending on whether we're rebasing or
     +    cherry-picking/reverting. As this function is only called when
     +    rebasing it is safe to use a hard coded string for the directory
     +    instead. An assertion is added to make sure we don't starting calling
     +    this function when cherry-picking in the future.
     +
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## sequencer.c ##
     @@ sequencer.c: static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
        * For the post-rewrite hook, we make a list of rewritten commits and
        * their new sha1s.  The rewritten-pending list keeps the sha1s of
      @@ sequencer.c: static int make_patch(struct repository *r,
     + 	char hex[GIT_MAX_HEXSZ + 1];
     + 	int res = 0;
     + 
     ++	if (!is_rebase_i(opts))
     ++		BUG("make_patch should only be called when rebasing");
     ++
     + 	oid_to_hex_r(hex, &commit->object.oid);
     + 	if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)
       		return -1;
       	res |= write_rebase_head(&commit->object.oid);
       
 3:  8f6c0e40567 ! 3:  818bdaf772d sequencer: use rebase_path_message()
     @@ Commit message
          made function to get the path name instead. This was the last
          remaining use of the strbuf so remove it as well.
      
     +    As with the previous patch we now use a hard coded string rather than
     +    git_dir() when constructing the path. This is safe for the same
     +    reason (make_patch() is only called when rebasing) and is protected by
     +    the assertion added in the previous patch.
     +
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## sequencer.c ##
 4:  a1fad70f4b9 = 4:  bd67765a864 sequencer: factor out part of pick_commits()
 5:  df401945866 ! 5:  f6f330f7063 rebase: fix rewritten list for failed pick
     @@ Commit message
          disabled the user will see the messages from the merge machinery
          detailing the problem.
      
     -    To simplify writing REBASE_HEAD in this case pick_one_commit() is
     -    modified to avoid duplicating the code that adds the failed command
     -    back into the todo list.
     +    The code to add a failed command back into the todo list is duplicated
     +    between pick_one_commit() and the loop in pick_commits(). Both sites
     +    print advice about the command being rescheduled, decrement the current
     +    item and save the todo list. To avoid duplicating this code
     +    pick_one_commit() is modified to set a flag to indicate that the command
     +    should be rescheduled in the main loop. This simplifies things as only
     +    the remaining copy of the code needs to be modified to set REBASE_HEAD
     +    rather than calling error_with_patch().
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
 6:  2ed7cbe5fff = 6:  0ca5fccca17 rebase --continue: refuse to commit after failed command
 7:  bbe0afde512 = 7:  8d5f6d51e19 rebase -i: fix adding failed command to the todo list

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Rather than constructing the path in a struct strbuf use the ready made function to get the path name instead. This was the last remaining use of the strbuf so remove it as well.

As with the previous patch we now use a hard coded string rather than git_dir() when constructing the path. This is safe for the same reason (make_patch() is only called when rebasing) and is protected by the assertion added in the previous patch.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When a rebase stops for the user to resolve conflicts it writes a patch for the conflicting commit to .git/rebase-merge/patch. This file has been written since the introduction of "git-rebase-interactive.sh" in 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the idea was to enable the user inspect the conflicting commit in the same way as they could for the patch based rebase. This file should be deleted when the rebase continues as if the rebase stops for a failed "exec" command or a "break" command it is confusing to the user if there is a stale patch lying around from an unrelated command. As the path is now used in two different places rebase_path_patch() is added and used to obtain the path for the patch.

To construct the path write_patch() previously used get_dir() which returns different paths depending on whether we're rebasing or cherry-picking/reverting. As this function is only called when rebasing it is safe to use a hard coded string for the directory instead. An assertion is added to make sure we don't starting calling this function when cherry-picking in the future.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                | 16 ++++++++++++----
 t/t3418-rebase-continue.sh | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

At the start of each iteration the loop that picks commits removes the state files from the previous pick. However some of these files are only written if there are conflicts in which case we exit the loop before the end of the loop body. Therefore they only need to be removed when the rebase continues, not at the start of each iteration.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This simplifies the next commit. If a pick fails we now return the error at the end of the loop body rather than returning early, a successful "edit" command continues to return early. There are three things to check to ensure that removing the early return for an error does not change the behavior of the code:

(1) We could enter the block guarded by "if (reschedule)". This block
    is not entered because "reschedlue" is always zero when picking a
    commit.

(2) We could enter the block guarded by
    "else if (is_rebase_i(opts) &&  check_todo && !res)". This block is
    not entered when returning an error because "res" is non-zero in
    that case.

(3) todo_list->current could be incremented before returning. That is
    avoided by moving the increment which is of course a potential
    change in behavior itself. The move is safe because none of the
    callers look at todo_list after this function returns. Moving the
    increment makes it clear we only want to advance the current item
    if the command was successful.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 132 ++++++++++++++++++++++++++++------------------------
 1 file changed, 71 insertions(+), 61 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

git rebase keeps a list that maps the OID of each commit before it was rebased to the OID of the equivalent commit after the rebase. This list is used to drive the "post-rewrite" hook that is called at the end of a successful rebase. When a rebase stops for the user to resolve merge conflicts the OID of the commit being picked is written to ".git/rebase-merge/stopped-sha". Then when the rebase is continued that OID is added to the list of rewritten commits. Unfortunately if a commit cannot be picked because it would overwrite an untracked file we still write the "stopped-sha1" file. This means that when the rebase is continued the commit is added into the list of rewritten commits even though it has not been picked yet.

Fix this by not calling error_with_patch() for failed commands. The pick has failed so there is nothing to commit and therefore we do not want to set up the state files for committing staged changes when the rebase continues. This change means we no-longer write a patch for the failed command or display the error message printed by error_with_patch(). As the command has failed the patch isn't really useful and in any case the user can inspect the commit associated with the failed command by inspecting REBASE_HEAD. Unless the user has disabled it we already print an advice message that is more helpful than the message from error_with_patch() which the user will still see. Even if the advice is disabled the user will see the messages from the merge machinery detailing the problem.

The code to add a failed command back into the todo list is duplicated between pick_one_commit() and the loop in pick_commits(). Both sites print advice about the command being rescheduled, decrement the current item and save the todo list. To avoid duplicating this code pick_one_commit() is modified to set a flag to indicate that the command should be rescheduled in the main loop. This simplifies things as only the remaining copy of the code needs to be modified to set REBASE_HEAD rather than calling error_with_patch().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 19 +++++---------
 t/t3404-rebase-interactive.sh |  6 +++++
 t/t3430-rebase-merges.sh      |  4 +--
 t/t5407-post-rewrite-hook.sh  | 48 +++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 14 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a commit cannot be picked because it would overwrite an untracked file then "git rebase --continue" should refuse to commit any staged changes as the commit was not picked. This is implemented by refusing to commit if the message file is missing. The message file is chosen for this check because it is only written when "git rebase" stops for the user to resolve merge conflicts.

Existing commands that refuse to commit staged changes when continuing such as a failed "exec" rely on checking for the absence of the author script in run_git_commit(). This prevents the staged changes from being committed but prints

    error: could not open '.git/rebase-merge/author-script' for
    reading

before the message about not being able to commit. This is confusing to users and so checking for the message file instead improves the user experience. The existing test for refusing to commit after a failed exec is updated to check that we do not print the error message about a missing author script anymore.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   |  5 +++++
 t/t3404-rebase-interactive.sh | 18 +++++++++++++++++-
 t/t3430-rebase-merges.sh      |  4 ++++
 3 files changed, 26 insertions(+), 1 deletion(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When rebasing commands are moved from the todo list in "git-rebase-todo" to the "done" file (which is used by "git status" to show the recently executed commands) just before they are executed. This means that if a command fails because it would overwrite an untracked file it has to be added back into the todo list before the rebase stops for the user to fix the problem.

Unfortunately when a failed command is added back into the todo list the command preceding it is erroneously appended to the "done" file. This means that when rebase stops after "pick B" fails the "done" file contains

	pick A
	pick B
	pick A

instead of

	pick A
	pick B

This happens because save_todo() updates the "done" file with the previous command whenever "git-rebase-todo" is updated. When we add the failed pick back into "git-rebase-todo" we do not want to update "done". Fix this by adding a "reschedule" parameter to save_todo() which prevents the "done" file from being updated when adding a failed command back into the "git-rebase-todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file.

Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 12 ++++++------
 t/t3404-rebase-interactive.sh | 29 ++++++++++++++++++-----------
 t/t3430-rebase-merges.sh      | 22 ++++++++++++++++------
 3 files changed, 40 insertions(+), 23 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Thanks. This version looks good to me (although I am not as familiar with this part of the codebase as others on the Cc: line).

Hi Phillip,

On Wed, 6 Sep 2023, Phillip Wood via GitGitGadget wrote:

Thank you for indulging me. This iteration looks good to me!

Ciao, Johannes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Thanks, all.  Let's merge it down to 'next'.