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

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> 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.

It is a bit surprising that we do not carefully parse each line to
identify a 'squash' or a 'fixup -c', which would make it unnecessary
to guess whether the current line is what we are looking for or if
the desired string immediately follows a newline later on.  Still,
this patch inherits that pattern from the original code, so it is
not a fault of this change.

>  static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
> @@ -2148,9 +2155,14 @@ static int update_squash_messages(struct repository *r,
>  	strbuf_release(&buf);
>  
>  	if (!res) {
> -		strbuf_addf(&ctx->current_fixups, "%s%s %s",
> +		const char *fixup_flag = "";
> +
> +		if (is_fixup_flag(command, flag) && (flag & TODO_EDIT_FIXUP_MSG))
> +			fixup_flag = " -c";
> +
> +		strbuf_addf(&ctx->current_fixups, "%s%s%s %s",
>  			    ctx->current_fixups.len ? "\n" : "",
> -			    command_to_string(command),
> +			    command_to_string(command), fixup_flag,
>  			    oid_to_hex(&commit->object.oid));
>  		res = write_message(ctx->current_fixups.buf,
>  				    ctx->current_fixups.len,
> @@ -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?

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

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

Thanks.