Re: [PATCH 01/15] rebase: extend the options for handling of empty commits

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2019, 21:29 UTC
Message-ID: <xmqqimma8z3t.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <13e2056e780b00baf86d4020c0974b6b05ce115b.1576861788.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Extend the interactive machinery with the ability to handle the full
> spread of options for how to handle commits that either start or become
> empty (by "become empty" I mean the changes in a commit are a subset of
> changes that exist upstream, so the net effect of applying the commit is
> no changes).  Introduce a new command line flag for selecting the
> desired behavior:
>     --empty={drop,keep,ask}
> with the definitions:
>     drop: drop empty commits
>     keep: keep empty commits
>     ask:  provide the user a chance to interact and pick what to do with
>           empty commits on a case-by-case basis

This looks like a logical and natural extension of the --keep-empty option.

After seeing the stress on "empty from the beginning and ending up to be empty" in the description, I somehow expected that we may be able to specify what happens to the empty commit separately, but that does not seem to be what the patch is about, which was somewhat disappointing.

Show 10 quoted lines

> +static long parse_empty_value(const char *value)
> +{
> +	if (!value)
> +		return EMPTY_UNSPECIFIED;
> +	else if (!strcasecmp(value, "drop"))
> +		return EMPTY_DROP;
> +	else if (!strcasecmp(value, "keep"))
> +		return EMPTY_KEEP;
> +	else if (!strcasecmp(value, "ask"))
> +		return EMPTY_ASK;

Not an error but just silently ignored?

> +	return EMPTY_UNSPECIFIED;
> +}
> +

> +static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *options = opt->value;
> +	long value = parse_empty_value(arg);

Ahh, OK.

Wouldn't it be better to make the variable and the parsing helper function of type "enum empty_type", not "long", just like the field in the rebase_options struct?

Show 8 quoted lines

> +	BUG_ON_OPT_NEG(unset);
> +	if (value < 0)
> +		return error(_("option empty accepts \"drop\", "
> +			       "\"keep\", and \"ask\""));
> +
> +	options->empty = value;
> +	return 0;
> +}