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 <[email protected]> writes:
> 
>>   	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 ...

>> @@ -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