Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Phillip Wood]

Hi Johannes

On 02/10/2018 14:50, Johannes Schindelin wrote:

Hi Phillip,

[sorry, I just got to this mail now]


Don't worry, I'm impressed you remembered it, I'd completely forgotten 
about it.




On Sun, 6 May 2018, Phillip Wood wrote:


On 27/04/18 21:48, Johannes Schindelin wrote:


During a series of fixup/squash commands, the interactive rebase builds
up a commit message with comments. This will be presented to the user in
the editor if at least one of those commands was a `squash`.

In any case, the commit message will be cleaned up eventually, removing
all those intermediate comments, in the final step of such a
fixup/squash chain.

However, if the last fixup/squash command in such a chain fails with
merge conflicts, and if the user then decides to skip it (or resolve it
to a clean worktree and then continue the rebase), the current code
fails to clean up the commit message.

This commit fixes that behavior.

The fix is quite a bit more involved than meets the eye because it is
not only about the question whether we are `git rebase --skip`ing a
fixup or squash. It is also about removing the skipped fixup/squash's
commit message from the accumulated commit message. And it is also about
the question whether we should let the user edit the final commit
message or not ("Was there a squash in the chain *that was not
skipped*?").

For example, in this case we will want to fix the commit message, but
not open it in an editor:

pick<- succeeds
fixup   <- succeeds
squash  <- fails, will be skipped

This is where the newly-introduced `current-fixups` file comes in real
handy. A quick look and we can determine whether there was a non-skipped
squash. We only need to make sure to keep it up to date with respect to
skipped fixup/squash commands. As a bonus, we can even avoid committing
unnecessarily, e.g. when there was only one fixup, and it failed, and
was skipped.

To fix only the bug where the final commit message was not cleaned up
properly, but without fixing the rest, would have been more complicated
than fixing it all in one go, hence this commit lumps together more than
a single concern.

For the same reason, this commit also adds a bit more to the existing
test case for the regression we just fixed.

The diff is best viewed with --color-moved.

Signed-off-by: Johannes Schindelin 
---
  sequencer.c| 113 -
  t/t3418-rebase-continue.sh |  35 ++--
  2 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 56166b0d6c7..cec180714ef 100644
--- a/sequencer.c
+++ b/sequencer.c
[...]
@@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts 
*opts)
if (get_oid_hex(rev.buf, _amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
-   if (oidcmp(, _amend))
+   if (!is_clean && oidcmp(, _amend))
return error(_("\nYou have uncommitted changes in your "
   "working tree. Please, commit them\n"
   "first and then run 'git rebase "
   "--continue' again."));
+   /*
+* When skipping a failed fixup/squash, we need to edit the
+* commit message, the current fixup list and count, and if it
+* was the last fixup/squash in the chain, we need to clean up
+* the commit message and if there was a squash, let the user
+* edit it.
+*/
+   if (is_clean && !oidcmp(, _amend) &&
+   opts->current_fixup_count > 0 &&
+   file_exists(rebase_path_stopped_sha())) {
+   const char *p = opts->current_fixups.buf;
+   int len = opts->current_fixups.len;
+
+   opts->current_fixup_count--;
+   if (!len)
+   BUG("Incorrect current_fixups:\n%s", p);
+   while (len && p[len - 1] != '\n')
+   len--;
+   strbuf_setlen(>current_fixups, len);
+   if (write_message(p, len, rebase_path_current_fixups(),
+ 0) < 0)
+   return error(_("could not write file: '%s'"),
+rebase_path_current_fixups());
+
+   /*
+* If a fixup/squash in a fixup/squash chain failed, the
+* commit message is already correct, no need to commit
+* it again.
+*
+* Only if it is the final command in the fixup/squash
+* chain, and only if the chain is longer than a single
+   

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Johannes Schindelin]

Hi Phillip,

[sorry, I just got to this mail now]

On Sun, 6 May 2018, Phillip Wood wrote:

> On 27/04/18 21:48, Johannes Schindelin wrote:
> > 
> > During a series of fixup/squash commands, the interactive rebase builds
> > up a commit message with comments. This will be presented to the user in
> > the editor if at least one of those commands was a `squash`.
> > 
> > In any case, the commit message will be cleaned up eventually, removing
> > all those intermediate comments, in the final step of such a
> > fixup/squash chain.
> > 
> > However, if the last fixup/squash command in such a chain fails with
> > merge conflicts, and if the user then decides to skip it (or resolve it
> > to a clean worktree and then continue the rebase), the current code
> > fails to clean up the commit message.
> > 
> > This commit fixes that behavior.
> > 
> > The fix is quite a bit more involved than meets the eye because it is
> > not only about the question whether we are `git rebase --skip`ing a
> > fixup or squash. It is also about removing the skipped fixup/squash's
> > commit message from the accumulated commit message. And it is also about
> > the question whether we should let the user edit the final commit
> > message or not ("Was there a squash in the chain *that was not
> > skipped*?").
> > 
> > For example, in this case we will want to fix the commit message, but
> > not open it in an editor:
> > 
> > pick<- succeeds
> > fixup   <- succeeds
> > squash  <- fails, will be skipped
> > 
> > This is where the newly-introduced `current-fixups` file comes in real
> > handy. A quick look and we can determine whether there was a non-skipped
> > squash. We only need to make sure to keep it up to date with respect to
> > skipped fixup/squash commands. As a bonus, we can even avoid committing
> > unnecessarily, e.g. when there was only one fixup, and it failed, and
> > was skipped.
> > 
> > To fix only the bug where the final commit message was not cleaned up
> > properly, but without fixing the rest, would have been more complicated
> > than fixing it all in one go, hence this commit lumps together more than
> > a single concern.
> > 
> > For the same reason, this commit also adds a bit more to the existing
> > test case for the regression we just fixed.
> > 
> > The diff is best viewed with --color-moved.
> > 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c| 113 -
> >  t/t3418-rebase-continue.sh |  35 ++--
> >  2 files changed, 131 insertions(+), 17 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index 56166b0d6c7..cec180714ef 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > [...]
> > @@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct 
> > replay_opts *opts)
> > if (get_oid_hex(rev.buf, _amend))
> > return error(_("invalid contents: '%s'"),
> > rebase_path_amend());
> > -   if (oidcmp(, _amend))
> > +   if (!is_clean && oidcmp(, _amend))
> > return error(_("\nYou have uncommitted changes in your "
> >"working tree. Please, commit them\n"
> >"first and then run 'git rebase "
> >"--continue' again."));
> > +   /*
> > +* When skipping a failed fixup/squash, we need to edit the
> > +* commit message, the current fixup list and count, and if it
> > +* was the last fixup/squash in the chain, we need to clean up
> > +* the commit message and if there was a squash, let the user
> > +* edit it.
> > +*/
> > +   if (is_clean && !oidcmp(, _amend) &&
> > +   opts->current_fixup_count > 0 &&
> > +   file_exists(rebase_path_stopped_sha())) {
> > +   const char *p = opts->current_fixups.buf;
> > +   int len = opts->current_fixups.len;
> > +
> > +   opts->current_fixup_count--;
> > +   if (!len)
> > +   BUG("Incorrect current_fixups:\n%s", p);
> > +   while (len && p[len - 1] != '\n')
> > +   len--;
> > +   strbuf_setlen(>current_fixups, len);
> > +   if (write_message(p, len, rebase_path_current_fixups(),
> > + 0) < 0)
> > +   return error(_("could not write file: '%s'"),
> > +rebase_path_current_fixups());
> > +
> > +   /*
> > +* If a fixup/squash in a fixup/squash chain failed, the
> > +* commit message is already correct, no need to commit
> > +* it again.
> > +*
> > +* Only if it is the final command in the fixup/squash
> > +

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Phillip Wood]

On 06/05/18 18:50, Phillip Wood wrote:

Hi Johannes, sorry it's taken me a while to look at this. I think it
mostly makes sense to me, the code is well documented. I've got one
comment below

On 27/04/18 21:48, Johannes Schindelin wrote:


During a series of fixup/squash commands, the interactive rebase builds
up a commit message with comments. This will be presented to the user in
the editor if at least one of those commands was a `squash`.

In any case, the commit message will be cleaned up eventually, removing
all those intermediate comments, in the final step of such a
fixup/squash chain.

However, if the last fixup/squash command in such a chain fails with
merge conflicts, and if the user then decides to skip it (or resolve it
to a clean worktree and then continue the rebase), the current code
fails to clean up the commit message.

This commit fixes that behavior.

The fix is quite a bit more involved than meets the eye because it is
not only about the question whether we are `git rebase --skip`ing a
fixup or squash. It is also about removing the skipped fixup/squash's
commit message from the accumulated commit message. And it is also about
the question whether we should let the user edit the final commit
message or not ("Was there a squash in the chain *that was not
skipped*?").

For example, in this case we will want to fix the commit message, but
not open it in an editor:

pick<- succeeds
fixup   <- succeeds
squash  <- fails, will be skipped

This is where the newly-introduced `current-fixups` file comes in real
handy. A quick look and we can determine whether there was a non-skipped
squash. We only need to make sure to keep it up to date with respect to
skipped fixup/squash commands. As a bonus, we can even avoid committing
unnecessarily, e.g. when there was only one fixup, and it failed, and
was skipped.

To fix only the bug where the final commit message was not cleaned up
properly, but without fixing the rest, would have been more complicated
than fixing it all in one go, hence this commit lumps together more than
a single concern.

For the same reason, this commit also adds a bit more to the existing
test case for the regression we just fixed.

The diff is best viewed with --color-moved.

Signed-off-by: Johannes Schindelin 
---
  sequencer.c| 113 -
  t/t3418-rebase-continue.sh |  35 ++--
  2 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 56166b0d6c7..cec180714ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2779,19 +2779,16 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
  }
  
-static int commit_staged_changes(struct replay_opts *opts)

+static int commit_staged_changes(struct replay_opts *opts,
+struct todo_list *todo_list)
  {
unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
+   unsigned int final_fixup = 0, is_clean;
  
  	if (has_unstaged_changes(1))

return error(_("cannot rebase: You have unstaged changes."));
-   if (!has_uncommitted_changes(0)) {
-   const char *cherry_pick_head = git_path_cherry_pick_head();
  
-		if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))

-   return error(_("could not remove CHERRY_PICK_HEAD"));
-   return 0;
-   }
+   is_clean = !has_uncommitted_changes(0);
  
  	if (file_exists(rebase_path_amend())) {

struct strbuf rev = STRBUF_INIT;
@@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts 
*opts)
if (get_oid_hex(rev.buf, _amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
-   if (oidcmp(, _amend))
+   if (!is_clean && oidcmp(, _amend))
return error(_("\nYou have uncommitted changes in your "
   "working tree. Please, commit them\n"
   "first and then run 'git rebase "
   "--continue' again."));
+   /*
+* When skipping a failed fixup/squash, we need to edit the
+* commit message, the current fixup list and count, and if it
+* was the last fixup/squash in the chain, we need to clean up
+* the commit message and if there was a squash, let the user
+* edit it.
+*/
+   if (is_clean && !oidcmp(, _amend) &&
+   opts->current_fixup_count > 0 &&
+   file_exists(rebase_path_stopped_sha())) {
+   const char *p = opts->current_fixups.buf;
+   int len = opts->current_fixups.len;
+
+   opts->current_fixup_count--;
+   if (!len)

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Phillip Wood]

Hi Johannes, sorry it's taken me a while to look at this. I think it
mostly makes sense to me, the code is well documented. I've got one
comment below

On 27/04/18 21:48, Johannes Schindelin wrote:
> 
> During a series of fixup/squash commands, the interactive rebase builds
> up a commit message with comments. This will be presented to the user in
> the editor if at least one of those commands was a `squash`.
> 
> In any case, the commit message will be cleaned up eventually, removing
> all those intermediate comments, in the final step of such a
> fixup/squash chain.
> 
> However, if the last fixup/squash command in such a chain fails with
> merge conflicts, and if the user then decides to skip it (or resolve it
> to a clean worktree and then continue the rebase), the current code
> fails to clean up the commit message.
> 
> This commit fixes that behavior.
> 
> The fix is quite a bit more involved than meets the eye because it is
> not only about the question whether we are `git rebase --skip`ing a
> fixup or squash. It is also about removing the skipped fixup/squash's
> commit message from the accumulated commit message. And it is also about
> the question whether we should let the user edit the final commit
> message or not ("Was there a squash in the chain *that was not
> skipped*?").
> 
> For example, in this case we will want to fix the commit message, but
> not open it in an editor:
> 
>   pick<- succeeds
>   fixup   <- succeeds
>   squash  <- fails, will be skipped
> 
> This is where the newly-introduced `current-fixups` file comes in real
> handy. A quick look and we can determine whether there was a non-skipped
> squash. We only need to make sure to keep it up to date with respect to
> skipped fixup/squash commands. As a bonus, we can even avoid committing
> unnecessarily, e.g. when there was only one fixup, and it failed, and
> was skipped.
> 
> To fix only the bug where the final commit message was not cleaned up
> properly, but without fixing the rest, would have been more complicated
> than fixing it all in one go, hence this commit lumps together more than
> a single concern.
> 
> For the same reason, this commit also adds a bit more to the existing
> test case for the regression we just fixed.
> 
> The diff is best viewed with --color-moved.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c| 113 -
>  t/t3418-rebase-continue.sh |  35 ++--
>  2 files changed, 131 insertions(+), 17 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 56166b0d6c7..cec180714ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2779,19 +2779,16 @@ static int continue_single_pick(void)
>   return run_command_v_opt(argv, RUN_GIT_CMD);
>  }
>  
> -static int commit_staged_changes(struct replay_opts *opts)
> +static int commit_staged_changes(struct replay_opts *opts,
> +  struct todo_list *todo_list)
>  {
>   unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
> + unsigned int final_fixup = 0, is_clean;
>  
>   if (has_unstaged_changes(1))
>   return error(_("cannot rebase: You have unstaged changes."));
> - if (!has_uncommitted_changes(0)) {
> - const char *cherry_pick_head = git_path_cherry_pick_head();
>  
> - if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> - return error(_("could not remove CHERRY_PICK_HEAD"));
> - return 0;
> - }
> + is_clean = !has_uncommitted_changes(0);
>  
>   if (file_exists(rebase_path_amend())) {
>   struct strbuf rev = STRBUF_INIT;
> @@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts 
> *opts)
>   if (get_oid_hex(rev.buf, _amend))
>   return error(_("invalid contents: '%s'"),
>   rebase_path_amend());
> - if (oidcmp(, _amend))
> + if (!is_clean && oidcmp(, _amend))
>   return error(_("\nYou have uncommitted changes in your "
>  "working tree. Please, commit them\n"
>  "first and then run 'git rebase "
>  "--continue' again."));
> + /*
> +  * When skipping a failed fixup/squash, we need to edit the
> +  * commit message, the current fixup list and count, and if it
> +  * was the last fixup/squash in the chain, we need to clean up
> +  * the commit message and if there was a squash, let the user
> +  * edit it.
> +  */
> + if (is_clean && !oidcmp(, _amend) &&
> + opts->current_fixup_count > 0 &&
> + file_exists(rebase_path_stopped_sha())) {
> + const char *p = opts->current_fixups.buf;
> + int len = 

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Johannes Schindelin]

Hi Stefan,

On Fri, 27 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 27, 2018 at 1:48 PM, Johannes Schindelin
>  wrote:
> > During a series of fixup/squash commands, the interactive rebase builds
> > up a commit message with comments. This will be presented to the user in
> > the editor if at least one of those commands was a `squash`.
> 
> This sounds as if the whole series will be presented to the user, i.e.
> 
>  pick A
>  squash B
>  fixup C
> 
> would present A+B+C in the editor.

And that is indeed the case. The commit message would look something like
this:

# This is a combination of 3 commits.
# This is commit message #1:

Hello Stefan

This is A.

# This is commit message #2:

squash! A

Me again, Stefan. I am here to be squashed.

# The commit message #3 will be skipped:
#
# fixup! A

> I always assumed the sequencer to be linear, i.e. pick A+B, open editor
> and then fixup C into the previous result?

Nope.

> No need to resend it reworded, I just realize that I never tested my
> potentially wrong assumption.

No worries, you learned something today.

> > The diff is best viewed with --color-moved.
> 
> ... and web pages are "best viewed with IE 6.0" ;-)

That is what I had in mind writing that.

> I found this so funny that I had to download the patches and actually
> look at them using the move detection only to find out that only very
> few lines are moved, as there are only very few deleted lines.

I agree that the current iteration is no longer such obvious a move. I had
to add tons of stuff to fix the extra issues I found while working on v4.

But still, I found it super-helpful to see that the code was actually
moved, and where, because I essentially had to break up the nice sequence
of "is it clean? Yes? Then nothing to be done! No? Is HEAD to be amended?
Yes? No?" and basically build a matrix what to do in all combinations of
"clean? Amend HEAD?"

Ciao,
Dscho

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Stefan Beller]

On Fri, Apr 27, 2018 at 1:48 PM, Johannes Schindelin
 wrote:
> During a series of fixup/squash commands, the interactive rebase builds
> up a commit message with comments. This will be presented to the user in
> the editor if at least one of those commands was a `squash`.

This sounds as if the whole series will be presented to the user, i.e.

 pick A
 squash B
 fixup C

would present A+B+C in the editor. I always assumed the sequencer
to be linear, i.e. pick A+B, open editor and then fixup C into the
previous result?

No need to resend it reworded, I just realize that I never tested my
potentially wrong assumption.

> The diff is best viewed with --color-moved.

... and web pages are "best viewed with IE 6.0" ;-)

I found this so funny that I had to download the patches and actually
look at them
using the move detection only to find out that only very few lines are moved,
as there are only very few deleted lines.

[PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Johannes Schindelin]

During a series of fixup/squash commands, the interactive rebase builds
up a commit message with comments. This will be presented to the user in
the editor if at least one of those commands was a `squash`.

In any case, the commit message will be cleaned up eventually, removing
all those intermediate comments, in the final step of such a
fixup/squash chain.

However, if the last fixup/squash command in such a chain fails with
merge conflicts, and if the user then decides to skip it (or resolve it
to a clean worktree and then continue the rebase), the current code
fails to clean up the commit message.

This commit fixes that behavior.

The fix is quite a bit more involved than meets the eye because it is
not only about the question whether we are `git rebase --skip`ing a
fixup or squash. It is also about removing the skipped fixup/squash's
commit message from the accumulated commit message. And it is also about
the question whether we should let the user edit the final commit
message or not ("Was there a squash in the chain *that was not
skipped*?").

For example, in this case we will want to fix the commit message, but
not open it in an editor:

pick<- succeeds
fixup   <- succeeds
squash  <- fails, will be skipped

This is where the newly-introduced `current-fixups` file comes in real
handy. A quick look and we can determine whether there was a non-skipped
squash. We only need to make sure to keep it up to date with respect to
skipped fixup/squash commands. As a bonus, we can even avoid committing
unnecessarily, e.g. when there was only one fixup, and it failed, and
was skipped.

To fix only the bug where the final commit message was not cleaned up
properly, but without fixing the rest, would have been more complicated
than fixing it all in one go, hence this commit lumps together more than
a single concern.

For the same reason, this commit also adds a bit more to the existing
test case for the regression we just fixed.

The diff is best viewed with --color-moved.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c| 113 -
 t/t3418-rebase-continue.sh |  35 ++--
 2 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 56166b0d6c7..cec180714ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2779,19 +2779,16 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
 }
 
-static int commit_staged_changes(struct replay_opts *opts)
+static int commit_staged_changes(struct replay_opts *opts,
+struct todo_list *todo_list)
 {
unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
+   unsigned int final_fixup = 0, is_clean;
 
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
-   if (!has_uncommitted_changes(0)) {
-   const char *cherry_pick_head = git_path_cherry_pick_head();
 
-   if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
-   return error(_("could not remove CHERRY_PICK_HEAD"));
-   return 0;
-   }
+   is_clean = !has_uncommitted_changes(0);
 
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
@@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts 
*opts)
if (get_oid_hex(rev.buf, _amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
-   if (oidcmp(, _amend))
+   if (!is_clean && oidcmp(, _amend))
return error(_("\nYou have uncommitted changes in your "
   "working tree. Please, commit them\n"
   "first and then run 'git rebase "
   "--continue' again."));
+   /*
+* When skipping a failed fixup/squash, we need to edit the
+* commit message, the current fixup list and count, and if it
+* was the last fixup/squash in the chain, we need to clean up
+* the commit message and if there was a squash, let the user
+* edit it.
+*/
+   if (is_clean && !oidcmp(, _amend) &&
+   opts->current_fixup_count > 0 &&
+   file_exists(rebase_path_stopped_sha())) {
+   const char *p = opts->current_fixups.buf;
+   int len = opts->current_fixups.len;
+
+   opts->current_fixup_count--;
+   if (!len)
+   BUG("Incorrect current_fixups:\n%s", p);
+   while (len && p[len - 1] != '\n')
+   len--;
+   strbuf_setlen(>current_fixups, len);