shortlog: do not accept revisions when run outside repo

Patch 3 stops git shortlog from BUG-ing when it's being used slightly wrong. Patches 1 and 2 are recursive preparation. Based on maint.

Someone trying this out might notice that man git-shortlog renders "\--" as "\--", which is not wanted. (Also visible on git-scm.com...) There is quite some history around such double-slashes and compatibility with AsciiDoc-versions, so I'd rather not do a "while at it" there. Regardless of the destiny of patch 1/3, I will follow up later to address various forms of "\--" throughout the tree.

Martin Ågren (3):
git-shortlog.txt: reorder usages
shortlog: add usage-string for stdin-reading
shortlog: do not accept revisions when run outside repo

 Documentation/git-shortlog.txt | 2 +-
 t/t4201-shortlog.sh            | 5 +++++
 builtin/shortlog.c             | 9 ++++++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

2.16.2.246.ga4ee44448f

The first usage we give is the original one where, e.g., git log is piped through git shortlog. The description that follows reads the other way round, by first focusing on the general behavior, then ending with the behavior when reading from stdin.

It is also a tiny bit odd that what is probably the most common usage and the one a reader is probably looking for is not at the top of the list. Of course, it is only a two-item list, so it is not _that_ hard to find... The next commit will add the original usage to the usage string in builtin/shortlog.c, and it feels more natural to do so below the most common usage. To avoid being inconsistent, reorder these two usages here first.

---
 Documentation/git-shortlog.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ee6c5476c..5e35ea18a 100644
@@ -8,8 +8,8 @@ git-shortlog - Summarize 'git log' output
 SYNOPSIS
 --------
 [verse]
-git log --pretty=short | 'git shortlog' [<options>]
 'git shortlog' [<options>] [<revision range>] [[\--] <path>...]
+git log --pretty=short | 'git shortlog' [<options>]
 
 DESCRIPTION
 -----------

2.16.2.246.ga4ee44448f

The result looks a lot more useful than without the patch. I think the current ordering is merely a historical accident made in 2006.

Will queue; thanks.

This has been missing since we learned to print usage, way back in 4e27fb06f (add commit count options to git-shortlog, 2006-10-06).

While at it, drop the [] around "<path>...". This matches git log -h and Documentation/git-{short}log.txt. It formally makes it look like we do not allow git shortlog --, but we gain readability and consistency.

---
 builtin/shortlog.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index e29875b84..dc4af03fc 100644
@@ -11,7 +11,8 @@
 #include "parse-options.h"
 
 static char const * const shortlog_usage[] = {
-	N_("git shortlog [<options>] [<revision-range>] [[--] [<path>...]]"),
+	N_("git shortlog [<options>] [<revision-range>] [[--] <path>...]"),
+	N_("git log --pretty=short | git shortlog [<options>]"),
 	NULL
 };
 

2.16.2.246.ga4ee44448f

If we are outside a repo and have any arguments left after option-parsing, setup_revisions() will try to do its job and something like this will happen:

$ git shortlog v2.16.0..
BUG: environment.c:183: git environment hasn't been setup
Aborted (core dumped)

The usage is wrong, but we could obviously handle this better. Note that commit abe549e179 (shortlog: do not require to run from inside a git repository, 2008-03-14) explicitly enabled git shortlog to run from outside a repo, since we do not need a repo for parsing data from stdin.

Disallow left-over arguments when run from outside a repo. Another approach would be to disallow them when reading from stdin. However, our logic is currently the other way round: we check the number of revisions in order to decide whether we should read from stdin. (So yes, after this patch, we will still silently ignore stdin for confused usage such as git log v2.15.0.. | git shortlog v2.16.0... But at least that does not crash.)

---
 t/t4201-shortlog.sh | 5 +++++
 builtin/shortlog.c  | 6 ++++++
 2 files changed, 11 insertions(+)

index da10478f5..78c5645a9 100755
@@ -127,6 +127,11 @@ test_expect_success !MINGW 'shortlog can read --format=raw output' '
 	test_cmp expect out
 '
 
+test_expect_success 'shortlog from non-git directory refuses revisions' '
+	test_must_fail env GIT_DIR=non-existing git shortlog HEAD 2>out &&
+	test_i18ngrep "no revisions can be given" out
+'
+
 test_expect_success 'shortlog should add newline when input line matches wraplen' '
 	cat >expect <<\EOF &&
 A U Thor (2):

index dc4af03fc..35e8c1ead 100644
@@ -293,6 +293,12 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 parse_done:
 	argc = parse_options_end(&ctx);
 
+	if (nongit && argc != 1) {
+		error(_("no revisions can be given when running "
+			"from outside a repository"));
+		usage_with_options(shortlog_usage, options);
+	}
+
 	if (setup_revisions(argc, argv, &rev, NULL) != 1) {
 		error(_("unrecognized argument: %s"), argv[1]);
 		usage_with_options(shortlog_usage, options);

2.16.2.246.ga4ee44448f

Hi,

Yikes. Thanks for fixing it.

[...]

I don't follow here. Are you saying this command should notice that there is input in stdin? How would it notice?

[...]

	error: no revisions can be given when running from outside a repository
	usage: ...

	error(_("no remote specified"));
	usage_with_options(builtin_remote_setbranches_usage, options);

	die(_("'%s' cannot be used with %s"), "--merge", "--patch");

I have no idea how it would notice (portably!) and the gain seems minimal. I added this to keep the reader from wondering "but wait a minute, doesn't that mean that we fail to catch this bad usage if we're in a repo?". So my answer would be "yep, but it's not a huge problem". Of course, my attempt to pre-emptively answer a question only provoked another one. :-) I could phrase this better.

Hmm, good point. It "shouldn't" be 0, but I guess it's better to be safe than sorry. (We seem to have both constructs, in various places.)

I can see an argument for "dumping the usage makes the error harder than necessary to find". I mainly went for consistency. This ties into your other observations below: what little consistency do we have and in which direction do we want to push it...

The other two approaches ("die" and "error and usage") can be argued for, but this one ("give usage") just seems wrong to me. I haven't looked for such a place in the code, and maybe they're "obvious", but it seems odd to just give the usage without any sort of hint about what was wrong.

I think it would be a larger project to make these consistent. The one I'm adding here is at least consistent with the other one in this file.

Some grepping suggests we do not usually name the command ("shortlog ..."), perhaps to play well with aliasing, nor do we use "such <syntax>" very often, but it does happen. Quoting and allowing for options might make this more correct, but perhaps less readable: "'shortlog [...] <revs>' can only ...". Slightly better than what I had, "revisions can only be given inside a git repository" would avoid some negating.

It does indeed. I'll give this another 24h and see what I come up with. I believe it will end up in a change to "<= 1", an improved error message and a clearer last few words in the commit message.

Martin

Ah, I think I see what I was missing. Let me look again at the whole paragraph in context:

[...]

How about something like this?

Disallow left-over arguments when run from outside a repo. This way, such invalid usage is diagnosed correctly:

$ git shortlog v2.16.0..
error: [...]
[...]

while still permitting the piped form:

$ git -C ~/src/git log --pretty=short | git shortlog A Large Angry SCM (15):
[...]

This cannot catch an incorrect usage that combines the piped and <revs> forms:

$ git log --pretty=short | git shortlog v2.16.0.. Alban Gruin (1)
[...]

but (1) the operating system does not provide a straightforward way to detect that and (2) at least it doesn't crash.

That detail is mostly irrelevant to what the patch does, though. I wouldn't expect git to be able to diagnose that third variant anyway. If we want to make the command less error-prone, I think a good path forward would be to introduce an explicit --stdin option. So I'd be tempted to go with the short and sweet:

Disallow left-over arguments when run from outside a repo.

[...]

[...]

Ah, thanks. That makes sense.

Sounds good to me.

Thanks,
Jonathan

    $ git log -p | git shortlog Documentation/

is also a nonsense request. When outside a repository, i.e.

    $ git -C $path_to_repo | git shortlog Documentation/

is not giving any revisions, so the error message should not say "no revisions can be given"---a nitpicky bug reporter would say "I gave no revisions, why are you complaining to me?"

Good point. I will see if I can make this similar to other places. Maybe something like "too many arguments given outside repository". Thanks.

Martin

This is v2 of my attempt at stopping shortlog from BUG-ing when it is used incorrectly outside a repo. Thanks Jonathan and Junio for helpful comments.

Patches 1 and 2 are identical to pu. The error message in patch 3 is now more general. The error condition on the other hand is a bit more specific, "argc > 1", to better match the intention and commit message.

Martin

Martin Ågren (3):
git-shortlog.txt: reorder usages
shortlog: add usage-string for stdin-reading
shortlog: disallow left-over arguments when run outside repo

 Documentation/git-shortlog.txt | 2 +-
 t/t4201-shortlog.sh            | 5 +++++
 builtin/shortlog.c             | 8 +++++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

2.16.2.246.ga4ee44448f

The first usage we give is the original one where, e.g., git log is piped through git shortlog. The description that follows reads the other way round, by first focusing on the general behavior, then ending with the behavior when reading from stdin.

It is also a tiny bit odd that what is probably the most common usage and the one a reader is probably looking for is not at the top of the list. Of course, it is only a two-item list, so it is not _that_ hard to find... The next commit will add the original usage to the usage string in builtin/shortlog.c, and it feels more natural to do so below the most common usage. To avoid being inconsistent, reorder these two usages here first.

---
 Documentation/git-shortlog.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ee6c5476c1..5e35ea18ac 100644
@@ -8,8 +8,8 @@ git-shortlog - Summarize 'git log' output
 SYNOPSIS
 --------
 [verse]
-git log --pretty=short | 'git shortlog' [<options>]
 'git shortlog' [<options>] [<revision range>] [[\--] <path>...]
+git log --pretty=short | 'git shortlog' [<options>]
 
 DESCRIPTION
 -----------

2.16.2.246.ga4ee44448f

This has been missing since we learned to print usage, way back in 4e27fb06f (add commit count options to git-shortlog, 2006-10-06).

While at it, drop the [] around "<path>...". This matches git log -h and Documentation/git-{short}log.txt. It formally makes it look like we do not allow git shortlog --, but we gain readability and consistency.

---
 builtin/shortlog.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index e29875b843..dc4af03fca 100644
@@ -11,7 +11,8 @@
 #include "parse-options.h"
 
 static char const * const shortlog_usage[] = {
-	N_("git shortlog [<options>] [<revision-range>] [[--] [<path>...]]"),
+	N_("git shortlog [<options>] [<revision-range>] [[--] <path>...]"),
+	N_("git log --pretty=short | git shortlog [<options>]"),
 	NULL
 };
 

2.16.2.246.ga4ee44448f

If we are outside a repo and have any arguments left after option-parsing, setup_revisions() will try to do its job and something like this will happen:

$ git shortlog v2.16.0..
BUG: environment.c:183: git environment hasn't been setup
Aborted (core dumped)

The usage is wrong, but we could obviously handle this better. Note that commit abe549e179 (shortlog: do not require to run from inside a git repository, 2008-03-14) explicitly enabled git shortlog to run from outside a repo, since we do not need a repo for parsing data from stdin.

Disallow left-over arguments when run from outside a repo.

---
 t/t4201-shortlog.sh | 5 +++++
 builtin/shortlog.c  | 5 +++++
 2 files changed, 10 insertions(+)

index da10478f59..ff6649ed9a 100755
@@ -127,6 +127,11 @@ test_expect_success !MINGW 'shortlog can read --format=raw output' '
 	test_cmp expect out
 '
 
+test_expect_success 'shortlog from non-git directory refuses extra arguments' '
+	test_must_fail env GIT_DIR=non-existing git shortlog foo 2>out &&
+	test_i18ngrep "too many arguments" out
+'
+
 test_expect_success 'shortlog should add newline when input line matches wraplen' '
 	cat >expect <<\EOF &&
 A U Thor (2):

index dc4af03fca..3a823b3128 100644
@@ -293,6 +293,11 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 parse_done:
 	argc = parse_options_end(&ctx);
 
+	if (nongit && argc > 1) {
+		error(_("too many arguments given outside repository"));
+		usage_with_options(shortlog_usage, options);
+	}
+
 	if (setup_revisions(argc, argv, &rev, NULL) != 1) {
 		error(_("unrecognized argument: %s"), argv[1]);
 		usage_with_options(shortlog_usage, options);

2.16.2.246.ga4ee44448f

I didn't see any follow-up here, but in case you were delaying because the history search seemed boring: dropping the backslash is the right thing to do. See the discussion in 1c262bb7b2 (doc: convert \--option to --option, 2015-05-13).

-Peff

Thanks for pinging and thanks for the pointer. That commit is indeed helpful and I am referencing it in a local topic, which I will submit once ma/shortlog-revparse hits master.

Martin

This is a patch series to convert \-- to -- in our documentation. The first patch is a reiteration of 1c262bb7b2 (doc: convert \--option to --option, 2015-05-13) to fix some instances that have appeared since. The other three patches deal with standalone "\--" which we can't always turn into "--" since it can be rendered as an em dash.

Martin

Martin Ågren (4):
doc: convert \--option to --option
doc: convert [\--] to [--]
git-[short]log.txt: unify quoted standalone -- git-submodule.txt: quote usage in monospace, drop backslash

 Documentation/git-format-patch.txt | 2 +-
 Documentation/git-log.txt          | 8 ++++----
 Documentation/git-push.txt         | 2 +-
 Documentation/git-shortlog.txt     | 6 +++---
 Documentation/git-submodule.txt    | 4 ++--
 Documentation/gitk.txt             | 2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

2.17.0.252.gfe0a9eaf31

Rather than using a backslash in \--foo, with or without ''-quoting, write --foo for better rendering. As explained in commit 1c262bb7b (doc: convert \--option to --option, 2015-05-13), the backslash is not needed for the versions of AsciiDoc that we support, but is rendered literally by Asciidoctor.

---
 Documentation/git-format-patch.txt | 2 +-
 Documentation/git-push.txt         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 6cbe462a77..b41e1329a7 100644
@@ -47,7 +47,7 @@ There are two ways to specify which commits to operate on.
 
 The first rule takes precedence in the case of a single <commit>.  To
 apply the second rule, i.e., format everything since the beginning of
-history up until <commit>, use the '\--root' option: `git format-patch
+history up until <commit>, use the `--root` option: `git format-patch
 --root <commit>`.  If you want to format only <commit> itself, you
 can do this with `git format-patch -1 <commit>`.
 

index 5b08302fc2..34410f9fca 100644
@@ -300,7 +300,7 @@ origin +master` to force a push to the `master` branch). See the
 	These options are passed to linkgit:git-send-pack[1]. A thin transfer
 	significantly reduces the amount of sent data when the sender and
 	receiver share many of the same objects in common. The default is
-	\--thin.
+	`--thin`.
 
 -q::
 --quiet::

2.17.0.252.gfe0a9eaf31

Commit 1c262bb7b (doc: convert \--option to --option, 2015-05-13) explains that we used to need to write \--option to play well with older versions of AsciiDoc, but that we do not support such versions anymore anyway, and that Asciidoctor literally renders \--.

With [\--], which is used to denote the optional separator between revisions and paths, Asciidoctor renders the backslash literally. Change all [\--] to [--]. This changes nothing for AsciiDoc version 8.6.9, but is an improvement for Asciidoctor version 1.5.4.

We use double-dashes in several list entries (\--::). In my testing, it appears that we do need to use the backslash there, so leave those.

---
 Documentation/git-log.txt      | 4 ++--
 Documentation/git-shortlog.txt | 4 ++--
 Documentation/gitk.txt         | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

index 5437f8b0f0..be2f10b70b 100644
@@ -9,7 +9,7 @@ git-log - Show commit logs
 SYNOPSIS
 --------
 [verse]
-'git log' [<options>] [<revision range>] [[\--] <path>...]
+'git log' [<options>] [<revision range>] [[--] <path>...]
 
 DESCRIPTION
 -----------

@@ -90,7 +90,7 @@ include::line-range-format.txt[]
 	ways to spell <revision range>, see the 'Specifying Ranges'
 	section of linkgit:gitrevisions[7].
 
-[\--] <path>...::
+[--] <path>...::
 	Show only commits that are enough to explain how the files
 	that match the specified paths came to be.  See 'History
 	Simplification' below for details and other simplification

index 5e35ea18ac..00a22152a2 100644
@@ -8,7 +8,7 @@ git-shortlog - Summarize 'git log' output
 SYNOPSIS
 --------
 [verse]
-'git shortlog' [<options>] [<revision range>] [[\--] <path>...]
+'git shortlog' [<options>] [<revision range>] [[--] <path>...]
 git log --pretty=short | 'git shortlog' [<options>]
 
 DESCRIPTION

@@ -69,7 +69,7 @@ them.
 	ways to spell <revision range>, see the "Specifying Ranges"
 	section of linkgit:gitrevisions[7].
 
-[\--] <path>...::
+[--] <path>...::
 	Consider only commits that are enough to explain how the files
 	that match the specified paths came to be.
 +

index ca96c281d1..244cd01493 100644
@@ -8,7 +8,7 @@ gitk - The Git repository browser
 SYNOPSIS
 --------
 [verse]
-'gitk' [<options>] [<revision range>] [\--] [<path>...]
+'gitk' [<options>] [<revision range>] [--] [<path>...]
 
 DESCRIPTION
 -----------

2.17.0.252.gfe0a9eaf31

In git-log.txt, we have an instance of \--, which is known to sometimes render badly. This one is even worse than normal though, since ``\-- '' (with or without that trailing space) appears to be entirely broken, both in HTML and manpages, both with AsciiDoc (version 8.6.9) and Asciidoctor (version 1.5.4).

Further down in git-log.txt we have a ``--'', which renders good. In git-shortlog.txt, we use "\-- " (including the quotes and the space), which happens to look fairly good. I failed to find any other similar instances. So all in all, we quote a double-dash in three different places and do it differently each time, with various degrees of success.

Switch all of these to --. This sets the double-dash in monospace and matches what we usually do with example command line usages and options. Note that we drop the trailing space as well, since -- does not render well. These should still be clear enough since just a few lines above each instance, the space is clearly visible in a longer context.

---
 Documentation/git-log.txt      | 4 ++--
 Documentation/git-shortlog.txt | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

index be2f10b70b..90761f1694 100644
@@ -96,7 +96,7 @@ include::line-range-format.txt[]
 	Simplification' below for details and other simplification
 	modes.
 +
-Paths may need to be prefixed with ``\-- '' to separate them from
+Paths may need to be prefixed with `--` to separate them from
 options or the revision range, when confusion arises.
 
 include::rev-list-options.txt[]

@@ -125,7 +125,7 @@ EXAMPLES
 `git log --since="2 weeks ago" -- gitk`::
 
 	Show the changes during the last two weeks to the file 'gitk'.
-	The ``--'' is necessary to avoid confusion with the *branch* named
+	The `--` is necessary to avoid confusion with the *branch* named
 	'gitk'
 
 `git log --name-status release..test`::

index 00a22152a2..bc80905a8a 100644
@@ -73,7 +73,7 @@ them.
 	Consider only commits that are enough to explain how the files
 	that match the specified paths came to be.
 +
-Paths may need to be prefixed with "\-- " to separate them from
+Paths may need to be prefixed with `--` to separate them from
 options or the revision range, when confusion arises.
 
 MAPPING AUTHORS

2.17.0.252.gfe0a9eaf31

We tend to quote command line examples using `` to set them in a monospace font. The immediate motivation for this patch is to get rid of another instance of \--. As noted in the previous commits, \-- has a tendency of rendering badly. Here, it renders ok (at least with AsciiDoc 8.6.9 and Asciidoctor 1.5.4), but by getting rid of this instance, we reduce the chances of \-- cropping up in places where it matters more.

---
 Documentation/git-submodule.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 71c5618e82..630999f41a 100644
@@ -213,8 +213,8 @@ sync [--recursive] [--] [<path>...]::
 	submodule URLs change upstream and you need to update your local
 	repositories accordingly.
 +
-"git submodule sync" synchronizes all submodules while
-"git submodule sync \-- A" synchronizes submodule "A" only.
+`git submodule sync` synchronizes all submodules while
+`git submodule sync -- A` synchronizes submodule "A" only.
 +
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.

2.17.0.252.gfe0a9eaf31

All looked sensible. As you mentioned in [2/4], "\--::" that is part of an enumulation appear in documentation for about a dozen commands after the series, but I do not think we can avoid it.

One thing that makes me wonder related to these patches is if a newer AsciiDoc we assume lets us do without {litdd} macro. This series and our earlier effort like 1c262bb7 ("doc: convert \--option to --option", 2015-05-13) mentions that "\--word" is less pleasant on the eyes than "--word", but the ugliness "two{litdd}words" has over "two--words" is far worse than that, so...

Thanks, will queue.

I think many cases that use {litdd} would be better off using literal backticks anyway (e.g., git-add.txt mentions the filename git-add--interactive.perl).

There are certainly a few that can't, though (e.g., config.txt uses linkgit:git-web{litdd}browse[1]). I agree that "\--" is less ugly there (and seems to work on my modern asciidoc). There's some history on the litdd versus "\--" choice in 565e135a1e (Documentation: quote double-dash for AsciiDoc, 2011-06-29). That in turn references the 2839478774 (Work around em-dash handling in newer AsciiDoc, 2010-08-23), but I wouldn't be surprised if all of that is now obsolete with our AsciiDoc 8+ requirement.

-Peff

PS Late review, I know, but the patches look good to me. :)

This series looked sane to me as well. I appreciate the work to keep our documentation working in both AsciiDoc and Asciidoctor.

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204