Re: [PATCH v2 3/4] branch: deprecate "-l" option

[Jeff King <peff@peff.net>]

On Fri, Jun 22, 2018 at 06:34:28PM -0400, Eric Sunshine wrote:

> I wonder if it would be better and cleaner to limit the visibility of
> this change to cmd_branch(), rather than spreading it across a global
> variable, a callback function, and cmd_branch(). Perhaps, like this:

I'd prefer that, too, but...

> @@ -615,7 +616,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT_BIT('c', "copy", &copy, N_("copy a branch and its reflog"), 1),
>  		OPT_BIT('C', NULL, &copy, N_("copy a branch, even if target exists"), 2),
>  		OPT_BOOL(0, "list", &list, N_("list branch names")),
> -		OPT_BOOL('l', "create-reflog", &reflog, N_("create the branch's reflog")),
> +		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
> +		OPT_HIDDEN_BOOL('l', NULL, &deprecated_reflog_option,
> +				N_("deprecated synonym for --create-reflog")),

Now that "-l" processing is delayed, it interacts in a funny way with
--create-reflog. For instance:

  git branch -l --no-create-reflog

currently cancels itself out, but after your patch would enable reflogs.

This is a pretty niche corner case, but I think it's important not to
change any behavior during the deprecation period. You'd have to do
something more like:

  reflog = -1;

  ... parse options ...

  if (deprecated_reflog_option && !list)
    warning(...);
  if (reflog < 0 && deprecated_reflog_option)
    reflog = 1;

I think that probably works in all cases, but I actually think the
existing callback/global is less invasive.

-Peff