{"thread":{"id":"12422","subject":"[PATCH 0/3] fix \"diff --raw\" on the work tree side","startedAt":"2008-03-02T09:43:29Z","lastAt":"2008-03-02T17:48:02Z","messageCount":8,"participants":["Junio C Hamano","Ping Yin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"70611","messageId":"1204451012-17487-1-git-send-email-gitster@pobox.com","threadId":"12422","inReplyTo":null,"subject":"[PATCH 0/3] fix \"diff --raw\" on the work tree side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T09:43:29Z","receivedAt":"2008-03-02T09:43:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The second patch fixes the inconsistency between \"git-diff --raw\" and\n\"git-diff-{index,files} --raw\" when they are used for submodules.\n\nThe third one is a bit controversial and changes the semantics and\nexisting callers and won't be considered for 1.5.5.\n\nJunio C Hamano (3):\n      diff-lib.c: constness strengthening\n      diff: make sure work tree side is shown as 0{40} when different\n      diff: show submodule object name when available even on the work tree side\n\n diff-lib.c                |   45 +++++++++++++++++++++++++++++++------\n diff.c                    |    7 +----\n t/t4027-diff-submodule.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+), 13 deletions(-)\n create mode 100755 t/t4027-diff-submodule.sh\n\n"},{"id":"70612","messageId":"1204451012-17487-2-git-send-email-gitster@pobox.com","threadId":"12422","inReplyTo":"1204451012-17487-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] diff-lib.c: constness strengthening","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T09:43:30Z","receivedAt":"2008-03-02T09:43:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The internal implementation of diff-index codepath used to use non const\npointer to pass sha1 around, but it did not have to.  With this, we can\nalso lose the private no_sha1[] array, as we can use the public null_sha1[]\narray that exists exactly for the same purpose.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff-lib.c |   13 ++++++-------\n 1 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 94b150e..4581b59 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -472,22 +472,21 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n static void diff_index_show_file(struct rev_info *revs,\n \t\t\t\t const char *prefix,\n \t\t\t\t struct cache_entry *ce,\n-\t\t\t\t unsigned char *sha1, unsigned int mode)\n+\t\t\t\t const unsigned char *sha1, unsigned int mode)\n {\n \tdiff_addremove(&revs->diffopt, prefix[0], mode,\n \t\t       sha1, ce->name, NULL);\n }\n \n static int get_stat_data(struct cache_entry *ce,\n-\t\t\t unsigned char **sha1p,\n+\t\t\t const unsigned char **sha1p,\n \t\t\t unsigned int *modep,\n \t\t\t int cached, int match_missing)\n {\n-\tunsigned char *sha1 = ce->sha1;\n+\tconst unsigned char *sha1 = ce->sha1;\n \tunsigned int mode = ce->ce_mode;\n \n \tif (!cached) {\n-\t\tstatic unsigned char no_sha1[20];\n \t\tint changed;\n \t\tstruct stat st;\n \t\tif (lstat(ce->name, &st) < 0) {\n@@ -501,7 +500,7 @@ static int get_stat_data(struct cache_entry *ce,\n \t\tchanged = ce_match_stat(ce, &st, 0);\n \t\tif (changed) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n-\t\t\tsha1 = no_sha1;\n+\t\t\tsha1 = null_sha1;\n \t\t}\n \t}\n \n@@ -514,7 +513,7 @@ static void show_new_file(struct rev_info *revs,\n \t\t\t  struct cache_entry *new,\n \t\t\t  int cached, int match_missing)\n {\n-\tunsigned char *sha1;\n+\tconst unsigned char *sha1;\n \tunsigned int mode;\n \n \t/* New file in the index: it might actually be different in\n@@ -533,7 +532,7 @@ static int show_modified(struct rev_info *revs,\n \t\t\t int cached, int match_missing)\n {\n \tunsigned int mode, oldmode;\n-\tunsigned char *sha1;\n+\tconst unsigned char *sha1;\n \n \tif (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {\n \t\tif (report_missing)\n-- \n1.5.4.3.468.g36990\n\n"},{"id":"70613","messageId":"1204451012-17487-3-git-send-email-gitster@pobox.com","threadId":"12422","inReplyTo":"1204451012-17487-2-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] diff: make sure work tree side is shown as 0{40} when different","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T09:43:31Z","receivedAt":"2008-03-02T09:43:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ping Yin noticed that \"git diff-index --raw\" shows 0{40} when work tree\nhas submodule difference, but \"git diff --raw\" didn't correctly do so.\n\nThere was a mistake in the diffcore_skip_stat_unmatch() that was meant to\nclean up the stat-only difference for running diff between the index and\nwork tree and diff between the tree and the work tree, to cause it re-read\nfrom the submodule repository HEAD.  When ce_stat_match() says work tree\nis different, we should always say 0{40} on the work tree side.\n\nThis patch fixes the issue, and adds tests.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The real change is a net deletion of 3 lines.\n\n diff.c                    |    7 +----\n t/t4027-diff-submodule.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 5 deletions(-)\n create mode 100755 t/t4027-diff-submodule.sh\n\ndiff --git a/diff.c b/diff.c\nindex b80f656..00e1590 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3187,11 +3187,8 @@ static void diffcore_apply_filter(const char *filter)\n static int diff_filespec_is_identical(struct diff_filespec *one,\n \t\t\t\t      struct diff_filespec *two)\n {\n-\tif (S_ISGITLINK(one->mode)) {\n-\t\tdiff_fill_sha1_info(one);\n-\t\tdiff_fill_sha1_info(two);\n-\t\treturn !hashcmp(one->sha1, two->sha1);\n-\t}\n+\tif (S_ISGITLINK(one->mode))\n+\t\treturn 0;\n \tif (diff_populate_filespec(one, 0))\n \t\treturn 0;\n \tif (diff_populate_filespec(two, 0))\ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nnew file mode 100755\nindex 0000000..3d2d081\n--- /dev/null\n+++ b/t/t4027-diff-submodule.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+test_description='difference in submodules'\n+\n+. ./test-lib.sh\n+. ../diff-lib.sh\n+\n+_z40=0000000000000000000000000000000000000000\n+test_expect_success setup '\n+\ttest_tick &&\n+\ttest_create_repo sub &&\n+\t(\n+\t\tcd sub &&\n+\t\techo hello >world &&\n+\t\tgit add world &&\n+\t\tgit commit -m submodule\n+\t) &&\n+\n+\ttest_tick &&\n+\techo frotz >nitfol &&\n+\tgit add nitfol sub &&\n+\tgit commit -m superproject &&\n+\n+\t(\n+\t\tcd sub &&\n+\t\techo goodbye >world &&\n+\t\tgit add world &&\n+\t\tgit commit -m \"submodule #2\"\n+\t) &&\n+\n+\tset x $(\n+\t\tcd sub &&\n+\t\tgit rev-list HEAD\n+\t) &&\n+\techo \":160000 160000 $3 $_z40 M\tsub\" >expect\n+'\n+\n+test_expect_success 'git diff --raw HEAD' '\n+\tgit diff --raw --abbrev=40 HEAD >actual &&\n+\tdiff -u expect actual\n+'\n+\n+test_expect_success 'git diff-index --raw HEAD' '\n+\tgit diff-index --raw HEAD >actual.index &&\n+\tdiff -u expect actual.index\n+'\n+\n+test_expect_success 'git diff-files --raw' '\n+\tgit diff-files --raw >actual.files &&\n+\tdiff -u expect actual.files\n+'\n+\n+test_done\n-- \n1.5.4.3.468.g36990\n\n"},{"id":"70614","messageId":"1204451012-17487-4-git-send-email-gitster@pobox.com","threadId":"12422","inReplyTo":"1204451012-17487-3-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] diff: show submodule object name when available even on the work tree side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T09:43:32Z","receivedAt":"2008-03-02T09:43:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Traditionally, we consistently showed 0{40} when work tree side was\ndifferent from what it was being compared against.  This was primarily\nbecause it did not make sense to re-hash potentially large blobs every\ntime diff-files and non-cached diff-index were asked for.\n\nHowever, we can afford to read from submodule HEAD, as it is much cheaper\nthan hashing blobs.\n\nThis makes the output somewhat inconsistent, but it probably is a good\nmove to give easily available information than not giving it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This could be enhanced to also hash symlink blobs as they tend to be\n   very short.\n\n   HOWEVER.\n\n   This changes the semantics established since almost day-one of git (at\n   least this 0{40} convention can be traced back to show-diff as of Apr\n   26, 2005), and tests (e.g. t3040) demonstrate that it breaks existing\n   callers such as git-commit.\n\n   Which means that I am sending this out only for discussion at this\n   time, not for inclusion in the near term.\n\n diff-lib.c                |   32 +++++++++++++++++++++++++++++++-\n t/t4027-diff-submodule.sh |    2 +-\n 2 files changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 4581b59..94e17a6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -10,6 +10,26 @@\n #include \"cache-tree.h\"\n #include \"path-list.h\"\n #include \"unpack-trees.h\"\n+#include \"refs.h\"\n+\n+/*\n+ * Traditionally, we have _always_ showed 0{40} on the work tree\n+ * side if there is a difference.  However, reading from HEAD of\n+ * a submodule is much cheaper than rehashing regular blob objects.\n+ */\n+static const unsigned char *get_gitlink_data(const char *path)\n+{\n+\tstatic unsigned char subhead[20];\n+\t/*\n+\t * NOTE: strictly speaking, this makes it non re-entrant, but\n+\t * we know the nature of the callchain makes it very\n+\t * short-lived --- the caller will give this to\n+\t * diff_addremove() or diff_change() immediately.\n+\t */\n+\tif (!resolve_gitlink_ref(path, \"HEAD\", subhead))\n+\t\treturn subhead;\n+\treturn null_sha1;\n+}\n \n /*\n  * diff-files\n@@ -349,6 +369,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tstruct stat st;\n \t\tunsigned int oldmode, newmode;\n \t\tstruct cache_entry *ce = active_cache[i];\n+\t\tconst unsigned char *worktree_sha1;\n \t\tint changed;\n \n \t\tif (DIFF_OPT_TST(&revs->diffopt, QUIET) &&\n@@ -454,8 +475,15 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tcontinue;\n \t\toldmode = ce->ce_mode;\n \t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n+\n+\t\tif (S_ISGITLINK(newmode))\n+\t\t\tworktree_sha1 = get_gitlink_data(ce->name);\n+\t\telse if (changed)\n+\t\t\tworktree_sha1 = null_sha1;\n+\t\telse\n+\t\t\tworktree_sha1 = ce->sha1;\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n-\t\t\t    ce->sha1, (changed ? null_sha1 : ce->sha1),\n+\t\t\t    ce->sha1, worktree_sha1,\n \t\t\t    ce->name, NULL);\n \n \t}\n@@ -501,6 +529,8 @@ static int get_stat_data(struct cache_entry *ce,\n \t\tif (changed) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\tsha1 = null_sha1;\n+\t\t\tif (S_ISGITLINK(mode))\n+\t\t\t\tsha1 = get_gitlink_data(ce->name);\n \t\t}\n \t}\n \ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex 3d2d081..c1c2500 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -32,7 +32,7 @@ test_expect_success setup '\n \t\tcd sub &&\n \t\tgit rev-list HEAD\n \t) &&\n-\techo \":160000 160000 $3 $_z40 M\tsub\" >expect\n+\techo \":160000 160000 $3 $2 M\tsub\" >expect\n '\n \n test_expect_success 'git diff --raw HEAD' '\n-- \n1.5.4.3.468.g36990\n\n"},{"id":"70630","messageId":"46dff0320803020241l29277bd4m1a711ff0a863e7f8@mail.gmail.com","threadId":"12422","inReplyTo":"1204451012-17487-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/3] fix \"diff --raw\" on the work tree side","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-03-02T10:41:08Z","receivedAt":"2008-03-02T10:41:08Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Sun, Mar 2, 2008 at 5:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> The second patch fixes the inconsistency between \"git-diff --raw\" and\n>  \"git-diff-{index,files} --raw\" when they are used for submodules.\n>\n>  The third one is a bit controversial and changes the semantics and\n>  existing callers and won't be considered for 1.5.5.\n>\nUnfortunately, my submodule summary patch series will depend on\ngit-diff or git-diff-index. So should i resend my improved patch only\nafter the thrid one is applied?\n\n\n-- \nPing Yin\n"},{"id":"70668","messageId":"7v7igloxtu.fsf@gitster.siamese.dyndns.org","threadId":"12422","inReplyTo":"46dff0320803020241l29277bd4m1a711ff0a863e7f8@mail.gmail.com","subject":"Re: [PATCH 0/3] fix \"diff --raw\" on the work tree side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T17:11:25Z","receivedAt":"2008-03-02T17:11:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ping Yin\" <pkufranky@gmail.com> writes:\n\n> On Sun, Mar 2, 2008 at 5:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> The second patch fixes the inconsistency between \"git-diff --raw\" and\n>>  \"git-diff-{index,files} --raw\" when they are used for submodules.\n>>\n>>  The third one is a bit controversial and changes the semantics and\n>>  existing callers and won't be considered for 1.5.5.\n>>\n> Unfortunately, my submodule summary patch series will depend on\n> git-diff or git-diff-index. So should i resend my improved patch only\n> after the thrid one is applied?\n\nI do not think re-introducing the inconsistency like the third one does is\na palatable option.\n\nWouldn't grabbing (cd $submodule && git rev-parse HEAD) yourself be more\nportable across git before and after the bugfix of \"git diff --raw\"?\n"},{"id":"70670","messageId":"46dff0320803020915u565ce50eraa663f9e5a795b81@mail.gmail.com","threadId":"12422","inReplyTo":"7v7igloxtu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 0/3] fix \"diff --raw\" on the work tree side","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-03-02T17:15:55Z","receivedAt":"2008-03-02T17:15:55Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On 3/3/08, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Ping Yin\" <pkufranky@gmail.com> writes:\n>\n> > On Sun, Mar 2, 2008 at 5:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> The second patch fixes the inconsistency between \"git-diff --raw\" and\n> >>  \"git-diff-{index,files} --raw\" when they are used for submodules.\n> >>\n> >>  The third one is a bit controversial and changes the semantics and\n> >>  existing callers and won't be considered for 1.5.5.\n> >>\n> > Unfortunately, my submodule summary patch series will depend on\n> > git-diff or git-diff-index. So should i resend my improved patch only\n> > after the thrid one is applied?\n>\n> I do not think re-introducing the inconsistency like the third one does is\n> a palatable option.\n>\n> Wouldn't grabbing (cd $submodule && git rev-parse HEAD) yourself be more\n> portable across git before and after the bugfix of \"git diff --raw\"?\n>\n\nOK, i will parse it myself\n\n\n-- \nPing Yin\n"},{"id":"70674","messageId":"7vlk51nhkd.fsf@gitster.siamese.dyndns.org","threadId":"12422","inReplyTo":"46dff0320803020915u565ce50eraa663f9e5a795b81@mail.gmail.com","subject":"Re: [PATCH 0/3] fix \"diff --raw\" on the work tree side","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T17:48:02Z","receivedAt":"2008-03-02T17:48:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ping Yin\" <pkufranky@gmail.com> writes:\n\n> On 3/3/08, Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> I do not think re-introducing the inconsistency like the third one does is\n>> a palatable option.\n>>\n>> Wouldn't grabbing (cd $submodule && git rev-parse HEAD) yourself be more\n>> portable across git before and after the bugfix of \"git diff --raw\"?\n>\n> OK, i will parse it myself\n\nJust to clarify, I am somewhat torn on [3/3].\n\nWe can certainly re-introduce the special casing for submodule across diff\nfamily consistently, but the rationale for doing that would be \"The real\nobject name is very cheaply available in that case so why not show it.\"\n\nThat line of thinking would lead to re-hashing of symbolic link blobs as I\nhinted in the message, but then it would also mean we would show\ninconsistent output between a symlink that points at foo.c and a regular\nfile whose content is foo.c (without the trailing LF), the latter of which\nwould still show 0{40}, and we will eventually end up saying \"Let's\nre-hash small regular file blobs.\"\n\nThat would lead to inconsistencies between smaller and larger regular file\nblobs, and the line between them is drawn at an arbitrary size limit.  The\nlogical conclusion of this would be to re-hash everything when showing\n\"diff --raw\" (and \"diff-anything --raw\").\n\nWhich may turn out not to be such a bad thing after all, and we might even\nend up doing exactly that in future releases, although I highly doubt it.\nIn any case, such a huge semantics change takes time to get right.\nPeople's scripts in the wild know 0{40} with non 0 mode in the raw format\nmeans it refers to an entity in the work tree (which is a correct thing to\nassume), but the convention has been used for the entire lifetime of git\nand they can (arguably incorrectly) be assuming that the inverse is also\ntrue (i.e. work tree entity is represented as 0{40} with non 0 mode).\n\nAnd the point is that \"submodule summary\" can be useful without waiting\nfor the conclusion of the above confusion caused by the can of worms [3/3]\nopens.  If we decide to always show the real object name for work tree\nentities in the future, we might want to update the implementation of\n\"submodule summary\" to _take advantage of it_, but by starting the new\nfeature not to depend on the current misbehaviour, we can try it out much\nearlier.\n\n"}]}