[RFC] diff-cache buglet

[RFC] diff-cache buglet

[Junio C Hamano]

Linus,

	what do you want diff-cache to do upon finding an
unmerged path?  Currently your code says "undecided" to me.

The main() attempts to first remove all merge entries before
letting the diff_cache() do its work, but the diff_cache()
itself has also some code to deal with unmerged entries.

In the attached patch, the second hunk starting at ll 76 is a
pure and obvious bugfix.  The function is attempting to remove
all merge entries, but it stops in the middle; that's why I said
"attempts" in the above.

The patch lets you have it in both ways by adding --unmerged
flag.  Without it, unmerged entries are culled at the beginning;
with it, you will see diffs for all unmerged stages.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

jit-snap -v 0
--- k/diff-cache.c
+++ l/diff-cache.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 
+static int leave_unmerged = 0;
 static int cached_only = 0;
 static int line_termination = '\n';
 
@@ -76,7 +77,7 @@ static void remove_merge_entries(void)
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
-			break;
+			continue;
 		printf("%s: unmerged\n", ce->name);
 		while (remove_entry_at(i)) {
 			if (!ce_stage(active_cache[i]))
@@ -85,7 +86,8 @@ static void remove_merge_entries(void)
 	}
 }
 
-static char *diff_cache_usage = "diff-cache [-r] [-z] [--cached] <tree sha1>";
+static char *diff_cache_usage =
+"diff-cache [-r] [-z] [--cached] [--unmerged] <tree sha1>";
 
 int main(int argc, char **argv)
 {
@@ -110,13 +112,18 @@ int main(int argc, char **argv)
 			cached_only = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--unmerged")) {
+			leave_unmerged = 1;
+			continue;
+		}
 		usage(diff_cache_usage);
 	}
 
 	if (argc != 2 || get_sha1_hex(argv[1], tree_sha1))
 		usage(diff_cache_usage);
 
-	remove_merge_entries();
+	if (!leave_unmerged)
+		remove_merge_entries();
 
 	tree = read_tree_with_tree_or_commit_sha1(tree_sha1, &size, 0);
 	if (!tree)

Re: [RFC] diff-cache buglet

[Linus Torvalds]

On Tue, 26 Apr 2005, Junio C Hamano wrote:
> 
> The patch lets you have it in both ways by adding --unmerged
> flag.  Without it, unmerged entries are culled at the beginning;
> with it, you will see diffs for all unmerged stages.

I'm ok with that, but if so I think it should show the stage somehow, and
make it clear that it's unmerged. Maybe by appending something to the name
(maybe just a ':' and stage number, but I'd almost prefer the stage number
to be translated into something human-readable, so maybe we could have
something like

	filename.c^orig
	filename.c^first
	filename.c^second

for stages 1-3 respectively)?

I'll apply your fix right now.

		Linus

Re: [RFC] diff-cache buglet

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> I'm ok with that, but if so I think it should show the stage somehow, and
LT> make it clear that it's unmerged. Maybe by appending something to the name
LT> (maybe just a ':' and stage number, but I'd almost prefer the stage number
LT> to be translated into something human-readable, so maybe we could have
LT> something like

LT> 	filename.c^orig
LT> 	filename.c^first
LT> 	filename.c^second

LT> for stages 1-3 respectively)?

I'm the one who has been trying very hard to keep the output
machine readable (remember? many of the -z flags are mine).

While I agree with you that we should somehow show the stage, I
do not like your suggestion above very much.  How about adding
one column for stage number before the filename when --unmerged
is given, just like show-files --stage shows?  You'd soon get
used to the pattern that has a single digit in between
whitespaces to recognize which is merged and which isn't.
E.g. this is from show-files --stage:

    100644 b258508afb7ceb449981bd9d63d2d3e971bf8d34 1 MM
    100644 b431b272d829ff3aa4d1a5085f4394ab4d3305b6 2 MM
    100644 19989d4559aae417fedee240ccf2ba315ea4dc2b 3 MM
    100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN

Another thing I'd like to rectify is that show-files use a space
while diff-tree and friends use a tab in between columns.  Is it
too late to standardize one way or the other?  My vote goes to
a space.

Re: [RFC] diff-cache buglet

[Linus Torvalds]

On Tue, 26 Apr 2005, Junio C Hamano wrote:
> 
> While I agree with you that we should somehow show the stage, I
> do not like your suggestion above very much.  How about adding
> one column for stage number before the filename when --unmerged
> is given, just like show-files --stage shows?  You'd soon get
> used to the pattern that has a single digit in between
> whitespaces to recognize which is merged and which isn't.

Well, except for the fact that that isn't machine-readable either, since 
the "1 " thing might just be part of the filename..

But you could use "/" to guarantee that it's unique, together with knowing
that git refuses to have non-canonical filenames.

Ie 1//filename.c _would_ be machine-readable, thanks to the "//" part,
which cannot be part of a valid canonical git filename.

		Linus

Re: [RFC] diff-cache buglet

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> Well, except for the fact that that isn't machine-readable
LT> either, since the "1 " thing might just be part of the
LT> filename..

Well, I somehow thought these things are in fixed column format;
mode, ->, sha, stage, and filename are all seperated with either
' ' or '\t'.  So if I copy MN to "1 MN", presumably you would
see this:

    100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN
    100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 1 MN

So while I agree that // would also work, I fail to see why you
would even need that.

Re: [RFC] diff-cache buglet

[Linus Torvalds]

On Tue, 26 Apr 2005, Junio C Hamano wrote:
> 
> Well, I somehow thought these things are in fixed column format;
> mode, ->, sha, stage, and filename are all seperated with either
> ' ' or '\t'.  So if I copy MN to "1 MN", presumably you would
> see this:
> 
>     100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN
>     100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 1 MN
> 
> So while I agree that // would also work, I fail to see why you
> would even need that.

Because I'd rather _not_ have the 0 in there at all for the normal case.

Yes, it's there for "show-files --stages", but it's certainly _not_ there 
in "diff-tree" output right now.

		Linus

Re: [RFC] diff-cache buglet

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> On Tue, 26 Apr 2005, Junio C Hamano wrote:
>> 
>> Well, I somehow thought these things are in fixed column format;
>> mode, ->, sha, stage, and filename are all seperated with either
>> ' ' or '\t'.  So if I copy MN to "1 MN", presumably you would
>> see this:
>> 
>> 100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN
>> 100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 1 MN
>> 
>> So while I agree that // would also work, I fail to see why you
>> would even need that.

LT> Because I'd rather _not_ have the 0 in there at all for the normal case.

LT> Yes, it's there for "show-files --stages", but it's certainly _not_ there 
LT> in "diff-tree" output right now.

I know.  But first let's step back a bit.

Running diff-cache when you have unmerged entries in your
GIT_INDEX_FILE is fundamentally broken.  You first read_cache(),
and then you read-tree into stage 1 of the named tree, and at
that point, you do not know what stage 1 means.

We should just fix "remove-merge-entries" and call that
unconditionally before the read-tree is called.  Once it is
fixed, we need to think about how to show this stage
information but that should be a separate discussion.

I have attached two versions of patch.  The first one is against
the original before my stupid question; the second one is
against the one if you applied my previous patch, to revert most
of its stupidity.

################################################################
--- Patch against the original before I asked that stupid question:

cd /opt/packrat/playpen/public/in-place/git/git.junio/
jit-snap -v 0
--- k/diff-cache.c
+++ l/diff-cache.c
@@ -76,7 +76,7 @@ static void remove_merge_entries(void)
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
-			break;
+			continue;
 		printf("%s: unmerged\n", ce->name);
 		while (remove_entry_at(i)) {
 			if (!ce_stage(active_cache[i]))


################################################################
--- Patch to revert the stupidity:
cd /opt/packrat/playpen/public/in-place/git/git.junio/
jit-snap -v 2
--- k/diff-cache.c
+++ l/diff-cache.c
@@ -1,6 +1,5 @@
 #include "cache.h"
 
-static int leave_unmerged = 0;
 static int cached_only = 0;
 static int line_termination = '\n';
 
@@ -86,8 +85,7 @@ static void remove_merge_entries(void)
 	}
 }
 
-static char *diff_cache_usage =
-"diff-cache [-r] [-z] [--cached] [--unmerged] <tree sha1>";
+static char *diff_cache_usage = "diff-cache [-r] [-z] [--cached] <tree sha1>";
 
 int main(int argc, char **argv)
 {
@@ -112,18 +110,13 @@ int main(int argc, char **argv)
 			cached_only = 1;
 			continue;
 		}
-		if (!strcmp(arg, "--unmerged")) {
-			leave_unmerged = 1;
-			continue;
-		}
 		usage(diff_cache_usage);
 	}
 
 	if (argc != 2 || get_sha1_hex(argv[1], tree_sha1))
 		usage(diff_cache_usage);
 
-	if (!leave_unmerged)
-		remove_merge_entries();
+	remove_merge_entries();
 
 	tree = read_tree_with_tree_or_commit_sha1(tree_sha1, &size, 0);
 	if (!tree)

Re: [RFC] diff-cache buglet

[Linus Torvalds]

On Tue, 26 Apr 2005, Junio C Hamano wrote:
> 
> We should just fix "remove-merge-entries" and call that
> unconditionally before the read-tree is called.  Once it is
> fixed, we need to think about how to show this stage
> information but that should be a separate discussion.

I already applied that part of your fix and pushed it out, there as no 
question about that one.

I just thought that _if_ you wanted the unmerged parts to show up, then 
the "1//filename.c" thing might be acceptable. Personally, I just think 
diff-cache is pretty nonsensical with unmerged files,

		Linus

Re: [RFC] diff-cache buglet

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> On Tue, 26 Apr 2005, Junio C Hamano wrote:
>> 
>> We should just fix "remove-merge-entries" and call that
>> unconditionally before the read-tree is called.  Once it is
>> fixed, we need to think about how to show this stage
>> information but that should be a separate discussion.

LT> I just thought that _if_ you wanted the unmerged parts to show up, then 
LT> the "1//filename.c" thing might be acceptable. Personally, I just think 
LT> diff-cache is pretty nonsensical with unmerged files,

I agree to what you said here.  We are not interested in the
unmerged files in the original GIT_INDEX_FILE at all.

However, remember that "unmerged" entries diff_cache() function
sees are from the tree you are comparing against, either your
GIT_INDEX_FILE (with --cached) or your working tree (without).

Currently I suspect the behaviour of diff-cache without --cached
flag may be broken.  Don't we need to check cached_only before
or inside of the first two if() statements in diff_cache()?  For
a path that appears in the tree you are comparing against (i.e.
stage 1 entries):

 - if GIT_INDEX_FILE does not have it but the working tree does,
   it would still say "deleted".

 - if GIT_INDEX_FILE does have it, the comparison goes against
   that entry, not against the working tree.

Similarly for entries that are not in the stage 1, the code ends
up comparing only the dircache entries and never goes to the
filesystem.

Here is a proposed fix.  When running without --cached,
diff_cache function really goes to the filesystem if the stage 0
entry in GIT_INDEX_FILE does not match what is in the working
tree for these cases.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

diff-cache.c |   40 ++++++++++++++++++++++++++++++++++++----
1 files changed, 36 insertions(+), 4 deletions(-)

./jit-snap -v 3:6
# - 04/26 12:25 Fix agreed with Linus.
# + 04/26 13:26 Proposed fix.
--- k/diff-cache.c
+++ l/diff-cache.c
@@ -10,6 +10,19 @@ static void show_file(const char *prefix
 	       sha1_to_hex(ce->sha1), ce->name, line_termination);
 }
 
+/* A file *may* have been added to the working tree */
+static void show_possible_local_add(struct cache_entry *new)
+{
+	static unsigned char no_sha1[20];
+	struct stat st;
+	if (stat(new->name, &st) < 0)
+		return; /* no, working tree does not have one. */
+	if (cache_match_stat(new, &st))
+		return show_file("+", new);
+
+	printf("+%o\t%s\t%s\t%s%c", st.st_mode, "blob",
+	       sha1_to_hex(no_sha1), new->name, line_termination);
+}
 static int show_modified(struct cache_entry *old, struct cache_entry *new)
 {
 	unsigned int mode = ntohl(new->ce_mode), oldmode;
@@ -29,6 +42,8 @@ static int show_modified(struct cache_en
 			mode = st.st_mode;
 			sha1 = no_sha1;
 		}
+		else if (old == new)
+			return 0;
 	}
 
 	oldmode = ntohl(old->ce_mode);
@@ -48,16 +63,33 @@ static int diff_cache(struct cache_entry
 	while (entries) {
 		struct cache_entry *ce = *ac;
 
-		/* No matching 0-stage (current) entry? Show it as deleted */
+		/* No matching 0-stage (current) entry?
+		 * Show it as deleted.
+		 */
 		if (ce_stage(ce)) {
-			show_file("-", ce);
+			/* ... well, not so fast.  We may have it in the
+			 * working tree and operating without --cache.
+			 */
+			if (cached_only)
+				show_file("-", ce);
+			else
+				/* this is sneaky but it works.  trust me. */
+				show_modified(ce, ce);
 			ac++;
 			entries--;
 			continue;
 		}
-		/* No matching 1-stage (tree) entry? Show the current one as added */
+		/* No matching 1-stage (tree) entry?
+		 * Show the current one as added.
+		 */
 		if (entries == 1 || !same_name(ce, ac[1])) {
-			show_file("+", ce);
+			/* ... again, we may not have that in the
+			 * working tree and operating without --cache.
+			 */
+			if (cached_only)
+				show_file("+", ce);
+			else
+				show_possible_local_add(ce);
 			ac++;
 			entries--;
 			continue;