[PATCH 0/3] shortlog: do not accept revisions when run outside repo

[PATCH 0/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

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

[PATCH 1/3] git-shortlog.txt: reorder usages

[Martin Ågren]

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

diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
index ee6c5476c..5e35ea18a 100644
--- a/Documentation/git-shortlog.txt
+++ b/Documentation/git-shortlog.txt
@@ -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

Re: [PATCH 1/3] git-shortlog.txt: reorder usages

[Junio C Hamano]

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

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

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.

> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> index ee6c5476c..5e35ea18a 100644
> --- a/Documentation/git-shortlog.txt
> +++ b/Documentation/git-shortlog.txt
> @@ -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
>  -----------

[PATCH 2/3] shortlog: add usage-string for stdin-reading

[Martin Ågren]

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

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index e29875b84..dc4af03fc 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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

[PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

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

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index da10478f5..78c5645a9 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -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):
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index dc4af03fc..35e8c1ead 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Jonathan Nieder]

Hi,

Martin Ågren wrote:

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

Yikes.  Thanks for fixing it.

[...]
>                                                       (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.)

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

[...]
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -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
> +'

\o/

[...]
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -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) {

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

> +		error(_("no revisions can be given when running "
> +			"from outside a repository"));
> +		usage_with_options(shortlog_usage, options);
> +	}
> +

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

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

On 13 March 2018 at 20:56, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Martin Ågren wrote:
>
>>                                                       (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.)
>
> I don't follow here.  Are you saying this command should notice that
> there is input in stdin?  How would it notice?

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.

>> --- a/builtin/shortlog.c
>> +++ b/builtin/shortlog.c
>> @@ -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) {
>
> Just curious: would argc ever be 0 here?  'argc <= 1' might be clearer.

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

>> +             error(_("no revisions can be given when running "
>> +                     "from outside a repository"));
>> +             usage_with_options(shortlog_usage, options);
>> +     }
>> +
>
> 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.

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

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

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.

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

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.

> 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

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.

> Thanks and hope that helps,

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

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Jonathan Nieder]

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

>>>                                                       (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.)
>>
>> I don't follow here.  Are you saying this command should notice that
>> there is input in stdin?  How would it notice?
>
> 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.

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

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

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.

[...]
>>> +             error(_("no revisions can be given when running "
>>> +                     "from outside a repository"));
>>> +             usage_with_options(shortlog_usage, options);
>>> +     }
>>> +
>>
>> 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.
>
> 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...
[...]
> 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.

Ah, thanks.  That makes sense.

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

Sounds good to me.

Thanks,
Jonathan

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Junio C Hamano]

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

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

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

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

On 13 March 2018 at 22:46, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> 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.)
>
>     $ 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

[PATCH v2 0/3] shortlog: disallow left-over arguments outside repo

[Martin Ågren]

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

[PATCH v2 1/3] git-shortlog.txt: reorder usages

[Martin Ågren]

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

diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
index ee6c5476c1..5e35ea18ac 100644
--- a/Documentation/git-shortlog.txt
+++ b/Documentation/git-shortlog.txt
@@ -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

[PATCH v2 2/3] shortlog: add usage-string for stdin-reading

[Martin Ågren]

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

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index e29875b843..dc4af03fca 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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

[PATCH v2 3/3] shortlog: disallow left-over arguments outside repo

[Martin Ågren]

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

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index da10478f59..ff6649ed9a 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -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):
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index dc4af03fca..3a823b3128 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -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

Re: [PATCH 0/3] shortlog: do not accept revisions when run outside repo

[Jeff King]

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

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

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

Re: [PATCH 0/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

On 28 March 2018 at 10:48, Jeff King <peff@peff.net> wrote:
> On Sat, Mar 10, 2018 at 12:52:09PM +0100, Martin Ågren wrote:
>
>> 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.
>
> 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).

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

[PATCH 0/4] doc: cleaning up instances of \--

[Martin Ågren]

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

[PATCH 1/4] doc: convert \--option to --option

[Martin Ågren]

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

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6cbe462a77..b41e1329a7 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -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>`.
 
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 5b08302fc2..34410f9fca 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -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

[PATCH 2/4] doc: convert [\--] to [--]

[Martin Ågren]

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

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 5437f8b0f0..be2f10b70b 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -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
diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
index 5e35ea18ac..00a22152a2 100644
--- a/Documentation/git-shortlog.txt
+++ b/Documentation/git-shortlog.txt
@@ -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.
 +
diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index ca96c281d1..244cd01493 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -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

[PATCH 3/4] git-[short]log.txt: unify quoted standalone --

[Martin Ågren]

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

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index be2f10b70b..90761f1694 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -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`::
diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
index 00a22152a2..bc80905a8a 100644
--- a/Documentation/git-shortlog.txt
+++ b/Documentation/git-shortlog.txt
@@ -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

[PATCH 4/4] git-submodule.txt: quote usage in monospace, drop backslash

[Martin Ågren]

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

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 71c5618e82..630999f41a 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -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

Re: [PATCH 0/4] doc: cleaning up instances of \--

[Junio C Hamano]

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

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

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.

Re: [PATCH 0/4] doc: cleaning up instances of \--

[Jeff King]

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

> Martin Ågren <martin.agren@gmail.com> writes:
> 
> > 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.
> 
> 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...

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

Re: [PATCH 0/4] doc: cleaning up instances of \--

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Tue, Apr 17, 2018 at 09:15:25PM +0200, Martin Ågren wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]