{"thread":{"id":"332","subject":"[RFC] diff-cache buglet","startedAt":"2005-04-26T16:51:41Z","lastAt":"2005-04-26T20:34:48Z","messageCount":9,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"1751","messageId":"7v7jippjky.fsf@assigned-by-dhcp.cox.net","threadId":"332","inReplyTo":null,"subject":"[RFC] diff-cache buglet","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-26T16:51:41Z","receivedAt":"2005-04-26T16:51:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus,\n\n\twhat do you want diff-cache to do upon finding an\nunmerged path?  Currently your code says \"undecided\" to me.\n\nThe main() attempts to first remove all merge entries before\nletting the diff_cache() do its work, but the diff_cache()\nitself has also some code to deal with unmerged entries.\n\nIn the attached patch, the second hunk starting at ll 76 is a\npure and obvious bugfix.  The function is attempting to remove\nall merge entries, but it stops in the middle; that's why I said\n\"attempts\" in the above.\n\nThe patch lets you have it in both ways by adding --unmerged\nflag.  Without it, unmerged entries are culled at the beginning;\nwith it, you will see diffs for all unmerged stages.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\njit-snap -v 0\n--- k/diff-cache.c\n+++ l/diff-cache.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n \n+static int leave_unmerged = 0;\n static int cached_only = 0;\n static int line_termination = '\\n';\n \n@@ -76,7 +77,7 @@ static void remove_merge_entries(void)\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (!ce_stage(ce))\n-\t\t\tbreak;\n+\t\t\tcontinue;\n \t\tprintf(\"%s: unmerged\\n\", ce->name);\n \t\twhile (remove_entry_at(i)) {\n \t\t\tif (!ce_stage(active_cache[i]))\n@@ -85,7 +86,8 @@ static void remove_merge_entries(void)\n \t}\n }\n \n-static char *diff_cache_usage = \"diff-cache [-r] [-z] [--cached] <tree sha1>\";\n+static char *diff_cache_usage =\n+\"diff-cache [-r] [-z] [--cached] [--unmerged] <tree sha1>\";\n \n int main(int argc, char **argv)\n {\n@@ -110,13 +112,18 @@ int main(int argc, char **argv)\n \t\t\tcached_only = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--unmerged\")) {\n+\t\t\tleave_unmerged = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(diff_cache_usage);\n \t}\n \n \tif (argc != 2 || get_sha1_hex(argv[1], tree_sha1))\n \t\tusage(diff_cache_usage);\n \n-\tremove_merge_entries();\n+\tif (!leave_unmerged)\n+\t\tremove_merge_entries();\n \n \ttree = read_tree_with_tree_or_commit_sha1(tree_sha1, &size, 0);\n \tif (!tree)\n\n\n\n"},{"id":"1753","messageId":"Pine.LNX.4.58.0504261005360.18901@ppc970.osdl.org","threadId":"332","inReplyTo":"7v7jippjky.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] diff-cache buglet","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-04-26T17:11:19Z","receivedAt":"2005-04-26T17:11:19Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Apr 2005, Junio C Hamano wrote:\n> \n> The patch lets you have it in both ways by adding --unmerged\n> flag.  Without it, unmerged entries are culled at the beginning;\n> with it, you will see diffs for all unmerged stages.\n\nI'm ok with that, but if so I think it should show the stage somehow, and\nmake it clear that it's unmerged. Maybe by appending something to the name\n(maybe just a ':' and stage number, but I'd almost prefer the stage number\nto be translated into something human-readable, so maybe we could have\nsomething like\n\n\tfilename.c^orig\n\tfilename.c^first\n\tfilename.c^second\n\nfor stages 1-3 respectively)?\n\nI'll apply your fix right now.\n\n\t\tLinus\n"},{"id":"1757","messageId":"7vy8b5o211.fsf@assigned-by-dhcp.cox.net","threadId":"332","inReplyTo":"Pine.LNX.4.58.0504261005360.18901@ppc970.osdl.org","subject":"Re: [RFC] diff-cache buglet","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-26T17:56:10Z","receivedAt":"2005-04-26T17:56:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds <torvalds@osdl.org> writes:\n\nLT> I'm ok with that, but if so I think it should show the stage somehow, and\nLT> make it clear that it's unmerged. Maybe by appending something to the name\nLT> (maybe just a ':' and stage number, but I'd almost prefer the stage number\nLT> to be translated into something human-readable, so maybe we could have\nLT> something like\n\nLT> \tfilename.c^orig\nLT> \tfilename.c^first\nLT> \tfilename.c^second\n\nLT> for stages 1-3 respectively)?\n\nI'm the one who has been trying very hard to keep the output\nmachine readable (remember? many of the -z flags are mine).\n\nWhile I agree with you that we should somehow show the stage, I\ndo not like your suggestion above very much.  How about adding\none column for stage number before the filename when --unmerged\nis given, just like show-files --stage shows?  You'd soon get\nused to the pattern that has a single digit in between\nwhitespaces to recognize which is merged and which isn't.\nE.g. this is from show-files --stage:\n\n    100644 b258508afb7ceb449981bd9d63d2d3e971bf8d34 1 MM\n    100644 b431b272d829ff3aa4d1a5085f4394ab4d3305b6 2 MM\n    100644 19989d4559aae417fedee240ccf2ba315ea4dc2b 3 MM\n    100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN\n\nAnother thing I'd like to rectify is that show-files use a space\nwhile diff-tree and friends use a tab in between columns.  Is it\ntoo late to standardize one way or the other?  My vote goes to\na space.\n\n"},{"id":"1760","messageId":"Pine.LNX.4.58.0504261103550.18901@ppc970.osdl.org","threadId":"332","inReplyTo":"7vy8b5o211.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] diff-cache buglet","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-04-26T18:06:48Z","receivedAt":"2005-04-26T18:06:48Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Apr 2005, Junio C Hamano wrote:\n> \n> While I agree with you that we should somehow show the stage, I\n> do not like your suggestion above very much.  How about adding\n> one column for stage number before the filename when --unmerged\n> is given, just like show-files --stage shows?  You'd soon get\n> used to the pattern that has a single digit in between\n> whitespaces to recognize which is merged and which isn't.\n\nWell, except for the fact that that isn't machine-readable either, since \nthe \"1 \" thing might just be part of the filename..\n\nBut you could use \"/\" to guarantee that it's unique, together with knowing\nthat git refuses to have non-canonical filenames.\n\nIe 1//filename.c _would_ be machine-readable, thanks to the \"//\" part,\nwhich cannot be part of a valid canonical git filename.\n\n\t\tLinus\n"},{"id":"1766","messageId":"7vsm1do0t4.fsf@assigned-by-dhcp.cox.net","threadId":"332","inReplyTo":"Pine.LNX.4.58.0504261103550.18901@ppc970.osdl.org","subject":"Re: [RFC] diff-cache buglet","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-26T18:22:31Z","receivedAt":"2005-04-26T18:22:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds <torvalds@osdl.org> writes:\n\nLT> Well, except for the fact that that isn't machine-readable\nLT> either, since the \"1 \" thing might just be part of the\nLT> filename..\n\nWell, I somehow thought these things are in fixed column format;\nmode, ->, sha, stage, and filename are all seperated with either\n' ' or '\\t'.  So if I copy MN to \"1 MN\", presumably you would\nsee this:\n\n    100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN\n    100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 1 MN\n\nSo while I agree that // would also work, I fail to see why you\nwould even need that.\n\n"},{"id":"1769","messageId":"Pine.LNX.4.58.0504261137350.18901@ppc970.osdl.org","threadId":"332","inReplyTo":"7vsm1do0t4.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] diff-cache buglet","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-04-26T18:38:37Z","receivedAt":"2005-04-26T18:38:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Apr 2005, Junio C Hamano wrote:\n> \n> Well, I somehow thought these things are in fixed column format;\n> mode, ->, sha, stage, and filename are all seperated with either\n> ' ' or '\\t'.  So if I copy MN to \"1 MN\", presumably you would\n> see this:\n> \n>     100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN\n>     100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 1 MN\n> \n> So while I agree that // would also work, I fail to see why you\n> would even need that.\n\nBecause I'd rather _not_ have the 0 in there at all for the normal case.\n\nYes, it's there for \"show-files --stages\", but it's certainly _not_ there \nin \"diff-tree\" output right now.\n\n\t\tLinus\n"},{"id":"1774","messageId":"7vk6mpnz96.fsf@assigned-by-dhcp.cox.net","threadId":"332","inReplyTo":"Pine.LNX.4.58.0504261137350.18901@ppc970.osdl.org","subject":"Re: [RFC] diff-cache buglet","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-26T18:56:05Z","receivedAt":"2005-04-26T18:56:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds <torvalds@osdl.org> writes:\n\nLT> On Tue, 26 Apr 2005, Junio C Hamano wrote:\n>> \n>> Well, I somehow thought these things are in fixed column format;\n>> mode, ->, sha, stage, and filename are all seperated with either\n>> ' ' or '\\t'.  So if I copy MN to \"1 MN\", presumably you would\n>> see this:\n>> \n>> 100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 MN\n>> 100644 a716d58de4a570e0038f5c307bd8db34daea021f 0 1 MN\n>> \n>> So while I agree that // would also work, I fail to see why you\n>> would even need that.\n\nLT> Because I'd rather _not_ have the 0 in there at all for the normal case.\n\nLT> Yes, it's there for \"show-files --stages\", but it's certainly _not_ there \nLT> in \"diff-tree\" output right now.\n\nI know.  But first let's step back a bit.\n\nRunning diff-cache when you have unmerged entries in your\nGIT_INDEX_FILE is fundamentally broken.  You first read_cache(),\nand then you read-tree into stage 1 of the named tree, and at\nthat point, you do not know what stage 1 means.\n\nWe should just fix \"remove-merge-entries\" and call that\nunconditionally before the read-tree is called.  Once it is\nfixed, we need to think about how to show this stage\ninformation but that should be a separate discussion.\n\nI have attached two versions of patch.  The first one is against\nthe original before my stupid question; the second one is\nagainst the one if you applied my previous patch, to revert most\nof its stupidity.\n\n################################################################\n--- Patch against the original before I asked that stupid question:\n\ncd /opt/packrat/playpen/public/in-place/git/git.junio/\njit-snap -v 0\n--- k/diff-cache.c\n+++ l/diff-cache.c\n@@ -76,7 +76,7 @@ static void remove_merge_entries(void)\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (!ce_stage(ce))\n-\t\t\tbreak;\n+\t\t\tcontinue;\n \t\tprintf(\"%s: unmerged\\n\", ce->name);\n \t\twhile (remove_entry_at(i)) {\n \t\t\tif (!ce_stage(active_cache[i]))\n\n\n################################################################\n--- Patch to revert the stupidity:\ncd /opt/packrat/playpen/public/in-place/git/git.junio/\njit-snap -v 2\n--- k/diff-cache.c\n+++ l/diff-cache.c\n@@ -1,6 +1,5 @@\n #include \"cache.h\"\n \n-static int leave_unmerged = 0;\n static int cached_only = 0;\n static int line_termination = '\\n';\n \n@@ -86,8 +85,7 @@ static void remove_merge_entries(void)\n \t}\n }\n \n-static char *diff_cache_usage =\n-\"diff-cache [-r] [-z] [--cached] [--unmerged] <tree sha1>\";\n+static char *diff_cache_usage = \"diff-cache [-r] [-z] [--cached] <tree sha1>\";\n \n int main(int argc, char **argv)\n {\n@@ -112,18 +110,13 @@ int main(int argc, char **argv)\n \t\t\tcached_only = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, \"--unmerged\")) {\n-\t\t\tleave_unmerged = 1;\n-\t\t\tcontinue;\n-\t\t}\n \t\tusage(diff_cache_usage);\n \t}\n \n \tif (argc != 2 || get_sha1_hex(argv[1], tree_sha1))\n \t\tusage(diff_cache_usage);\n \n-\tif (!leave_unmerged)\n-\t\tremove_merge_entries();\n+\tremove_merge_entries();\n \n \ttree = read_tree_with_tree_or_commit_sha1(tree_sha1, &size, 0);\n \tif (!tree)\n\n"},{"id":"1776","messageId":"Pine.LNX.4.58.0504261207380.18901@ppc970.osdl.org","threadId":"332","inReplyTo":"7vk6mpnz96.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] diff-cache buglet","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-04-26T19:09:14Z","receivedAt":"2005-04-26T19:09:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Apr 2005, Junio C Hamano wrote:\n> \n> We should just fix \"remove-merge-entries\" and call that\n> unconditionally before the read-tree is called.  Once it is\n> fixed, we need to think about how to show this stage\n> information but that should be a separate discussion.\n\nI already applied that part of your fix and pushed it out, there as no \nquestion about that one.\n\nI just thought that _if_ you wanted the unmerged parts to show up, then \nthe \"1//filename.c\" thing might be acceptable. Personally, I just think \ndiff-cache is pretty nonsensical with unmerged files,\n\n\t\tLinus\n"},{"id":"1791","messageId":"7vacnlnuon.fsf@assigned-by-dhcp.cox.net","threadId":"332","inReplyTo":"Pine.LNX.4.58.0504261207380.18901@ppc970.osdl.org","subject":"Re: [RFC] diff-cache buglet","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-26T20:34:48Z","receivedAt":"2005-04-26T20:34:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds <torvalds@osdl.org> writes:\n\nLT> On Tue, 26 Apr 2005, Junio C Hamano wrote:\n>> \n>> We should just fix \"remove-merge-entries\" and call that\n>> unconditionally before the read-tree is called.  Once it is\n>> fixed, we need to think about how to show this stage\n>> information but that should be a separate discussion.\n\nLT> I just thought that _if_ you wanted the unmerged parts to show up, then \nLT> the \"1//filename.c\" thing might be acceptable. Personally, I just think \nLT> diff-cache is pretty nonsensical with unmerged files,\n\nI agree to what you said here.  We are not interested in the\nunmerged files in the original GIT_INDEX_FILE at all.\n\nHowever, remember that \"unmerged\" entries diff_cache() function\nsees are from the tree you are comparing against, either your\nGIT_INDEX_FILE (with --cached) or your working tree (without).\n\nCurrently I suspect the behaviour of diff-cache without --cached\nflag may be broken.  Don't we need to check cached_only before\nor inside of the first two if() statements in diff_cache()?  For\na path that appears in the tree you are comparing against (i.e.\nstage 1 entries):\n\n - if GIT_INDEX_FILE does not have it but the working tree does,\n   it would still say \"deleted\".\n\n - if GIT_INDEX_FILE does have it, the comparison goes against\n   that entry, not against the working tree.\n\nSimilarly for entries that are not in the stage 1, the code ends\nup comparing only the dircache entries and never goes to the\nfilesystem.\n\nHere is a proposed fix.  When running without --cached,\ndiff_cache function really goes to the filesystem if the stage 0\nentry in GIT_INDEX_FILE does not match what is in the working\ntree for these cases.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\ndiff-cache.c |   40 ++++++++++++++++++++++++++++++++++++----\n1 files changed, 36 insertions(+), 4 deletions(-)\n\n./jit-snap -v 3:6\n# - 04/26 12:25 Fix agreed with Linus.\n# + 04/26 13:26 Proposed fix.\n--- k/diff-cache.c\n+++ l/diff-cache.c\n@@ -10,6 +10,19 @@ static void show_file(const char *prefix\n \t       sha1_to_hex(ce->sha1), ce->name, line_termination);\n }\n \n+/* A file *may* have been added to the working tree */\n+static void show_possible_local_add(struct cache_entry *new)\n+{\n+\tstatic unsigned char no_sha1[20];\n+\tstruct stat st;\n+\tif (stat(new->name, &st) < 0)\n+\t\treturn; /* no, working tree does not have one. */\n+\tif (cache_match_stat(new, &st))\n+\t\treturn show_file(\"+\", new);\n+\n+\tprintf(\"+%o\\t%s\\t%s\\t%s%c\", st.st_mode, \"blob\",\n+\t       sha1_to_hex(no_sha1), new->name, line_termination);\n+}\n static int show_modified(struct cache_entry *old, struct cache_entry *new)\n {\n \tunsigned int mode = ntohl(new->ce_mode), oldmode;\n@@ -29,6 +42,8 @@ static int show_modified(struct cache_en\n \t\t\tmode = st.st_mode;\n \t\t\tsha1 = no_sha1;\n \t\t}\n+\t\telse if (old == new)\n+\t\t\treturn 0;\n \t}\n \n \toldmode = ntohl(old->ce_mode);\n@@ -48,16 +63,33 @@ static int diff_cache(struct cache_entry\n \twhile (entries) {\n \t\tstruct cache_entry *ce = *ac;\n \n-\t\t/* No matching 0-stage (current) entry? Show it as deleted */\n+\t\t/* No matching 0-stage (current) entry?\n+\t\t * Show it as deleted.\n+\t\t */\n \t\tif (ce_stage(ce)) {\n-\t\t\tshow_file(\"-\", ce);\n+\t\t\t/* ... well, not so fast.  We may have it in the\n+\t\t\t * working tree and operating without --cache.\n+\t\t\t */\n+\t\t\tif (cached_only)\n+\t\t\t\tshow_file(\"-\", ce);\n+\t\t\telse\n+\t\t\t\t/* this is sneaky but it works.  trust me. */\n+\t\t\t\tshow_modified(ce, ce);\n \t\t\tac++;\n \t\t\tentries--;\n \t\t\tcontinue;\n \t\t}\n-\t\t/* No matching 1-stage (tree) entry? Show the current one as added */\n+\t\t/* No matching 1-stage (tree) entry?\n+\t\t * Show the current one as added.\n+\t\t */\n \t\tif (entries == 1 || !same_name(ce, ac[1])) {\n-\t\t\tshow_file(\"+\", ce);\n+\t\t\t/* ... again, we may not have that in the\n+\t\t\t * working tree and operating without --cache.\n+\t\t\t */\n+\t\t\tif (cached_only)\n+\t\t\t\tshow_file(\"+\", ce);\n+\t\t\telse\n+\t\t\t\tshow_possible_local_add(ce);\n \t\t\tac++;\n \t\t\tentries--;\n \t\t\tcontinue;\n\n\n"}]}