{"thread":{"id":"35879","subject":"git diff, external diff tool, and submodules","startedAt":"2014-02-15T12:19:45Z","lastAt":"2014-02-24T17:39:21Z","messageCount":6,"participants":["Grégory Pakosz","Thomas Rast","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"234853","messageId":"CAC_01E0Pu+_UeSniFVhaqfu90d=iaFDqLrKZ1zjM6GMA4BvcGQ@mail.gmail.com","threadId":"35879","inReplyTo":null,"subject":"git diff, external diff tool, and submodules","fromName":"Grégory Pakosz","fromEmail":"gregory.pakosz@gmail.com","sentAt":"2014-02-15T12:19:45Z","receivedAt":"2014-02-15T12:19:45Z","isPatch":false,"sender":{"key":"gregory.pakosz@gmail.com","avatar":null},"body":"Hello,\n\nWhen a submodule has new commits, I noticed the following:\n\n$ git diff\ndiff --git a/submodule b/submodule\nindex 4c75be6..b272d40 160000\n--- a/submodule\n+++ b/submodule\n@@ -1 +1 @@\n-Subproject commit 4c75be6435cd515887d35c300ed8b487f8143d8e\n+Subproject commit b272d4077fda29028c0bd02efba2837e12a8319c\n\nAs you can see, the diff shows the submodule has new commits.\n\nHowever when using an external diff tool, it seems to me that git diff\nfails to handle the submodule case:\n\n$ GIT_EXTERNAL_DIFF=echo git diff\nsubmodule /var/folders/1h/q9nt7m6d2fs61_v177kq1_h00000gn/T//cz1ati_submodule\n4c75be6435cd515887d35c300ed8b487f8143d8e 160000 submodule\n0000000000000000000000000000000000000000 160000\n\n$LOCAL is set to a temp file that contains:\nSubproject commit 4c75be6435cd515887d35c300ed8b487f8143d8e\n\nAnd I expected $REMOTE to be set to another temp file that contains:\nSubproject commit b272d4077fda29028c0bd02efba2837e12a8319c\n\nInstead, $REMOTE is set to the actual submodule path and then visual\ndiff tools rightfully complains $REMOTE doesn't point to a valid file.\n\nI think git diff should handle the submodule case and create 2\ntemporary files containing \"Subproject commit sha1\" for external diff\ntools to compare.\n\nWhat do you think?\nGregory\n\nPS: git difftool has a special \"directory mode\" triggered with \"-d\"\nthat does what git diff does when GIT_EXTERNAL_DIFF is not set. It\ncreates temp files with \"Subproject commit sha1\" lines for diff tools\nto compare.\n"},{"id":"234901","messageId":"d08b7e5a36ee13226d1ad56a731016762ae89938.1392569505.git.tr@thomasrast.ch","threadId":"35879","inReplyTo":"CAC_01E0Pu+_UeSniFVhaqfu90d=iaFDqLrKZ1zjM6GMA4BvcGQ@mail.gmail.com","subject":"[PATCH] diff: do not reuse_worktree_file for submodules","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-02-16T16:52:34Z","receivedAt":"2014-02-16T16:52:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The GIT_EXTERNAL_DIFF calling code attempts to reuse existing worktree\nfiles for the worktree side of diffs, for performance reasons.\nHowever, that code also tries to do the same with submodules.  This\nresults in calls to $GIT_EXTERNAL_DIFF where the old-file is a file of\nthe form \"Submodule commit $sha1\", but the new-file is a directory in\nthe worktree.\n\nFix it by never reusing a worktree \"file\" in the submodule case.\n\nReported-by: Grégory Pakosz <gregory.pakosz@gmail.com>\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n diff.c                   |  5 +++--\n t/t4020-diff-external.sh | 30 +++++++++++++++++++++++++++++-\n 2 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7c59bfe..e9a8874 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\tremove_tempfile_installed = 1;\n \t}\n \n-\tif (!one->sha1_valid ||\n-\t    reuse_worktree_file(name, one->sha1, 1)) {\n+\tif (!S_ISGITLINK(one->mode) &&\n+\t    (!one->sha1_valid ||\n+\t     reuse_worktree_file(name, one->sha1, 1))) {\n \t\tstruct stat st;\n \t\tif (lstat(name, &st) < 0) {\n \t\t\tif (errno == ENOENT)\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex bcae35a..0446201 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -226,12 +226,13 @@ keep_only_cr () {\n }\n \n test_expect_success 'external diff with autocrlf = true' '\n-\tgit config core.autocrlf true &&\n+\ttest_config core.autocrlf true &&\n \tGIT_EXTERNAL_DIFF=./fake-diff.sh git diff &&\n \ttest $(wc -l < crlfed.txt) = $(cat crlfed.txt | keep_only_cr | wc -c)\n '\n \n test_expect_success 'diff --cached' '\n+\ttest_config core.autocrlf true &&\n \tgit add file &&\n \tgit update-index --assume-unchanged file &&\n \techo second >file &&\n@@ -239,4 +240,31 @@ test_expect_success 'diff --cached' '\n \ttest_cmp \"$TEST_DIRECTORY\"/t4020/diff.NUL actual\n '\n \n+test_expect_success 'clean up crlf leftovers' '\n+\tgit update-index --no-assume-unchanged file &&\n+\trm -f file* &&\n+\tgit reset --hard\n+'\n+\n+test_expect_success 'submodule diff' '\n+\tgit init sub &&\n+\t( cd sub && test_commit sub1 ) &&\n+\tgit add sub &&\n+\ttest_tick &&\n+\tgit commit -m \"add submodule\" &&\n+\t( cd sub && test_commit sub2 ) &&\n+\twrite_script gather_pre_post.sh <<-\\EOF &&\n+\techo \"$1 $4\" # path, mode\n+\tcat \"$2\" # old file\n+\tcat \"$5\" # new file\n+\tEOF\n+\tGIT_EXTERNAL_DIFF=./gather_pre_post.sh git diff >actual &&\n+\tcat >expected <<-EOF &&\n+\tsub 160000\n+\tSubproject commit $(git rev-parse HEAD:sub)\n+\tSubproject commit $(cd sub && git rev-parse HEAD)\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.9.0.313.g3d0a325\n"},{"id":"235018","messageId":"xmqqy518cezh.fsf@gitster.dls.corp.google.com","threadId":"35879","inReplyTo":"d08b7e5a36ee13226d1ad56a731016762ae89938.1392569505.git.tr@thomasrast.ch","subject":"Re: [PATCH] diff: do not reuse_worktree_file for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-18T21:01:22Z","receivedAt":"2014-02-18T21:01:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> The GIT_EXTERNAL_DIFF calling code attempts to reuse existing worktree\n> files for the worktree side of diffs, for performance reasons.\n> However, that code also tries to do the same with submodules.  This\n> results in calls to $GIT_EXTERNAL_DIFF where the old-file is a file of\n> the form \"Submodule commit $sha1\", but the new-file is a directory in\n> the worktree.\n>\n> Fix it by never reusing a worktree \"file\" in the submodule case.\n>\n> Reported-by: Grégory Pakosz <gregory.pakosz@gmail.com>\n> Signed-off-by: Thomas Rast <tr@thomasrast.ch>\n> ---\n>  diff.c                   |  5 +++--\n>  t/t4020-diff-external.sh | 30 +++++++++++++++++++++++++++++-\n>  2 files changed, 32 insertions(+), 3 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 7c59bfe..e9a8874 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n>  \t\tremove_tempfile_installed = 1;\n>  \t}\n>  \n> -\tif (!one->sha1_valid ||\n> -\t    reuse_worktree_file(name, one->sha1, 1)) {\n> +\tif (!S_ISGITLINK(one->mode) &&\n> +\t    (!one->sha1_valid ||\n> +\t     reuse_worktree_file(name, one->sha1, 1))) {\n\nI agree with the goal/end result, but I have to wonder if the\nreuse_worktree_file() be the helper function that ought to\nencapsulate such a logic?\n\nInstead of feeding it an object name and a path, if we passed a\ndiff_filespec to the helper, it would have access to the mode as\nwell.  It would result in a more intrusive change, so I'd prefer to\nsee your patch applied first and then build such a refactor on top,\nperhaps like the attached.\n\n diff.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a96992a..74eec80 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2582,11 +2582,13 @@ void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,\n  * the work tree has that object contents, return true, so that\n  * prepare_temp_file() does not have to inflate and extract.\n  */\n-static int reuse_worktree_file(const char *name, const unsigned char *sha1, int want_file)\n+static int reuse_worktree_file(const struct diff_filespec *spec, int want_file)\n {\n \tconst struct cache_entry *ce;\n \tstruct stat st;\n \tint pos, len;\n+\tconst char *name = spec->path;\n+\tconst unsigned char *sha1 = spec->sha1;\n \n \t/*\n \t * We do not read the cache ourselves here, because the\n@@ -2698,7 +2700,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\treturn diff_populate_gitlink(s, size_only);\n \n \tif (!s->sha1_valid ||\n-\t    reuse_worktree_file(s->path, s->sha1, 0)) {\n+\t    reuse_worktree_file(s, 0)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct stat st;\n \t\tint fd;\n@@ -2844,17 +2846,17 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \n \tif (!S_ISGITLINK(one->mode) &&\n \t    (!one->sha1_valid ||\n-\t     reuse_worktree_file(name, one->sha1, 1))) {\n+\t     reuse_worktree_file(one, 1))) {\n \t\tstruct stat st;\n-\t\tif (lstat(name, &st) < 0) {\n+\t\tif (lstat(one->path, &st) < 0) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\tgoto not_a_valid_file;\n-\t\t\tdie_errno(\"stat(%s)\", name);\n+\t\t\tdie_errno(\"stat(%s)\", one->path);\n \t\t}\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tif (strbuf_readlink(&sb, name, st.st_size) < 0)\n-\t\t\t\tdie_errno(\"readlink(%s)\", name);\n+\t\t\tif (strbuf_readlink(&sb, one->path, st.st_size) < 0)\n+\t\t\t\tdie_errno(\"readlink(%s)\", one->path);\n \t\t\tprep_temp_blob(name, temp, sb.buf, sb.len,\n \t\t\t\t       (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n@@ -2864,7 +2866,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t}\n \t\telse {\n \t\t\t/* we can borrow from the file in the work tree */\n-\t\t\ttemp->name = name;\n+\t\t\ttemp->name = one->path;\n \t\t\tif (!one->sha1_valid)\n \t\t\t\tstrcpy(temp->hex, sha1_to_hex(null_sha1));\n \t\t\telse\n"},{"id":"235187","messageId":"8738jbtmji.fsf@thomasrast.ch","threadId":"35879","inReplyTo":"xmqqy518cezh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: do not reuse_worktree_file for submodules","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-02-22T11:27:29Z","receivedAt":"2014-02-22T11:27:29Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <tr@thomasrast.ch> writes:\n>\n>> @@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n>>  \t\tremove_tempfile_installed = 1;\n>>  \t}\n>>  \n>> -\tif (!one->sha1_valid ||\n>> -\t    reuse_worktree_file(name, one->sha1, 1)) {\n>> +\tif (!S_ISGITLINK(one->mode) &&\n>> +\t    (!one->sha1_valid ||\n>> +\t     reuse_worktree_file(name, one->sha1, 1))) {\n>\n> I agree with the goal/end result, but I have to wonder if the\n> reuse_worktree_file() be the helper function that ought to\n> encapsulate such a logic?\n>\n> Instead of feeding it an object name and a path, if we passed a\n> diff_filespec to the helper, it would have access to the mode as\n> well.  It would result in a more intrusive change, so I'd prefer to\n> see your patch applied first and then build such a refactor on top,\n> perhaps like the attached.\n\nI see that you already queued 721e727, which has the change you\ndescribed plus moving the S_ISGITLINK test into reuse_worktree_file.\nThe change looks good to me.  However, two nits about the comments:\ndiff.c now says\n\n  /*\n   * Given a name and sha1 pair, if the index tells us the file in\n   * the work tree has that object contents, return true, so that\n   * prepare_temp_file() does not have to inflate and extract.\n   */\n  static int reuse_worktree_file(const struct diff_filespec *spec, int want_file)\n  {\n          const struct cache_entry *ce;\n          struct stat st;\n          int pos, len;\n          const char *name = spec->path;\n          const unsigned char *sha1 = spec->sha1;\n\n          /* reading the directory will not give us \"Submodule commit XYZ\" */\n          if (S_ISGITLINK(spec->mode))\n                  return 0;\n\nBut the function comment is no longer accurate, and the comment about\nthe S_ISGITLINK exit is rather obscure if one doesn't know what the\ncallers want.  So how about this on top?\n\n diff.c | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git i/diff.c w/diff.c\nindex a342ea6..dabf913 100644\n--- i/diff.c\n+++ w/diff.c\n@@ -2578,9 +2578,14 @@ void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,\n }\n \n /*\n- * Given a name and sha1 pair, if the index tells us the file in\n- * the work tree has that object contents, return true, so that\n- * prepare_temp_file() does not have to inflate and extract.\n+ * Given a diff_filespec, determine if the corresponding worktree file\n+ * can be used for diffing instead of reading the object from the\n+ * repository.\n+ *\n+ * We normally try packfiles, worktree, loose objects in this order.\n+ *\n+ * If want_file=1 or git was compiled with NO_FAST_WORKING_DIRECTORY,\n+ * the order is: worktree, packfiles, loose objects.\n  */\n static int reuse_worktree_file(const struct diff_filespec *spec, int want_file)\n {\n@@ -2590,7 +2595,11 @@ static int reuse_worktree_file(const struct diff_filespec *spec, int want_file)\n \tconst char *name = spec->path;\n \tconst unsigned char *sha1 = spec->sha1;\n \n-\t/* reading the directory will not give us \"Submodule commit XYZ\" */\n+\t/*\n+\t * The diff representation of a submodule is \"Submodule commit\n+\t * XYZ\", but in the worktree we have a directory.  So they\n+\t * never match.\n+\t */\n \tif (S_ISGITLINK(spec->mode))\n \t\treturn 0;\n \n\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"235204","messageId":"87bnxyq9n6.fsf@thomasrast.ch","threadId":"35879","inReplyTo":"8738jbtmji.fsf@thomasrast.ch","subject":"Re: [PATCH] diff: do not reuse_worktree_file for submodules","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-02-23T12:46:37Z","receivedAt":"2014-02-23T12:46:37Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Thomas Rast <tr@thomasrast.ch> writes:\n>>\n>>> @@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n>>>  \t\tremove_tempfile_installed = 1;\n>>>  \t}\n>>>  \n>>> -\tif (!one->sha1_valid ||\n>>> -\t    reuse_worktree_file(name, one->sha1, 1)) {\n>>> +\tif (!S_ISGITLINK(one->mode) &&\n>>> +\t    (!one->sha1_valid ||\n>>> +\t     reuse_worktree_file(name, one->sha1, 1))) {\n>>\n>> I agree with the goal/end result, but I have to wonder if the\n>> reuse_worktree_file() be the helper function that ought to\n>> encapsulate such a logic?\n>>\n>> Instead of feeding it an object name and a path, if we passed a\n>> diff_filespec to the helper, it would have access to the mode as\n>> well.  It would result in a more intrusive change, so I'd prefer to\n>> see your patch applied first and then build such a refactor on top,\n>> perhaps like the attached.\n>\n> I see that you already queued 721e727, which has the change you\n> described plus moving the S_ISGITLINK test into reuse_worktree_file.\n> The change looks good to me.\n\nI spoke too soon; it breaks the test I wrote to cover this case, for a\nreason that gives me a headache.\n\nWhen we hit the conditional\n\n>>> -\tif (!one->sha1_valid ||\n>>> -\t    reuse_worktree_file(name, one->sha1, 1)) {\n>>> +\tif (!S_ISGITLINK(one->mode) &&\n>>> +\t    (!one->sha1_valid ||\n>>> +\t     reuse_worktree_file(name, one->sha1, 1))) {\n\nsha1_valid=0 for the submodule on the worktree side of the diff.  The\nreason is that we start out with sha1_valid=0 and sha1=000..000 for the\nworktree side of all dirty entries, which makes sense at that point.  We\nlater set the sha1 by looking inside the submodule in\ndiff_fill_sha1_info(), but we never set sha1_valid.  So the above\nconditional will now trigger on the !one->sha1_valid arm, completely\ndefeating the change to reuse_worktree_file().\n\nWe can fix it like below, but it feels a bit wrong to me.  Are\nsubmodules the only case where it makes sense to set sha1_valid when we\nfill the sha1?\n\n\n diff.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git i/diff.c w/diff.c\nindex dabf913..cf7281d 100644\n--- i/diff.c\n+++ w/diff.c\n@@ -3081,6 +3082,8 @@ static void diff_fill_sha1_info(struct diff_filespec *one)\n \t\t\t\tdie_errno(\"stat '%s'\", one->path);\n \t\t\tif (index_path(one->sha1, one->path, &st, 0))\n \t\t\t\tdie(\"cannot hash %s\", one->path);\n+\t\t\tif (S_ISGITLINK(one->mode))\n+\t\t\t\tone->sha1_valid = 1;\n \t\t}\n \t}\n \telse\n\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"235270","messageId":"xmqq4n3obeba.fsf@gitster.dls.corp.google.com","threadId":"35879","inReplyTo":"87bnxyq9n6.fsf@thomasrast.ch","subject":"Re: [PATCH] diff: do not reuse_worktree_file for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T17:39:21Z","receivedAt":"2014-02-24T17:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> I spoke too soon; it breaks the test I wrote to cover this case, for a\n> reason that gives me a headache.\n>\n> When we hit the conditional\n>\n>>>> -\tif (!one->sha1_valid ||\n>>>> -\t    reuse_worktree_file(name, one->sha1, 1)) {\n>>>> +\tif (!S_ISGITLINK(one->mode) &&\n>>>> +\t    (!one->sha1_valid ||\n>>>> +\t     reuse_worktree_file(name, one->sha1, 1))) {\n>\n> sha1_valid=0 for the submodule on the worktree side of the diff.  The\n> reason is that we start out with sha1_valid=0 and sha1=000..000 for the\n> worktree side of all dirty entries, which makes sense at that point.  We\n> later set the sha1 by looking inside the submodule in\n> diff_fill_sha1_info(), but we never set sha1_valid.  So the above\n> conditional will now trigger on the !one->sha1_valid arm, completely\n> defeating the change to reuse_worktree_file().\n>\n> We can fix it like below, but it feels a bit wrong to me.  Are\n> submodules the only case where it makes sense to set sha1_valid when we\n> fill the sha1?\n\nThe meaning of filespec->sha1_valid is \"Is it known that the\nfilespec->sha1 and filespec->mode field should be used?\"; I agree\nthat this feels wrong.\n\nWhich means that the previous one was wrong, and your original was\nthe right approach.  I'll drop the update.\n\nThanks.\n\n>\n>  diff.c | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git i/diff.c w/diff.c\n> index dabf913..cf7281d 100644\n> --- i/diff.c\n> +++ w/diff.c\n> @@ -3081,6 +3082,8 @@ static void diff_fill_sha1_info(struct diff_filespec *one)\n>  \t\t\t\tdie_errno(\"stat '%s'\", one->path);\n>  \t\t\tif (index_path(one->sha1, one->path, &st, 0))\n>  \t\t\t\tdie(\"cannot hash %s\", one->path);\n> +\t\t\tif (S_ISGITLINK(one->mode))\n> +\t\t\t\tone->sha1_valid = 1;\n>  \t\t}\n>  \t}\n>  \telse\n"}]}