diff mbox series

[3/3] log: give --[no-]use-mailmap a more sensible synonym --[no-]mailmap

Message ID 20200316212857.259093-4-gitster@pobox.com (mailing list archive)
State New, archived
Headers show
Series [1/3] parse-options: teach "git cmd -h" to show alias as alias | expand

Commit Message

Junio C Hamano March 16, 2020, 9:28 p.m. UTC
The option name "--use-mailmap" looks OK, but it becomes awkward
when you have to negate it, i.e. "--no-use-mailmap".  I, perhaps
with many other users, always try "--no-mailmap" and become unhappy
to see it fail.

Add an alias "--[no-]mailmap" to remedy this.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-log.txt | 1 +
 builtin/log.c             | 1 +
 2 files changed, 2 insertions(+)

Comments

Eric Sunshine March 16, 2020, 9:39 p.m. UTC | #1
On Mon, Mar 16, 2020 at 5:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> The option name "--use-mailmap" looks OK, but it becomes awkward
> when you have to negate it, i.e. "--no-use-mailmap".  I, perhaps
> with many other users, always try "--no-mailmap" and become unhappy
> to see it fail.
>
> Add an alias "--[no-]mailmap" to remedy this.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> @@ -49,6 +49,7 @@ OPTIONS
> +--[no-]mailmap::
>  --[no-]use-mailmap::
>         Use mailmap file to map author and committer names and email
>         addresses to canonical real names and email addresses. See

Here, the documentation seems to promote --mailmap over --use-mailmap.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -173,6 +173,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>                 OPT_BOOL(0, "use-mailmap", &mailmap, N_("Use mail map file")),
> +               OPT_ALIAS(0, "mailmap", "use-mailmap"),

So, along the lines of patch 2/3, I wonder if this should instead make
--mailmap the "real" option and --use-mailmap the alias; namely, use
OPT_ALIAS for --use-mailmap and place it after --mailmap. (Genuine but
very minor question; should not hold up acceptance of patch.)
Junio C Hamano March 16, 2020, 10:38 p.m. UTC | #2
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 16, 2020 at 5:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>> The option name "--use-mailmap" looks OK, but it becomes awkward
>> when you have to negate it, i.e. "--no-use-mailmap".  I, perhaps
>> with many other users, always try "--no-mailmap" and become unhappy
>> to see it fail.
>>
>> Add an alias "--[no-]mailmap" to remedy this.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
>> @@ -49,6 +49,7 @@ OPTIONS
>> +--[no-]mailmap::
>>  --[no-]use-mailmap::
>>         Use mailmap file to map author and committer names and email
>>         addresses to canonical real names and email addresses. See
>
> Here, the documentation seems to promote --mailmap over --use-mailmap.
>
>> diff --git a/builtin/log.c b/builtin/log.c
>> @@ -173,6 +173,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>>                 OPT_BOOL(0, "use-mailmap", &mailmap, N_("Use mail map file")),
>> +               OPT_ALIAS(0, "mailmap", "use-mailmap"),
>
> So, along the lines of patch 2/3, I wonder if this should instead make
> --mailmap the "real" option and --use-mailmap the alias; namely, use
> OPT_ALIAS for --use-mailmap and place it after --mailmap. (Genuine but
> very minor question; should not hold up acceptance of patch.)

Actually, I do not think "--use-mailmap" is too bad.  It is just
"--no-use-mailmap" felt horrible.  If the enable-disable interface
for this feature were "--(no|use)-mailmap", I would not have written
this series.

There are two subcommands other than "log" that has "use-something"
that needs "--no-use-something" to countermand:

    "git fast-export" has "--use-done-feature"
    "git pack-objects" has "--use-bitmap-index"

So an alternative approach which might be better is to teach
parse-options interface to handle "--no-something" by doing the
following:

 - first find "--something" in the option[] array, and if there is,
   use it just like we do today;

 - if there is no option "--something" in the option[] array,
   instead of erroring out, see if "--use-something" is there, and
   pretend as if the user said "--no-use-something".

I guess that is very similar to the way we avoid "--no-no-frotz" for
option[] entries whose long name already begins with "--no-".


[Footnote]

Optionally, when the command line option says "--something", and
there is no such entry in the option[] array, we could check it with
the "use-" prefix, and pretend that "--use-something" was what the
user asked if there is such an entry.  But if we were to go that
route, it is not all that different to have an alias, as there are
only three subcommands (counting "log") that has the "use-something"
(and one of them, i.e. "pack-objects", is not even an end-user
facing command).
diff mbox series

Patch

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index bed09bb09e..619577f23b 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -49,6 +49,7 @@  OPTIONS
 	Print out the ref name given on the command line by which each
 	commit was reached.
 
+--[no-]mailmap::
 --[no-]use-mailmap::
 	Use mailmap file to map author and committer names and email
 	addresses to canonical real names and email addresses. See
diff --git a/builtin/log.c b/builtin/log.c
index 83a4a6188e..ca1e789ba0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -173,6 +173,7 @@  static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 		OPT__QUIET(&quiet, N_("suppress diff output")),
 		OPT_BOOL(0, "source", &source, N_("show source")),
 		OPT_BOOL(0, "use-mailmap", &mailmap, N_("Use mail map file")),
+		OPT_ALIAS(0, "mailmap", "use-mailmap"),
 		OPT_STRING_LIST(0, "decorate-refs", &decorate_refs_include,
 				N_("pattern"), N_("only decorate refs that match <pattern>")),
 		OPT_STRING_LIST(0, "decorate-refs-exclude", &decorate_refs_exclude,