Re: [PATCH v4 4/8] am, rebase--merge: do not overlook --skip'ed commits with post-rewrite

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 11 Dec 2018, Elijah Newren wrote:

> The post-rewrite hook is supposed to be invoked for each rewritten
> commit.  The fact that a commit was selected and processed by the rebase
> operation (even though when we hit an error a user said it had no more
> useful changes), suggests we should write an entry for it.  In
> particular, let's treat it as an empty commit trivially squashed into
> its parent.
> 
> This brings the rebase--am and rebase--merge backends in sync with the
> behavior of the interactive rebase backend.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>

This makes sense. I think, though, that we need to be more careful...

> ---
>  builtin/am.c                 | 9 +++++++++
>  git-rebase--merge.sh         | 2 ++
>  t/t5407-post-rewrite-hook.sh | 3 +++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/builtin/am.c b/builtin/am.c
> index 8f27f3375b..af9d034838 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2000,6 +2000,15 @@ static void am_skip(struct am_state *state)
>  	if (clean_index(&head, &head))
>  		die(_("failed to clean index"));
>  
> +	if (state->rebasing) {
> +		FILE *fp = xfopen(am_path(state, "rewritten"), "a");
> +
> +		assert(!is_null_oid(&state->orig_commit));
> +		fprintf(fp, "%s ", oid_to_hex(&state->orig_commit));

... here. What if `fp == NULL`? (Users do all kinds of interesting
things...)

Ciao,
Dscho

> +		fprintf(fp, "%s\n", oid_to_hex(&head));
> +		fclose(fp);
> +	}
> +
>  	am_next(state);
>  	am_load(state);
>  	am_run(state, 0);
> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> index aa2f2f0872..91250cbaed 100644
> --- a/git-rebase--merge.sh
> +++ b/git-rebase--merge.sh
> @@ -121,6 +121,8 @@ continue)
>  skip)
>  	read_state
>  	git rerere clear
> +	cmt="$(cat "$state_dir/cmt.$msgnum")"
> +	echo "$cmt $(git rev-parse HEAD^0)" >> "$state_dir/rewritten"
>  	msgnum=$(($msgnum + 1))
>  	while test "$msgnum" -le "$end"
>  	do
> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
> index 6426ec8991..a4a5903cba 100755
> --- a/t/t5407-post-rewrite-hook.sh
> +++ b/t/t5407-post-rewrite-hook.sh
> @@ -78,6 +78,7 @@ test_expect_success 'git rebase --skip' '
>  	git rebase --continue &&
>  	echo rebase >expected.args &&
>  	cat >expected.data <<-EOF &&
> +	$(git rev-parse C) $(git rev-parse HEAD^)
>  	$(git rev-parse D) $(git rev-parse HEAD)
>  	EOF
>  	verify_hook_input
> @@ -91,6 +92,7 @@ test_expect_success 'git rebase --skip the last one' '
>  	echo rebase >expected.args &&
>  	cat >expected.data <<-EOF &&
>  	$(git rev-parse E) $(git rev-parse HEAD)
> +	$(git rev-parse F) $(git rev-parse HEAD)
>  	EOF
>  	verify_hook_input
>  '
> @@ -120,6 +122,7 @@ test_expect_success 'git rebase -m --skip' '
>  	git rebase --continue &&
>  	echo rebase >expected.args &&
>  	cat >expected.data <<-EOF &&
> +	$(git rev-parse C) $(git rev-parse HEAD^)
>  	$(git rev-parse D) $(git rev-parse HEAD)
>  	EOF
>  	verify_hook_input
> -- 
> 2.20.0.8.g5de428d695
> 
>