patchgit-diff: Add --staged as a synonym for --cached.

---
 Consider this as a replacement to the previous git-staged series.

 Documentation/git-diff.txt |    1 +
 builtin-diff.c             |    5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

On Wed, Oct 29, 2008 at 09:15:36AM -0700, David Symonds wrote:

I think this is a much more sensible (actual) approach.

Hmm. I wonder if it would make it more sense to make the "official" name --staged, and leave --cached forever as a synonym. If the goal is giving sane names to end users, then we should probably advertise the sane ones.

OTOH, maybe it is better to start slow, let people who are doing training materials mention --staged, and see how that works.

I had to investigate this hunk closely, as it really looks at first glance (from the function name, and the fact that there are two hunks, one here and one for cmd_diff) that this is impacting diff-index --cached, but it's not. We just checked --cached in two different places inside git-diff (but at least one of them is prefixed by a comment that includes the world "Eek.").

-Peff

On Wed, Oct 29, 2008 at 9:42 AM, Jeff King <peff@peff.net> wrote:

I agree. If there's some consensus, I can make that shift, keeping --cached as a backward-compatibility synonym.

Dave.

Hi,

On Wed, 29 Oct 2008, David Symonds wrote:

Yes, I would like that, too.

However, note that we have to hash out what to do about the convention that --cached traditionally means that only the staging area (formerly known as "the index") is affected, while --index means that the command touches the working directory, too.

Ciao, Dscho

On Wed, Oct 29, 2008 at 06:06:09PM +0100, Johannes Schindelin wrote:

If we assume that we have only the word "stage" and variations available, then there aren't too many options.

  only the staging area:
    --stage-only, --staged-only

  both:
    --staged (as opposed to --staged-only) --stage-and-worktree (too
    long), --both (not descriptive enough), --stage-too (yuck)

-Peff

Jeff King <peff@peff.net> writes:

A flag "--staged" that means "staged changes and changes in the work tree" is no worse than the current "--index". If we were to shoot for clarity, how about --staged-only (aka --cached) vs --staged-and-unstaged (aka --index)?

I am actually actively unhappy about the latter, but I like more descriptive --staged-only for the former a lot better.

On Sun, Nov 02, 2008 at 01:30:46AM -0700, Junio C Hamano wrote:

Well, there is another flag state to be considered, of course, which is "no flag". So I think "--staged" is fine to mean the working tree and staged, as long as the default (i.e., no option given) is to operate on the working tree.

I can't think offhand of any commands that violate that assumption.

Agreed. --staged-only is fine to me, but --staged-and-unstaged just seems too long.

-Peff

On 2008.10.29 13:11:22 -0400, Jeff King wrote:

Hm, I don't think that would work out nicely with stash. --keep-index would become --keep-staged-only, which is IMHO pretty confusing, as the default is to keep nothing. And even if you add another option to keep all changes, so that the current state is just put onto the stash, but the working tree and index are unchanged, you would have --keep-staged and --keep-staged-only. Not really any better.

Admittedly, --keep-index is quite different from --index, but if you're going to change the CLI to hide the word "index", that option needs to be changed as well and the usage of the new terms should be unified.

Looking at --cached/--index we have basically three things:

  --cached to refer to the state of the index (diff, grep, [stash], ...)
  --cached to _work on_ the index only (rm, apply, ...)
  --index to _work on_ both the index and the working tree (apply, ...)

Maybe that could be translated to:

  --staged: refer to the state of the index
  --stage: in addition to changing the working tree, also stage the changes
  --stage-only: only stage the changes, don't change the working tree

That would give us, for example: git diff --staged git grep --staged

git apply --stage git apply --stage-only git rm --stage-only

git stash --keep-staged

A quick look through Documentation/ revealed only one problematic case, which is ls-files that already has a --stage option. And that looks like a dealbreaker :-(

Björn

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

I think the earlier two are the same thing. The only difference between them is that in the first one, the definition of your "work on" happens to be a read-only operation. Am I mistaken?

'ls-files' is primarily about the index contents and all else is a fluff ;-)

You could say --show-stage-too if you wanted to, but the command is a plumbing to begin with, so perhaps if we can identify the cases where people need to use the command and enhance some Porcelain (likely candidate is 'status' or perhaps 'status --short') to give the information people use ls-files for, we hopefully wouldn't have to change ls-files itself at all.

The only case I use ls-files these days when I am _using_ git (as opposed to developing/debugging git) is "git ls-files -u" to get the list of still unmerged paths during a conflicted merge.

On 2008.11.02 10:30:16 -0800, Junio C Hamano wrote:

Yeah, I actually wanted to change that "work on" to a simple "change", but forgot to do that before sending... :-(

The idea was that currently "--cached" can be "passive" (just look at the index instead of the working tree) or "active" (change the index instead of the working tree). Thus there could be three "flag words", and their usage can be unified, including stash.

"git diff [my] --staged [changes]" "git stash [but] --keep-staged [changes]" "git apply [and] --stage my_patch" "git rm [but] --stage-only some_file"

OK, the last one is still not even close to a proper sentence, and but I guess you get the idea ;-)

Heh, that's probably the one thing for which I use "git status" the most.

Björn

On Sun, Nov 02, 2008 at 10:30:16AM -0800, Junio C Hamano wrote:

I think that is somewhat the case for "grep", for example. But the confusion is that diff is really a different beast, because you are comparing two _different_ locations.

So "git diff --staged", while it makes sense to us (since we are asking "what is staged"), is not consistent with the discussed rules. In particular:

  1. It operates on just the "stage" and not the working tree, so it
     should be "--staged-only". But the only there is nonsensical.

  2. The default is _already_ operating on the staging area, so you are
     really switching up the working tree for the HEAD in what you are
     diffing. So in that sense, it doesn't convey the change in
     operation very well.

And I am not proposing a change here (except to perhaps "git diff --staged" instead of "--cached"). Just pointing out that it does not follow the "--staged operates on both, --staged-only operates on just the index" rule.

Hrm. For that matter, grep is a bit different, too. Since I would expect "git grep --staged" to find only staged things, not things in both the working tree and the index. So perhaps there is a difference between commands that modify and commands that inspect.

-Peff

On Mon, Nov 3, 2008 at 6:14 PM, Jeff King <peff@peff.net> wrote:

So apart from the wider discussion, I think this patch by itself is a nice step forward towards improving the UI of this part of git. Is there any further discussion on this one alone?

Dave.

On Tue, Nov 11, 2008 at 10:37:37AM +1100, David Symonds wrote:

I think --staged for --cached is definitely an improvement. Even if we don't have a set rule for "this is when you use --staged, and this is when you use --staged-only", I think everyone so far agrees that any such rule will have to incorporate "diff --staged" somehow, since it reads pretty clearly.

So yes, I would like to see this applied. However, there are two possible improvements:

  1. Your documentation update says --staged is a synonym. Should
     --staged perhaps be the "new" way of doing this, and --cached is
     left as a historical alias? IOW, point users at the name we think
     is more sensible.

  2. Part of the point of this is to avoid using the world "cached" in
     instructional materials. The tutorial and user manual should
     probably be updated to use --staged (and it is reasonably safe to
     do so immediately, since they are tied to the installed git
     version; I hope Scott will update his materials eventually, but
     lagging makes sense there as people will still be using existing
     versions for some time).

     And of course, such updates would be a small part of any larger
     decision to call the index something else (the "stage" or
     whatever) in those materials, but I think your point is to do this
     one positive thing and not wait for the other bits.

-Peff

"David Symonds" <dsymonds@gmail.com> writes:

I do not think anybody is fundamentally opposed to introduce a consistent set of new synonyms. I do not think anybody disagrees that the word "stage" will be involved in that set, either.

I however have a suspicion that people would regret having applied this "diff --staged" patch, after they realize that other commands need two options "--staged-only" and "--staged-too", and would wish this patch were to introduce a synonym "diff --staged-only", not "diff --staged", for uniformity's sake.

I doubt "Is there any further discussion on THIS ONE ALONE?" is a valid question to ask. What are the other command options we are introducing synonyms for? There is no need for two variants of staged for "diff" (you don't have --staged-too option but instead you give a committish argument, e.g. HEAD), so --staged-only can be abbreviated to --staged without risking any ambiguity. But at least a fully-spelled-out --staged-only should also be accepted, shouldn't it?

On Mon, Nov 10, 2008 at 05:11:08PM -0800, Junio C Hamano wrote:

I'm not sure that "staged-only" really makes sense here. In modification commands like "apply", it is about "do this one thing to the working tree, to both the index and the working tree, or to just the index".

But here, you are selecting two points for comparison. So while it is tempting to say "the default for diff just happens to work on the index and the working tree, so we don't need --staged-too", I don't think that is right. Doing "--staged-only" is _not_ about saying "do the thing we would have done to the working tree and the index to just the index." It is about "use HEAD as one of the points instead of the working tree (and reverse the order of points :) )".

To me, what is really being asked with "git diff --staged" (or "git diff --cached" for that matter), is "what is staged?" That is, diff is not about an operation on a data location (like HEAD, index, or working tree), but rather an operatoin on a data _relationship_. So you ask for "what is not staged" (the relationship between index and working tree), "what is staged" (the relationship between HEAD and index), "what is different between the working tree and HEAD", or "what is different between these two trees".

-Peff

Jeff King <peff@peff.net> writes:

Ok, "what is staged (in index)?", relative to the named commit which defaults to HEAD, is a good argument. Let's apply it.

On Mon, Nov 3, 2008 at 2:14 AM, Jeff King <peff@peff.net> wrote:

Speaking just for myself, I would find this all a lot less confusing if "staged" were a refspec of some sort, not an option at all.

   git diff HEAD..STAGED
   git diff STAGED..WORKTREE
   git grep pattern STAGED HEAD sillybranch WORKTREE ^ignorebranch --
path/to/files

git-rev-parse already gives us a nice syntax for including/excluding particular trees as much as we like; the only problem is you can't talk about the work tree or index as if they were revisions.

Have fun,

Avery

"Avery Pennarun" <apenwarr@gmail.com> writes:

Another thing that seems strange to me is that the operation of diffing HEAD..STAGED is so verbose -- currently it's "git diff --cached", with no short option.

Surely this is very common operation... I rather often want to see details of what's staged for commit...

-Miles

On Mon, Nov 10, 2008 at 11:04:42PM -0500, Avery Pennarun wrote:

I agree that such a thing is reasonably intuitive. I have thought about "magic" refspecs before; my local git has an "EMPTY" refspec which points to the empty tree for diffing. However, that was trivial to implement (since it turns into a sha1), and yours is very hard (since you will have to pass these "pretend" objects around).

So I think it is a neat idea, but I am not volunteering to work on it. :)

-Peff

Hi,

On Wed, 12 Nov 2008, Jeff King wrote:

Just in case anybody thought about creating tree objects on the fly and use their SHA-1s: that won't fly, as you can have unmerged entries in the index. So STAGED.. is a _fundamentally_ different thing from HEAD^..

Maybe we could play tricks with a special staged_commit (pretending to be a commit with SHA-1 000000... so that git log STAGED.. would do the same as plain git log, the rationale being that STAGED is no commit, so ^STAGED should be a nop).

"git diff" would then have some special handling for the case that there are exactly two revs, exactly one of them negative, and exactly one of them being the staged_commit, passing off to the respective diff backends.

Ciao, Dscho

On Wed, Nov 12, 2008 at 12:10:57PM +0100, Johannes Schindelin wrote:

I thought about that at first, too, but the working tree is even more painful. You would have to hash every changed file on the filesystem to create the tree object.

-Peff

On Wed, Nov 12, 2008 at 6:06 AM, Jeff King <peff@peff.net> wrote:

Is that so bad?  You have to read all those files anyway in order to do a diff.

Avery

On Wed, Nov 12, 2008 at 10:39:21AM -0500, Avery Pennarun wrote:

I don't know for sure, as I haven't tried it. But you would need to read them twice (once to hash, and then once to diff) plus the extra computation time of hashing. So assuming you have a decent cache, you pay the disk access only once.

Maybe it would be negligible, but I would have to see numbers to be convinced either way.

-Peff

Jeff King <peff@peff.net> writes:

I think you guys are barking up a wrong tree.

The staged state, the work tree state and the committed states are three conceptually different things. Making them stand out as distinct entities at the UI level is a _good thing_.

Introducing STAGED or WORKTREE psuedonym to deliberately muddy the distinction goes against helping the users form a clear vision of what s/he is working on at the conceptual level.

On Wed, Nov 12, 2008 at 11:29:35AM -0800, Junio C Hamano wrote:

I'm not sure I agree. They _are_ different things, but in the case of diff, you are really treating each of them like a tree (which makes range operators a little silly, but then that is a silliness already present in "git diff tree1..tree2").

But again, I would not be convinced this is a good direction until I saw:

 - the actual design, especially to what degree any ugliness is exposed
   when we realize that they _aren't_ trees. IOW, how badly does this
   abstraction leak?

 - numbers showing that it isn't going to perform significantly worse

And I'm still not volunteering to work on it, so somebody else will have to come up with those things. ;)

-Peff

Jeff King <peff@peff.net> writes:

It is not _little_ silly, but quite silly. It is a historical accident and I personally suggest against using it when I teach git to others.

And we are _not_ treating each of them like a tree. We might be treating them as collection of paths (and the range operator is about commits).

Even if they would, I do not think it is a good direction to go. It makes the UI less intuitive and harder to learn.

On Wed, Nov 12, 2008 at 2:57 PM, Junio C Hamano <gitster@pobox.com> wrote:

I assume the reason is that "git diff tree1..tree2" works with the differences between tree1 and tree2, much like "git log tree1..tree2" does. On the other hand, "git log tree1 tree2" is something completely different.

So at least in my mental model, it's "git diff tree1 tree2" that's out of place, not really the one with the range specifier.

Apparently what's intuitive to one person isn't always intuitive to the next.

Avery

"Avery Pennarun" <apenwarr@gmail.com> writes:

Actually, that perception is already confused. The analogue to "log A..B" is expressed as "diff A...B", and not "diff A..B".

That is one of the reasons why I tend to teach against using "diff A..B" unless you know what it is doing. I'd suggest to get out of that habit before you confuse yourself even more ;-).

The _only_ reason diff takes A..B and A...B syntax is because the command line parameter parser was easy to write that way. IOW, it was an artifact of the implementation convenience.

On Wed, Nov 12, 2008 at 6:10 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Hmm, I tried it to see, and "git diff --cached branchname" when there are unmerged entries looks like this (one line):

* Unmerged path /whatever/file

Which is pretty unhelpful anyhow (although I don't know what would be better). I can think of several ways to produce the same output, including using a magic SHA-1 that means "unmerged", or using a different filemode for unmerged files in the tree object, or actually including all three versions of the file in the tree object, each with a different mode. I admit that sounds pretty gross, though.

I might have imagined STAGED to be a child commit of HEAD (or rather, its parents should be the same as if you did 'git commit'), but I don't really know for sure. In such a case, ^STAGED would definitely have a meaning.

Have fun,

Avery