patch, 5 partsDocumentation/git-diff.txt improvements

Hi again,

So I decided that builtin/diff.c is hardcoded for the <commit>..<commit> and <commit>...<commit> forms, and we can do nothing about it unless we want to break compatibility (maybe a git 2.0 candidate?). The least we can do is document it properly in the SYNOPSIS. I've done this in [4/5].

The other patches are just general "while we're there" cleanups.

Ramkumar Ramachandra (5):
  git-diff.txt: reorder the <commit> <commit> form
  git-diff.txt: strip the leading "--" from options template
  git-diff.txt: group the [--] and [<path>...] templates
  git-diff.txt: document the .. and ... forms in SYNOPSIS
  git-diff.txt: fix ordering of the [--no-index] form

 Documentation/git-diff.txt | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

In DESCRIPTION, the '<commit>..<commit>' form refers to "the previous form", namely the '<commit> <commit>' form. However, bd52900 (Documentation: Describe "git diff <blob> <blob>" separately, 2012-12-18) broke this by inserting a form in between these two forms. Fix this by reordering a form.

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

Ramkumar Ramachandra wrote:

Good catch.

[...]

For the sake of exposition I think it makes more sense to put <commit> <commit> *before* <blob> <blob>, since the former is used more often.

Thanks, Jonathan

It is imperative to specify options with the [options] template. [--options] is inconsistent as well as misleading (as there are short options which can be specified with a single "-"). Fix this.

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

Ramkumar Ramachandra wrote:

Why is it imperative?

Anyway, this looks like a good change. With that sentence removed or some other clarification,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Jonathan Nieder wrote:

Sorry about the thinko; s/imperative/idiomatic/

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Yeah, I think this step looks reasonable.

Clarify that "--" is meant to disambiguate paths from the other options.

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

You can say

    "git diff A B --" without any path
    "git diff A B pathspec" without any double-dashes
    "git diff -- pathspec"

and all three of them are expressed by versions before or after the patch, so there is no change in the semantics introduced by this patch.

While the update might be logically more correct, it looks to me that the only end-user visibile effect of it is to make the end result harder to read.

Junio C Hamano wrote:

Hm, I thought it improves readability. I'm trying to say that -- is used to separate [<path>...] from [everything else].

Ramkumar Ramachandra <artagnon@gmail.com> writes:

The same logic would apply to this semi-nonsense rewrite, no?

	git diff [[options] [--]] [<path>...]

Everything else comes before "--" (if exists) that separates it from the list of pathspecs.

Junio C Hamano wrote:

Yeah, except it's sometimes

    [[options] [revision range] [--]] [<path>...]

and other times

    [[options] [<commit>...] [--]] [<path>...]

and yet other times

    [[options] [--]] [<path>...]

Whereas mine is always

    ... [[--] [<path>]]

(everywhere in git)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

You are missing the entire point. Neither [[everything else][--]][path] or [everything else][[--][path]] is better than without the extra level of [], which does not help clarifying _anything_.

Junio C Hamano wrote:

Yeah, okay: I'll drop this patch. I was just saying; I don't feel strongly about it at all.

Ramkumar Ramachandra wrote:

I agree with the goal, but I don't think this change achieves it. Maybe adding gitcli(7) to the SEE ALSO section would work?

Jonathan

The DESCRIPTION refers to the '<commit>..<commit>' and '<commit>...<commit>' forms, but the SYNOPSIS does not list them at all. Fix this.

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

Ramkumar Ramachandra wrote:

This is preparation for removing <commit>..<commit> from the documentation, right? It makes sense to add the ... form since there's no other way to spell it but I can't say I'm excited about adding the .. form. Especially since <tree>..<tree> doesn't work, that form's a little embarrassing. ;-)

*checks the manpage*

Ah, this is to make the SYNOPSIS a better table of contents for the DESCRIPTION (except that the DESCRIPTION treats "git diff --no-index <path> <path>" before "git diff [<commit>] -- <pathspec>" while the SYNOPSIS doesn't). Maybe it would make sense to go all the way and make the two follow the same order?

My 2¢, Jonathan

The SYNOPSIS lists the [--no-index] form as the last item, but the DESCRIPTION lists it as a natural extension of the first form. Fix this with a reordering. Also since the DESCRIPTION breaks up the first form in the SYNOPSIS into two different forms (one without the optional [<commit>], and the other with it), reorder to make these two forms appear together.

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Perhaps either the description or your reading is wrong.

The "--no-index" mode was a hack to allow "git diff" goodies to be used outside the context of "git", and a proper execution of it should have been to send patches to GNU or BSD diff maintainers, not to add the "--no-index" option that is unrelated to "git" to our code.

I think it is more correct to list it as a footnote and list separately from the main text, which is what the original SYNOPSIS section does.

Junio C Hamano wrote:

Yeah, I know.  I use it instead of diff all the time.

I was trying to be more "correct" here. The form is a special case of the first form where the number of paths are limited to two. Besides, isn't that how the DESCRIPTION section explains it now?

Ramkumar Ramachandra wrote:

Sort of. It's a completely different form, but when --no-index is left out the syntax is a special case of "git diff -- <path>...".

Maybe it would make sense to move towards eliminating the "implicit --no-index for paths outside the repository" trick. I use "git diff --no-index" all the time, but I always spell it out to be careful.

Jonathan Nieder wrote:

Huh? Why do you want to endure the pain of spelling it out, when your intent is perfectly clear?

What would make sense is a tristate diff.no-index: inside-worktree|outside-worktree|always. I don't know why we should even have a "never".

Ramkumar Ramachandra wrote:

Because typing paths does not make my intent perfectly clear.

Oh please no.

Hope that helps, Jonathan

Jonathan Nieder wrote:

If I understand this correctly, you are horrified by this? https://github.com/artagnon/dotfiles/blob/master/.gitconfig#L30

By the way, my zsh aliases git to g.

Ramkumar Ramachandra wrote:

Nope, that looks like a useful way to save typing, and "git help" helpfully expands any of your custom commands when you want to translate in order to help other people.

Jonathan Nieder wrote:

I'm not able to understand this. Doesn't your prompt tell you which directory you're in, and if you're in a git repository? When you type out paths, you know what is inside and what is outside your repository. By extension, you know when --no-index is implied and when it isn't.

Can you explain what your problem is?

Ramkumar Ramachandra wrote:

Are you wondering why I use --no-index or why I think we should eventually stop moving into --no-index mode by default?

The answers are different. I use --no-index because it means I don't have to think about whether the files I am comparing are in a Git repository. It's relaxing. I'm not advocating that you follow suit; I'm just describing my own usage of the command.

I think git should eventually stop doing --no-index implicitly because it is a very different mode using the same syntax triggered by external conditions. That is

 * hard to document
 * problematic for scripts calling "git diff", can create lurking bugs
 * unnecessarily complicated.  For example:
   * "git diff -h" output depends on whether I am in a git worktree.
   * git searches for a .git directory, possibly hitting filesystem
     automount points, just to decide whether its arguments are inside
     the current repository.

Jonathan Nieder wrote:

I completely disagree, but we don't have to agree: make it a configuration variable. Even if it's turned to "never" by default, I don't mind having one extra line in my .gitconfig. But you went all "Oh please no" when I brought it up. I thought you were claiming that nobody finds it useful. Now, you seem to be claiming that people who find it useful must suffer.

Ramkumar Ramachandra wrote:

I thought we had discussed before how every configuration variable costs quite a lot in terms of Git's teachability.

What would that configuration variable even mean? "Set this to make other people's scripts work when they assume --no-index won't be triggered automatically"?

Jonathan Nieder wrote:

No.  "Set this for interactive use".

The '<commit>..<commit>' and '<commit>...<commit>' forms are confusing as they are reminiscent of the corresponding forms in the "SPECIFYING RANGES" section of revisions.txt. We can remove the '<commit>..<commit>' form now (hence discouraging its use), since it is exactly equivalent to the clearer '<commit> <commit>' form. However, we must keep the '<commit>...<commit>' form around until we can find a different way to express the same thing.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-diff.txt | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

I agree with the end result not to list .. form in the SYNOPSIS, but you shouldn't have added it in the first place in the earlier patch.

I do not think it is a bad idea to drop this enumerated item, but I would have expected a sidenote to be added for the description for "git diff A B" form, so that people who saw "git diff A..B" somewhere on the web and wonder what it means can look it up.

On Wed, Apr 24, 2013 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:

I'm expecting to re-roll anyway.  I just wanted to show it to you now.

Yeah, good idea.  I'll put it in the section after the ... syntax.

So, I decided to drop everything and just submit the first two parts. Until we can decide what to do about the '<commit>..<commit>' and '<commit>...<commit>' forms (new rev spec? overload existing rev spec to change the meaning? deprecate without a viable alternative?), let's leave them as it is.

Nothing to resubmit.  Junio: can you just queue the first two parts?

Thanks.

Ramkumar Ramachandra (2):
  git-diff.txt: reorder the <commit> <commit> form
  git-diff.txt: strip the leading "--" from options template

 Documentation/git-diff.txt | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

I had an impression that many of them, with the suggested fix-ups, were improvements, with or without the A~B discussion. It seems that the entire review wasted many braincycles if you are to drop the ball at this point, which is very dissapointing.

I do not appreciate "Nothing to resubmit". [1/5] needs fixes to the issue Jonathan pointed out, doesn't it?

Junio C Hamano wrote:

No, I don't meant to drop the ball at all. I'm _very_ annoyed by this inconsistency, and have a burning desire to fix it. It's just that I don't want to fixup something now and regret it later, so I'm putting it off until the A~B issue gets resolved. In the meantime, the first two patches are fine and can be applied as is.

Oh, I thought it was fairly trivial and that you'd fix it up locally. Would you like me to resend with the fixup?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

As far as I am concerned, there is no A~B issue.

Please don't let it take good changes hostage.

Junio C Hamano wrote:

You don't have to remind me about how hostile you are towards new proposals. I already know.

Those are my changes, and I will not check them in until I'm happy with them. How did this turn into a hostage situation? I've already signed off on my changes: you or anyone else has every right to pick them up and merge them (even if I don't like them).

Ramkumar Ramachandra <artagnon@gmail.com> writes:

That's not "hostile" or "new". I only express strong doubt against "ideas" (or non-"ideas") that do not make sense to me.

They may be valid and merely are not well explained, or they may be total nonsense. The latter does not deserve time from people on this list, but the former needs to be better explained.