patch, 5 partsDocumentation/shortlog improvements

Hi,

I was going through the shortlog documentation and was saddened to see that it was inaccurate and inconsistent with the log documentation. I use shortlog quite a lot, and like it very much. So, here's a small series fixing some problems.

[3/5] and [4/5] came out of my desire to copy out parts of git-log.txt into git-shortlog.txt (I had to fix git-log.txt first).

Thanks.

Ramkumar Ramachandra (5):
  git-shortlog.txt: remove (-h|--help) from OPTIONS
  builtin/shortlog.c: make usage string consistent with log
  git-log.txt: fix description of <since>..<until>
  git-log.txt: rewrite note on why "--" may be required
  git-shortlog.txt: make SYNOPSIS match log, update OPTIONS

 Documentation/git-log.txt      | 10 +++++-----
 Documentation/git-shortlog.txt | 23 ++++++++++++++++++-----
 builtin/shortlog.c             |  4 +---
 3 files changed, 24 insertions(+), 13 deletions(-)

To be consistent with the documentation of all the other commands, remove (-h|--help) from the OPTIONS section.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-shortlog.txt | 4 ----
 1 file changed, 4 deletions(-)

"--" is used to separate pathspecs from the rev specs, and not rev specs from the options, as the shortlog_usage string currently indicates. In correcting this usage string, make it consistent with the log_usage string.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/shortlog.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

First, <since> and <until> are ways to specify "revisions", not "commits", as gitrevisions.txt would indicate. Second, '<since>..<until>' is simply indicative of how users would normally want to specify the rev spec: it need not conform to this form, and can take any form that gitrevisions.txt lists. A 'git log :/quuxery' is perfectly valid, for instance. Make these two corrections to the description of '<since>..<until>' under OPTIONS.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-log.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Ramkumar Ramachandra wrote:

What's the difference between a revision and a commit? The definition in gitglossary(7) only confuses me.

Jonathan Nieder wrote:

What's so confusing about it?

commit:: As a noun: A single point in the Git history; the entire history of a project is represented as a set of interrelated commits. The word "commit" is often used by Git in the same places other revision control systems use the words "revision" or "version". Also used as a short hand for commit object.

revision:: A particular state of files and directories which was stored in the object database. It is referenced by a commit object.

-- 8< --

There a reference to "revision" in the "commit" description which might be misleading.

Otherwise, these are quite accurate. master~3:README is a valid revision, but not a commit, for instance. Another problem with the word commit is that it doesn't encapsulate ranges like 'master~3..HEAD'. It would probably help to add the terms "commitish" and "treeish" to the glossary to refer to "rev specs" (those in revisions.txt) that resolve to commits or trees. I'll give you two simple examples:

1. In 'git show master:README', master is a treeish and master:README
is a blob.  If we were to say 'git blame master: -- README', it would
error out because master: is not a commitish, but a treeish.

2. In 'git log master~3.. -- README', master~3.. is a commitish.  In
general, log can take only rev specs that resolve to a commits.
Although 'git log master:' doesn't error out, it doesn't make any
sense either.  Perhaps we should tighten it?

Ramkumar Ramachandra wrote:

[...]

So a revision is a tree? ("one of the things pointed to by a commit object, representing a particular state of files and directories")

Jonathan Nieder <jrnieder@gmail.com> writes:

They are the same, but I think it is irrelevant. We do not want revision or commit there, anyway. We want a revision range there. This is "git log" not "git show".

Ramkumar Ramachandra <artagnon@gmail.com> writes:

A bigger issue is if it is a good idea to single out A..B like this in the enumeration of options and parameters. We are listing this among the optoins and pathspec, so this is where we _ought_ to be saying that you can specify a revision range on the command line. We use A..B only because it is the most commonly used form, asking e.g. "show me the commit since v1.7.12 up to v1.8.0", but this existing headline makes it as if it is the _only_ form, which is somewhat misleading.

It might be better _not_ to say <since>..<until> here and on the synopsis part, replace it with <revision range>, and update the text to mention <since>..<until> as a more concrete "v1.0..v2.0" example. Perhaps like

    <revision range>::

	Show only commits in the specified revision range.  When no
	<revision range> is specified, defaults to "HEAD" (i.e. the
	whole history leading to the current commit).  v1.0..v2.0
	asks for commits since the commit tagged as version 1.0 up
	to the commit tagged as version 2.0.  For a more complete
	list of ways to spell this parameter, see "Specifying
	Ranges" section of linkgit:gitrevisions[7].

or something.

Documentation/gitrevisions.txt lists <rev1>..<rev2>, but does not mention that missing <rev1> (or <rev2>) defaults to HEAD, which is worth adding there with or without the above suggested update.

In its current form, the note talks about separating options from "branch names" and "refnames" in the same sentence. This is entirely inaccurate, as the rev spec need not be a set of branch names or ref names. Rewrite it to use the word "revisions".

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-log.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

I think "branch names" was an attempt to make it more newbie friendly by sacrificing technical accuracy. With the suggested update (see the review for the previous one), it would be easier to read if this part said "options or the revision range".

Two other things I noticed with the current text are:

 * As the synopsis section shows, on the command line, --options
   come first, then revision range and then pathspec.  The order of
   the description should follow that as well.  The current text
   shows <since>..<until> at the very beginning, which is wrong.

 * The part with the new "-L" option seems to be throwing the
   overall formatting off.  Its second paragraph "<start> and <end>
   can take..." is not indented to the same level as its first
   paragraph "Trace the evolution of...", and the following items,
   like "--full-line-diff" and "[--] <pathspec>..." are indented one
   level too deeply.

Junio C Hamano wrote:

Why does it have to be a range? It might well be a list of revisions, so I'm tempted to stick with the word "revisions". As I wrote in the previous email, I think "revision" can be referenced by any one of the syntaxes specified in revisions.txt (this includes a "committish range").

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Study "Specifying Ranges" section of gitrevisions and come back, perhaps?

A list of revisions is merely a way to specify "revision range" that are reachable from any of these revisions listed. "log A B" won't stop by just showing A and B (that would be "show A B"), but will list those that are reachable from A B, so in the context of discussing the arguments given to "git log" command, "A B" is still "revision range".

Junio C Hamano wrote:

... and what about 'git log HEAD^!'? Is that a range? What about 'git log HEAD:README' (hint: it doesn't error out)? I would argue that "A B" is not inherently a revision range, but rather two revisions. It's upto different commands to interpret it differently.

Then again, in the context of log, we just want ranges (also considering we had <since>..<until> for this long, and I didn't even object to it). So I'll go with your "revision range", but I won't claim that it's technically accurate.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Of course it _is_ a range; didn't you read it in the "specifying ranges" section? It is a short-hand for "include HEAD, exclude its parents", and you can further combine it with other starting points.

Sounds like a bug, if it doesn't.

Patches welcome---I suspect that it can be solved the same way as the recent cherry-pick patch by Miklos.

That is why I said this is "git log", not "git show".

Junio C Hamano wrote:

It's not as pressing as the glaring Documentation inaccuracies, so I'll queue the task.

To exclude this case, I would say the most technically accurate description of what 'git log' takes is a "committish range" (basically a "revision range" that resolves to commits).

Ramkumar Ramachandra wrote:

What is a revision range that doesn't resolve to commits? Am I wrong in thinking revision is nothing more than a synonym for commit?

When gitrevisions(7) says "A revision parameter <rev> typically, but not necessarily, names a commit object", I suspect it is residue from 3a45f625 trying to apologize for the extended SHA1 syntax parser being called "git rev-parse" instead of "git object-name-parse".

Jonathan Nieder wrote:

Yes. If master@{3}~2:README isn't a revision, what is it? And it fits into the Specifying Revisions section quite snugly: I see no reason to mangle the meaning of "revision".

Ramkumar Ramachandra wrote:

An extended SHA1 expression referring to a blob.

Jonathan Nieder <jrnieder@gmail.com> writes:

 - A revision range is a slice of history.  There is no need for
   "committish revision range" as there is no other kinds of ranges.

 - A revision should be a synonym to a committish, as glossary
   defines.  We historically used "revision" more or less
   interchangeably with "object name", especially "an extended SHA-1
   expression that is used as an object name", to mean whatever
   get_sha1() can grok down to a single object name.  This must be
   rectified to avoid causing confusion to new readers of our
   documentation.

 - Documentation/revisions.txt needs to be cleaned up.  It lists
   "specifying revisions" and "specifying ranges", which is a good
   start primarily because some notations used in ranges are not
   revisions (e.g. "^C"), but it should have three (not two) sections.

   They are: "specifying revisions", "specifying object names", and
   "specifying ranges".  Move non committish specification such as
   <revision>:<path> from the current "specifying revisions" section
   and make the new "object names" section.  The "ranges" section is
   already written in terms of revisions, and does not need any
   update, once we clarify the definition of a "revision" as
   "committish".

Junio C Hamano wrote:

For the record, 'git rev-parse master:README..HEAD@{3}~2' works just fine. I don't know whether you want to consider it "sensible" or not, but the current revisions.txt is consistent with this.

The question is: who is the authority on this? The combination of rev-parse, rev-list, and revisions.txt, or gitglossary.txt.

Personally, I don't like giving commit objects a special status, and like things just the way they currently are. Why do you want to separate "revisions" (which are really just "extended SHA-1 expressions" that incidentally resolve to commit objects) and "extended SHA-1 expressions that resolve to objects other than commits"? Is the current hand-wavy unclear gitglossary.txt the only basis for your argument?

Ramkumar Ramachandra wrote:

rev-list errors out though:

  error: Object 15bfea is a blob, not a commit
  fatal: Invalid revision range master:README..HEAD@{3}~2

Hence my suggestion to change the meaning of what a valid "revision range" is (using rev-list as the guideline) , and be done with it.

Ramkumar Ramachandra wrote:

No one I've met actually uses the word "revisions" to mean anything other than the English-language meaning of "revisions", which in a Git context is "commits". There's value in matching common usage.

Junio C Hamano <gitster@pobox.com> writes:

A bit more specifically, there is a reason why we list "Specifying Revisions" and "Specifying Ranges" separately in that manual page.

I think you are trying explain "git log --short A B ^C" as if it takes "--short" (which is an option), "A", "B", and "^C" (all of which are revisions). And I am saying that is wrong.

It is "--short" (which is an option) and "A B ^C" (which is a revision range).

"git log --short A" is taking "--short" (an option) and "A". This "A" is still a revision range and not a single revision. It is a single commit that is used to name a revision range which is the entire history behind the commit "A".

Junio C Hamano wrote:

Got it.  Your explanation makes more sense.

Junio C Hamano <gitster@pobox.com> writes:

This is a minimalistic patch to fix the formatting. I removed the extra sentence after the enumeration and moved it to the end of the main text, but somebody may have a better idea to persuade AsciiDoc to format it in a more reasonable way while keeping the sentence there.

-- >8 --
Subject: line-log: fix documentation formatting

The second paragraph of the added description for the -L option "<start> and <end> can take one of these forms:", and the list of forms that follow the headline, were indented one level too short, due to the missing "+" to signal that the next paragraph continues the previous one.

Also "You can specify this option more than once" is about the -L option, not about its various forms of starting and ending points. Move it to the end of the main text.

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

Junio C Hamano <gitster@pobox.com> writes:

Sorry for being a bit late to this. I think it's a good solution; putting "You can specify this option more than once" after all the other text was probably worse because it gets lost down there.

As for

That's just stale and not currently implemented. Sigh. We should remove it.

-- >8 --
Subject: [PATCH] git-log(1): remove --full-line-diff description

This option is a remnant of an earlier log -L version, and not currently implemented. Remove it until (if at all) it is implemented again.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 Documentation/git-log.txt | 4 ----
 1 file changed, 4 deletions(-)

Thomas Rast <trast@inf.ethz.ch> writes:

BTW, I generated this with your jc/format-patch, but it stopped working after fc/send-email-annotate made it into next; I need this on top. Am I missing something?

-- >8 --
Subject: [PATCH] FIXUP jc/format-patch: adapt for fc/send-email-annotate

2a4c260 (format-patch: add format.coverLetter configuration variable,
2013-04-07) changed the coverletter variable to -1 by default, so the
die("... incompatible") always triggers.  Test if it is >0 instead.
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Thomas Rast <trast@inf.ethz.ch> writes:

No, the topic has been stalled and left behind and needs to be rebased on top of that other topic with your patch. Thanks.

It also needs a lot more work to de-mime its output to be eligible for 'next', though.

There are broadly two problems with the current SYNOPSIS. First, it completely omits the detail that paths can be specified. Second, it attempts to list all the options: this is futile as, in addition to the options unique to it, it accepts all the options that git-rev-list accepts. In fixing these problems, make the SYNOPSIS consistent with that in git-log.txt. Also add the corresponding sections to OPTIONS.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-shortlog.txt | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

The same "revision range" comment, and also that this should be after all options and before "-- <pathspec>", apply to this.

While it is true that this page is missing many options, including rev-list-options.txt as-is is more harmful than helpful and the patch needs more work (hint: with "ifdef::git-shortlog[]" sprinkled in rev-list-options.txt).

Some of the earlier one in rev-list-options are fine, but some others are inappropriate in the context of shortlog. For example, --grep=<pattern>, while it would internally work as it is told to do, is unlikely to be clear how well/correctly worked when viewing the end result. So are many others, such as --all-match, --walk-reflogs, --sparse, etc. Some e.g. --topo-order, do not make sense at all.

I am not sure what the command would do if you give --stdin; the command wants to read from the standard input to operate as a filter for "git log" output, but the "--stdin" from rev-list family wants to read revision range specifiers from the standard input. I do not think it is something any sane user would want to do.

Junio C Hamano wrote:

I was just saying: I don't want to introduce new terms either. I think revisions.txt needs to be fixed. Under the Specifying Ranges section:

'<rev>'::
	Include commits that are reachable from (i.e. ancestors of)
	<rev>.

This doesn't make sense because <rev> might as well refer to a blob*. So this should say s/<rev>/<rev> that names a commit object/.

* The first line in the Specifying Revisions section: A revision
parameter '<rev>' typically, but not necessarily, names a commit
object.

Ramkumar Ramachandra wrote:

In other words, my fix is to change the meaning of "revision range" to not literally mean "range of revisions", but rather "range of (revisions that are commits)". I think it is more sensible than Jonathan's fix which breaks too many existing things; most significantly, the names rev-parse/rev-list.

The current definition of 'revision' sounds like it is saying that a revision is a tree object. In reality it is just a commit.

This should be especially useful for people used to other revision control systems trying to see how familiar concepts translate into git terms.

Reported-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

[...]

Thanks.

 Documentation/glossary-content.txt | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

I think the existing glossary text is just confusing (it does wants to refer to a commit (noun) as a concept, as opposed to its concrete realization, i.e. a commit object, and tries to do so but does a bad job).

Your update is an improvement, but there may need a note that says that various web pages and documents, especially historical ones [*1*], may loosely use this word to refer to any single object that is not necessarily a commit at times.

[Footnote]

*1* The "rev-parse" command started as a tool to parse "revisions" aka "commit", "revision range" e.g. "A..B" and non-revisions (pathspecs) and flags. It was primarily designed as a way to sift arguments given to "git mylog -p A..B Makefile", so that a scripted "mylog" can turn it into a pipeline that feeds "git rev-list A..B Makefile" output as the input to "git diff-tree -p --stdin Makefile".

This was back when we did not even have non-committish extended SHA-1 object name notation. From the very start, "rev" in the name "rev-parse" did not even mean that it is limited to "revision" aka "commit". Later we added tree:path form of extended SHA-1 notation and "rev-parse --verify $it" has become its primary usage, while its role as argument sifter has diminished as we made "log" a built-in, not a pipeline. Around that time, we loosely started calling anything that get_sha1() can turn into an object name a "revision".

Also 99.9% of the time people use it to parse committish in real life.

For these reasons, I do not think it is wise to rename "rev-parse" to "object-name-parse". "rev" in "rev-parse" may be related to "revisions", and while it technically can operate on "revisions" (in the colloquial "object name" sense), it is not limited to revisions. And when it is used for revisions it mostly is used for "revisions" (in the "nothing but committish" sense). The name that most faithfully reflects the use of the command is "git kitchen-sink" these days.

The user still needs to know that in some context, "revision" may loosely refer to "object name" when talking about a single object, if only to understand the name of that command, let alone various tutorials, screencasts and web resources people made over time.