Re: [PATCH v2 3/4] branch: deprecate "-l" option
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", , N_("list branch names")), > - OPT_BOOL('l', "create-reflog", , N_("create the branch's > reflog")), > + OPT_BOOL(0, "create-reflog", , N_("create the branch's > reflog")), > + OPT_HIDDEN_BOOL('l', NULL, _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
Re: [PATCH v2 3/4] branch: deprecate "-l" option
On Fri, Jun 22, 2018 at 05:24:14AM -0400, Jeff King wrote: > Let's deprecate "-l" in hopes of eventually re-purposing it > to "--list". > > Signed-off-by: Jeff King > --- > diff --git a/builtin/branch.c b/builtin/branch.c > @@ -36,6 +36,7 @@ static const char * const builtin_branch_usage[] = { > +static int used_deprecated_reflog_option; > @@ -573,6 +574,14 @@ static int edit_branch_description(const char > *branch_nam> +static int deprecated_reflog_option_cb(const struct option *opt, > +const char *arg, int unset) > +{ > + used_deprecated_reflog_option = 1; > + *(int *)opt->value = !unset; > + return 0; > +} > @@ -615,7 +624,13 @@ int cmd_branch(int argc, const char **argv, const char > *p> + OPT_BOOL(0, "create-reflog", , N_("create the > branch's reflog")), > + { > + OPTION_CALLBACK, 'l', NULL, , NULL, > + N_("deprecated synonym for --create-reflog"), > + PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, > + deprecated_reflog_option_cb > @@ -688,6 +703,11 @@ int cmd_branch(int argc, const char **argv, const char > *p> + if (used_deprecated_reflog_option && !list) { > + warning("the '-l' alias for '--create-reflog' is deprecated;"); > + warning("it will be removed in a future version of Git"); > + } 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: --- >8 --- diff --git a/builtin/branch.c b/builtin/branch.c index 5217ba3bde..893e5f481a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -584,6 +584,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) int icase = 0; static struct ref_sorting *sorting = NULL, **sorting_tail = struct ref_format format = REF_FORMAT_INIT; + int deprecated_reflog_option = 0; struct option options[] = { OPT_GROUP(N_("Generic options")), @@ -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", , N_("list branch names")), - OPT_BOOL('l', "create-reflog", , N_("create the branch's reflog")), + OPT_BOOL(0, "create-reflog", , N_("create the branch's reflog")), + OPT_HIDDEN_BOOL('l', NULL, _reflog_option, + N_("deprecated synonym for --create-reflog")), OPT_BOOL(0, "edit-description", _description, N_("edit the description for the branch")), OPT__FORCE(, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE), @@ -688,6 +691,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (list) setup_auto_pager("branch", 1); + if (deprecated_reflog_option && !list) { + reflog = 1; + warning("the '-l' alias for '--create-reflog' is deprecated;"); + warning("it will be removed in a future version of Git"); + } + if (delete) { if (!argc) die(_("branch name required")); --- >8 ---
[PATCH v2 3/4] branch: deprecate "-l" option
The "-l" option is short for "--create-reflog". This has caused much confusion over the years. Most people expect it to work as "--list", because that would match the other "mode" options like -d/--delete and -m/--move, as well as the similar -l/--list option of git-tag. Adding to the confusion, using "-l" _appears_ to work as "--list" in some cases: $ git branch -l * master because the branch command defaults to listing (so even trying to specify --list in the command above is redundant). But that may bite the user later when they add a pattern, like: $ git branch -l foo which does not return an empty list, but in fact creates a new branch (with a reflog, naturally) called "foo". It's also probably quite uncommon for people to actually use "-l" to create a reflog. Since 0bee591869 (Enable reflogs by default in any repository with a working directory., 2006-12-14), this is the default in non-bare repositories. So it's rather unfortunate that the feature squats on the short-and-sweet "-l" (which was only added in 3a4b3f269c (Create/delete branch ref logs., 2006-05-19), meaning there were only 7 months where it was actually useful). Let's deprecate "-l" in hopes of eventually re-purposing it to "--list". Note that we issue the warning only when we're not in list mode. This means that people for whom it works as a happy accident, namely: $ git branch -l master won't see the warning at all. And when we eventually switch to it meaning "--list", that will just continue to work. We do the issue the warning for these important cases: - when we are actually creating a branch, in case the user really did mean it as "--create-reflog" - when we are in some _other_ mode, like deletion. There the "-l" is a noop for now, but it will eventually conflict with any other mode request, and the user should be told that this is changing. Signed-off-by: Jeff King --- This one has the interesting changes in this re-roll. Documentation/git-branch.txt | 3 ++- builtin/branch.c | 22 +- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index 02eccbb931..1072ca0eb6 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -91,7 +91,6 @@ OPTIONS -D:: Shortcut for `--delete --force`. --l:: --create-reflog:: Create the branch's reflog. This activates recording of all changes made to the branch ref, enabling use of date @@ -101,6 +100,8 @@ OPTIONS The negated form `--no-create-reflog` only overrides an earlier `--create-reflog`, but currently does not negate the setting of `core.logAllRefUpdates`. ++ +The `-l` option is a deprecated synonym for `--create-reflog`. -f:: --force:: diff --git a/builtin/branch.c b/builtin/branch.c index 5217ba3bde..ed4c093747 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -36,6 +36,7 @@ static const char * const builtin_branch_usage[] = { static const char *head; static struct object_id head_oid; +static int used_deprecated_reflog_option; static int branch_use_color = -1; static char branch_colors[][COLOR_MAXLEN] = { @@ -573,6 +574,14 @@ static int edit_branch_description(const char *branch_name) return 0; } +static int deprecated_reflog_option_cb(const struct option *opt, + const char *arg, int unset) +{ + used_deprecated_reflog_option = 1; + *(int *)opt->value = !unset; + return 0; +} + int cmd_branch(int argc, const char **argv, const char *prefix) { int delete = 0, rename = 0, copy = 0, force = 0, list = 0; @@ -615,7 +624,13 @@ 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", , N_("list branch names")), - OPT_BOOL('l', "create-reflog", , N_("create the branch's reflog")), + OPT_BOOL(0, "create-reflog", , N_("create the branch's reflog")), + { + OPTION_CALLBACK, 'l', NULL, , NULL, + N_("deprecated synonym for --create-reflog"), + PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, + deprecated_reflog_option_cb + }, OPT_BOOL(0, "edit-description", _description, N_("edit the description for the branch")), OPT__FORCE(, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE), @@ -688,6 +703,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (list) setup_auto_pager("branch", 1); + if (used_deprecated_reflog_option && !list) { + warning("the '-l' alias for '--create-reflog' is deprecated;"); +