On Wed, Jun 27, 2012 at 03:17:54PM -0700, Junio C Hamano wrote: > > I think that's bad. I wonder if it should have "*" attributes applied to > > it or not. While I can see it being convenient in some cases, I think it > > makes the rules confusingly complex. > > It is likely that the crlf conversion on Dos systems wants to be > applied regardless. Yeah, that's specifically the case I was thinking of. I would say "well, we don't need to care about path at all, they can just use core.autocrlf", but I think autocrlf is discouraged these days in favor of using attributes. > This is unrelated to the "standard input confusion" issue, but there > are two more things coming either from the way the no-index code is > done or from the way it is bolted onto git. > > With the current code, this: > > mkdir foo bar > echo hello >foo/1 > echo bye >bar/2 > git diff --no-index foo bar > > shows differences between a/foo/1 and b/bar/1, as the no-index code > records foo/1 and bar/1 as the paths in the filespec for them. > > But if you are comparing two directory hierarchies, it is a lot more > likely that you would want to see corresponding files in these two > directories. In other words, the patch is better shown as > differences between a/1 and b/1 (the code makes the core compare > foo/1 and bar/2 after all). This of course requires no-index to > differentiate the logical pathname (i.e. "this is the path inside > collection a/ (or b/)") and the physical location from which the > contents are read. Such a differentiation would allow us to also do > renames and rename classifications much more sanely. We had to add > DIFF_PAIR_RENAME() and filepair->renamed_pair only to support this > codepath because of this misdesign. We could have just run strcmp() > between the logical pathname of one/two members of the filepair to > see if the pair was renamed if it was done that way. Yeah, that makes sense. Really you want to split the idea of diff_filespec into a logical unit of "the thing I am diffing" and a source struct of "here is where I get the data from". And the latter could be a union of blob information, filesystem path, and stdin flag, all contained inside the filespec. > And the way diff-no-index.c::queue_diff() walks two directories to > grab paths from them in parallel and incrementally means that the > filesystem walking code cannot be reused for something like this: > > git diff master:Documentation /var/tmp/docs > > to compare a hierarchy represented with a tree object with another > hierarchy stored in the filesystem outside git's control. A natural > way to do so would be to grab a set of paths from /var/tmp/docs and > have that set compared against the other set that comes from the tree, > and the "grab a set of paths from /var/tmp/docs" machinery can be > used twice to implement the current > > git diff --no-index /var/tmp/foo /var/tmp/bar > > which would have been a lot cleaner implementation. Agreed. I have occasionally wanted to do something like the tree comparison you mentioned above, and I think I resorted to actually making a git tree out of it. All of that is nice, and if you feel like working on it, great. But I admit I don't care too much about the --no-index code path. The key thing to me is fixing the "-" path in the regular code path without regressing the no-index stdin code-path too badly. And I think your patches already do that, so it might be a good stopping point. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html