threads / patch / 53005

patch, 3 partsI keep typing "git log --no-mailmap" X-<

Subject: [PATCH 0/3] I keep typing "git log --no-mailmap" X-<

## tl;dr

6 messages between Mar 16, 2020 and Mar 16, 2020. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Junio C Hamano· Mar 16, 2020, 21:28 UTC · lore

The ultimate objective of this miniseries is to allow me to say "git log --no-mailmap" by adding an alias "--mailmap" to the existing "--use-mailmap" option.

But I had to do a little cleaning up of the OPT_ALIAS() interface, which is the topic of the first two patches. Without them, an option that is an alias to another option is shown in "git cmd -h" output like this illustration:

    $ git clone -h
    usage: git clone [<options>] [--] <repo> [<dir>]
	...
	--recursive[=<pathspec>]
			      initialize submodules in the clone
	--recurse-submodules[=<pathspec>]
			      initialize submodules in the clone
	-j, --jobs <n>        number of submodules cloned in parallel
	...

The "recursive" option is defined as an alias to "recurse-submodules", but with exactly the same short text, the user may realize that they are identical, or they may suspect they are largely similar with subtle differences that the short help text cannot adequately convey.

With the first two patches, the output becomes like this:
    $ git clone -h
    usage: git clone [<options>] [--] <repo> [<dir>]
	...
	--recurse-submodules[=<pathspec>]
			      initialize submodules in the clone
	--recursive[=<pathspec>]
			      alias of --recurse-submodules
	-j, --jobs <n>        number of submodules cloned in parallel
	...

which would make it clear that the latter is a way to spell the same thing as the other.

Junio C Hamano (3):
  parse-options: teach "git cmd -h" to show alias as alias
  clone: reorder --recursive/--recurse-submodules
  log: give --[no-]use-mailmap a more sensible synonym --[no-]mailmap
 Documentation/git-log.txt | 1 +
 builtin/clone.c           | 2 +-
 builtin/log.c             | 1 +
 parse-options.c           | 9 +++------
 t/t0040-parse-options.sh  | 2 +-
 5 files changed, 7 insertions(+), 8 deletions(-)
-- 
2.26.0-rc1-11-g30e9940356
Junio C Hamano· Mar 16, 2020, 21:28 UTC · re: Junio C Hamano · lore

[PATCH 2/3] clone: reorder --recursive/--recurse-submodules

The previous step made an option that is an alias to another option identify itself as an alias to the latter. Because it is easier to scan the list when a pointer goes backward to what a reader already has seen, mention "recurse-submodules" first with its true short help string, and then "recurse" with the statement that it is a synonym to "recurse-submodules".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 The history of --recursive/--recurse-submodules options dates back
 to ccdd3da6 (clone: add the --recurse-submodules option as alias
 for --recursive, 2010-11-04), where we decided to encourage use of
 more descriptive "--recurse-submodules" over "--recursive" which
 does not say to what extent the process is allowed to recurse.
 Documentation/git-clone.txt mentions only "--recurse-submodules"
 for this exact reason, and it may not be a bad idea to start
 planning to deprecate the alias "--recursive".  
 But that obviously is a separate topic.
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/clone.c +1 −1
diff --git a/builtin/clone.c b/builtin/clone.c
index 1ad26f4d8c..54b0acbe73 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -102,10 +102,10 @@ static struct option builtin_clone_options[] = {
 		    N_("don't use local hardlinks, always copy")),
 	OPT_BOOL('s', "shared", &option_shared,
 		    N_("setup as shared repository")),
-	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
+	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	OPT_INTEGER('j', "jobs", &max_jobs,
 		    N_("number of submodules cloned in parallel")),
 	OPT_STRING(0, "template", &option_template, N_("template-directory"),

-- 
2.26.0-rc1-11-g30e9940356
Junio C Hamano· Mar 16, 2020, 21:28 UTC · re: Junio C Hamano · lore

[PATCH 1/3] parse-options: teach "git cmd -h" to show alias as alias

There is a long-standing NEEDSWORK comment that complains about inconsistency between how an aliased option ("git clone --recurse" which is the only one that currently exists) gives a help text in a usage-error message vs "git cmd -h"). Get rid of it and then make sure we say an option is an alias for another, instead of repeating the same short help text for both, which leads to "they seem to do the same---is there any subtle difference?" puzzlement to end-users.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * I couldn't easily see how to trigger usage_with_options() with
   the original options[], which is the potential cause of the
   inconsistency the NEEDSWORK comment mentions.  But if there were,
   this should take care of it.  Our test suite did not have an
   example, though.
 parse-options.c          | 9 +++------
 t/t0040-parse-options.sh | 2 +-
 2 files changed, 4 insertions(+), 7 deletions(-)
Show changes to 2 files +4 −7

parse-options.c, t/t0040-parse-options.sh

diff --git a/parse-options.c b/parse-options.c
index 63d6bab60c..c57618d537 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -648,6 +648,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 		int short_name;
 		const char *long_name;
 		const char *source;
+		struct strbuf help = STRBUF_INIT;
 		int j;
 
 		if (newopt[i].type != OPTION_ALIAS)
@@ -659,6 +660,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 
 		if (!long_name)
 			BUG("An alias must have long option name");
+		strbuf_addf(&help, _("alias of --%s"), source);
 
 		for (j = 0; j < nr; j++) {
 			const char *name = options[j].long_name;
@@ -669,15 +671,10 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			if (options[j].type == OPTION_ALIAS)
 				BUG("No please. Nested aliases are not supported.");
 
-			/*
-			 * NEEDSWORK: this is a bit inconsistent because
-			 * usage_with_options() on the original options[] will print
-			 * help string as "alias of %s" but "git cmd -h" will
-			 * print the original help string.
-			 */
 			memcpy(newopt + i, options + j, sizeof(*newopt));
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
+			newopt[i].help = strbuf_detach(&help, NULL);
 			break;
 		}
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 3483b72db4..f8178ee4e3 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -54,7 +54,7 @@ Alias
     -A, --alias-source <string>
                           get a string
     -Z, --alias-target <string>
-                          get a string
+                          alias of --alias-source
 
 EOF
 

-- 
2.26.0-rc1-11-g30e9940356
Junio C Hamano· Mar 16, 2020, 21:28 UTC · re: Junio C Hamano · lore

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

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(+)
Show changes to 2 files +2 −0

Documentation/git-log.txt, builtin/log.c

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,

-- 
2.26.0-rc1-11-g30e9940356
Eric Sunshine· Mar 16, 2020, 21:39 UTC · re: Junio C Hamano · lore

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

On Mon, Mar 16, 2020 at 5:29 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> 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· Mar 16, 2020, 22:38 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 28 quoted lines
> 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).

← back to recent threads