On Fri, Dec 20, 2019 at 1:29 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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.
My main reason for stressing both was that I've commits and code that
talked about "empty" commits, but which often only dealt with one of
the two types (sometimes when I would expect both to have been
addressed), so I was just trying to be careful in my explanation of
what it was about.
If wanted, we could implement finer control, but I don't know what the
UI would look like or what the range of options we might want. Do we
want to mix all possible permutations and have two flags, e.g.
--become-empty=drop --start-empty=keep or --start-empty=ask
--become-empty=keep? Would --empty=<whatever> imply both
--become-empty=<whatever> and --start-empty=<whatever> or would users
need to specify both? Do we think all the permutations make sense, or
would we want to limit users to some subset?
The current choice of three covered the things that I had wanted in
the past, and which I think made for good defaults for various
different flags, so I just implemented it. We could always extend it
later.
>
> > +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?
Oops, I'll switch it to an error.
> > + 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?
Indeed, I'll fix this up.
