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", ©, N_("copy a branch and its reflog"),
> 1),
> OPT_BIT('C', NULL, ©, 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
