Re: [PATCH 2/3] sequencer: remove REBASE_HEAD at the end of a rebase

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

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

> Several existing tests are extended to check that REBASE_HEAD is
> present when we stop for conflicts and is absent when the rebase
> finishes.

Good.

> diff --git a/sequencer.c b/sequencer.c
> index 2ac78e15b89..2d6a5f525c7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1781,7 +1781,10 @@ static int do_commit(struct repository *r,
>  		if (is_rebase_i(opts) && oid)
>  			if (write_rebase_head(oid))
>  			    return -1;
> -		return run_git_commit(msg_file, reflog_action, opts, flags);
> +		res = run_git_commit(msg_file, reflog_action, opts, flags);
> +		if (!res && is_rebase_i(opts) && oid)
> +			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
> +					NULL, REF_NO_DEREF);
>  	}
>  
>  	return res;
> @@ -5178,6 +5181,8 @@ static int pick_commits(struct repository *r,
>  	unlink(rebase_path_amend());
>  	unlink(rebase_path_patch());
>  	unlink(git_path_merge_labels(r));
> +	refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD", NULL,
> +			REF_NO_DEREF);
>  
>  	while (todo_list->current < todo_list->nr) {
>  		struct todo_item *item = todo_list->items + todo_list->current;
> @@ -5206,8 +5211,6 @@ static int pick_commits(struct repository *r,
>  			unlink(git_path_merge_head(r));
>  			refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
>  					NULL, REF_NO_DEREF);
> -			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
> -					NULL, REF_NO_DEREF);
>  
>  			if (item->command == TODO_BREAK) {
>  				if (!opts->verbose)

The patch is a bit dense (not a fault of the patch, but the blame
largely lies on the overly long functions involved with multi-level
callchain), but the idea is that the existing removal in the loop
inside pick_commits() may not trigger if the user resumes (via
"--continue" or "--skip") with an exhausted todo_list and that would
leave the REBASE_HEAD file behind, so we remove at the beginning of
pick_commits() before entering the loop, and also in do_commit(),
which is called deep inside the callchain from the loop to handle
one step, immediately before it returns on successful commit.

Which makes perfect sense.  The new tests seems to cover these cases
adequately, too.

Thanks.