Re: [PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash

Hi Junio

On 24/07/2026 22:18, Junio C Hamano wrote:

Phillip Wood [off-list ref] writes:

quoted

  	return starts_with(ctx->current_fixups.buf, "squash") ||
  		strstr(ctx->current_fixups.buf, "\nsquash");
+}
+
+/* Does the current fixup chain contain a "fixup -c" command? */
+static int seen_fixup_edit_msg(struct replay_ctx *ctx)
+{
+	return starts_with(ctx->current_fixups.buf, "fixup -c") ||
+		strstr(ctx->current_fixups.buf, "\nfixup -c");
  }

It is a bit annoying that "git diff" decided to consider the "}" at
the end of the otherwise unmodified function to be the one that was
added X-<.  But thanks to it, we can see this mirrors the previous
function to check if we have "squash" anywhere.  I wonder what
diff-algorithm was used to produce this result, but it is an
unrelated tangent.

Patience diff without the diff slider. When I was reviewing some of 
Ezekiel's patches I noticed that the diff slider was munging some diffs 
generated by patience in a way I didn't like so I tried turning it off 
to see what happened. It seems I haven't rebuilt my local git in a while ...

quoted

@@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,
  				 * message, no need to bother the user with
  				 * opening the commit message in the editor.
  				 */
-				if (!starts_with(p, "squash ") &&
-				    !strstr(p, "\nsquash "))
+				if (!seen_squash(ctx) &&
+				    !seen_fixup_edit_msg(ctx))
  					flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;

If 'fixup -c' is anywhere in the chain, we would need to offer the
user a chance to edit (similar to having 'squash').

It is a bit surprising that the 'squash' detection, for which we
already had a helper function, was open-coded here.  I also notice
that the helpers (including the new 'fixup -c' one) do not insist on
having a space immediately after the verb 'squash'.  Should we add
one above?

I'm not sure the space thing makes much difference as this isn't the 
todo file that the user edits. We're reading a file that we've written 
and the lines can only start with "fixup" or "squash"

Other than these minor nits, this looks good.

It is a bit disappointing that, with so many users who crucially
depend on the proper operation of 'rebase -i', we have received no
review comments on these two patches so far.  Perhaps summer is a
truly quiet and slow season ;-)

Oswald mentioned in another thread that he'd read these and they seemed 
to make sense. In general I find it hard to attract reviewers for 
rebase/sequencer patches - it is one of those features that everyone 
uses but not many people on the list seem to be familiar with the code.

I will wait for a few more days and then mark the topic for 'next'.

Thanks for your review, I've sent a re-roll fixing the newline detection 
in the previous patch.

Thanks

Phillip