Re: [PATCH v2 2/6] commit: add amend suboption to --fixup to create amend! commit

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

Charvi Mendiratta <charvi077@gmail.com> writes:

> `git commit --fixup=amend:<commit>` will create an "amend!" commit.
> The resulting commit message subject will be "amend! ..." where
> "..." is the subject line of <commit> and the initial message
> body will be <commit>'s message. -m can be used to override the
> message body.
>
> The "amend!" commit when rebased with --autosquash will fixup the
> contents and replace the commit message of <commit> with the
> "amend!" commit's message body.
>
> Inorder to prevent rebase from creating commits with an empty

In order to prevent ...

> message we refuse to create an "amend!" commit if commit message
> body is empty.
> ...
> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> +								struct pretty_print_context *ctx) {

Don't indent the second line unnecessarily too deep.

> +	/*
> +	 * If we amend the 'amend!' commit then we don't want to
> +	 * duplicate the subject line.
> +	 */
> +	const char *format = NULL;
> +	if (starts_with(sb->buf, "amend! amend!"))
> +		format = "%b";
> +	else
> +		format = "%B";

I am not sure how well this strategy of special case only two amend!
scales.  What would happen when we --fixup another "fixup!"commit?

Shouldn't the caller, when it called format_commit_message() to
prepare sb it passed to us, have stripped out existing prefix, if
any, so that we can always use the same %B format, or something like
that?

> ...
> +		format_commit_message(commit, fmt, &sb, &ctx);
> +		free(fmt);
>  		hook_arg1 = "message";
> +
> +		if ((have_option_m) && !strcmp(fixup_prefix,"fixup"))

Unnecessary () around have_option_m, and missing SP after ",".

> +			strbuf_addbuf(&sb, &message);
> +
> +		if (!strcmp(fixup_prefix,"amend")) {

Missing SP after "," (I won't repeat---please re-check the whole
patch series before rerolling).

> +			if (have_option_m)
> +				die(_("cannot combine -m with --fixup:%s"), fixup_message);
> +			else
> +				prepare_amend_commit(commit, &sb, &ctx);

Hmph, why is -m so special?  Should we allow --fixup=amend:<cmd>
with -F (or -c/-C for that matter), or are these other options
caught at a lot higher layer already and we do not have to check
them here?

>  	if (also + only + all + interactive > 1)
>  		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
> +
> +	if (fixup_message) {
> +		/*
> +		 * As `amend` suboption contains only alpha
> +		 * character. So check if first non alpha
> +		 * character in fixup_message is ':'.
> +		 */
> +		size_t len = get_alpha_len(fixup_message);
> +		if (len && fixup_message[len] == ':') {
> +			fixup_message[len] = '\0';
> +			fixup_commit = fixup_message + ++len;

It would be easier to follow to write it this way, no?

			fixup_message[len++] = '\0';
			fixup_commit = fixup_message + len;

> +			if (starts_with("amend", fixup_message))
> +				fixup_prefix = "amend";
> +			else
> +				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
> +		} else {
> +			fixup_commit = fixup_message;
> +			fixup_prefix = "fixup";
> +			use_editor = 0;
> +		}
> +	}