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

Message ID 20181211161139.31686-5-newren@gmail.com
State New
Headers show
Series
  • Reimplement rebase --merge via interactive machinery
Related show

Commit Message

Elijah Newren Dec. 11, 2018, 4:11 p.m. UTC
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>
---
 builtin/am.c                 | 9 +++++++++
 git-rebase--merge.sh         | 2 ++
 t/t5407-post-rewrite-hook.sh | 3 +++
 3 files changed, 14 insertions(+)

Comments

Johannes Schindelin Jan. 21, 2019, 4:07 p.m. UTC | #1
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
> 
>
Elijah Newren Jan. 21, 2019, 5:59 p.m. UTC | #2
Hi Dscho,

On Mon, Jan 21, 2019 at 8:07 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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...)

This if-block is actually copy-pasted from the end of the do_commit()
function, since the same logic was needed in both places.  The fact
that a `fp == NULL` case never triggered for do_commit() suggests that
the check has never been needed in the wild (or perhaps it just
indicates a latent bug that no one has triggered yet).  However, it
does suggest a code cleanup regardless.  I thought of it as such a
small block that I didn't think to put it in a separate function, but
perhaps I should so that someone spotting the possibility of a NULL fp
could fix it for both callers in a single place.

Should I insert a preliminary change pulling this block out of
do_commit into a separate function, and then modify this patch to just
call this function?  Or perhaps given the length of time it has
already been cooking (and Junio's rerere resolution of our two series
that I don't want to mess up), just do it as a simple post-series
fixup?


Elijah
Johannes Schindelin Jan. 21, 2019, 6:11 p.m. UTC | #3
Hi Elijah,

On Mon, 21 Jan 2019, Elijah Newren wrote:

> On Mon, Jan 21, 2019 at 8:07 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > 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...)
> 
> This if-block is actually copy-pasted from the end of the do_commit()
> function, since the same logic was needed in both places.  The fact
> that a `fp == NULL` case never triggered for do_commit() suggests that
> the check has never been needed in the wild (or perhaps it just
> indicates a latent bug that no one has triggered yet).  However, it
> does suggest a code cleanup regardless.  I thought of it as such a
> small block that I didn't think to put it in a separate function, but
> perhaps I should so that someone spotting the possibility of a NULL fp
> could fix it for both callers in a single place.
> 
> Should I insert a preliminary change pulling this block out of
> do_commit into a separate function, and then modify this patch to just
> call this function?  Or perhaps given the length of time it has
> already been cooking (and Junio's rerere resolution of our two series
> that I don't want to mess up), just do it as a simple post-series
> fixup?

Speaking for myself: I am fine with either way.

Ciao,
Dscho

Patch
diff mbox series

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));
+		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