| Message ID | 20181211161139.31686-5-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reimplement rebase --merge via interactive machinery | expand |
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 > >
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
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
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
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(+)