patch, 3 partsshortlog: 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(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-shortlog.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Martin Ågren <martin.agren@gmail.com> writes:

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/shortlog.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t4201-shortlog.sh | 5 +++++
 builtin/shortlog.c  | 6 ++++++
 2 files changed, 11 insertions(+)

Hi,

Martin Ågren wrote:

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?

[...]

\o/

[...]

Just curious: would argc ever be 0 here?  'argc <= 1' might be clearer.

The error message is

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

Do we need to dump usage here? I wonder if a simple die() call would be easier for the user to act on.

Not about this patch: I was a little surprised to see 'error:' instead of 'usage:' or 'fatal:'. It turns out git is pretty inconsistent about that: e.g. there is

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

Some other callers just use usage_with_options without describing the error. check-attr has a private error_with_usage() helper to implement the error() followed by usage_with_options() idiom. Most callers just use die(), like

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

Documentation/technical/api-error-handling.txt says

 - `usage` is for errors in command line usage.  After printing its
   message, it exits with status 129.  (See also `usage_with_options`
   in the link:api-parse-options.html[parse-options API].)

which is not prescriptive enough to help.

Separate from that, I wonder if the error message can be made a little shorter and clearer. E.g.

	fatal: shortlog <revs> can only be used inside a git repository

Thanks and hope that helps, Jonathan

On 13 March 2018 at 20:56, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

Martin Ågren wrote:

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

Martin Ågren <martin.agren@gmail.com> writes:

    $ 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?"

On 13 March 2018 at 22:46, Junio C Hamano <gitster@pobox.com> wrote:

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(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-shortlog.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/shortlog.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t4201-shortlog.sh | 5 +++++
 builtin/shortlog.c  | 5 +++++
 2 files changed, 10 insertions(+)

On Sat, Mar 10, 2018 at 12:52:09PM +0100, Martin Ågren wrote:

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

On 28 March 2018 at 10:48, Jeff King <peff@peff.net> wrote:

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(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-format-patch.txt | 2 +-
 Documentation/git-push.txt         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-log.txt      | 4 ++--
 Documentation/git-shortlog.txt | 4 ++--
 Documentation/gitk.txt         | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-log.txt      | 4 ++--
 Documentation/git-shortlog.txt | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-submodule.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Martin Ågren <martin.agren@gmail.com> writes:

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.

On Wed, Apr 18, 2018 at 01:24:55PM +0900, Junio C Hamano wrote:

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. :)

On Tue, Apr 17, 2018 at 09:15:25PM +0200, Martin Ågren wrote:

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