{"thread":{"id":"22176","subject":"[RFC PATCH (WIP)] Show a dirty working tree and a detached HEAD in status for submodule","startedAt":"2010-01-11T22:05:10Z","lastAt":"2010-01-17T20:33:54Z","messageCount":14,"participants":["Jens Lehmann","Junio C Hamano","Nanako Shiraishi"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"131321","messageId":"4B4BA096.5000909@web.de","threadId":"22176","inReplyTo":null,"subject":"[RFC PATCH (WIP)] Show a dirty working tree and a detached HEAD in status for submodule","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-11T22:05:10Z","receivedAt":"2010-01-11T22:05:10Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Until now a submodule only showed up as changed in the supermodule when\nthe last commit in the submodule differed from the one in the index or\nthe last commit of the superproject. A dirty working tree or a detached\nHEAD in a submodule were just ignored when looking at it from the\nsuperproject.\n\nThis patch shows these changes when using git status or one of the diff\ncommands which compare against the working tree in the superproject.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nThis is the first version of a patch letting git status and the git\ndiff family show dirty working directories and a detached HEAD in\ndirectories. It is not intended to be merged in its present form but\nto be used as a starting point for discussion if this is going in\nthe right direction.\n\n\nWhat the patch does:\n\n* It makes git show submodules as modified in the superproject when\n  one or more of these conditions are met:\n\n    a) The submodule contains untracked files\n    b) The submodule contains modified files\n    c) The submodules HEAD is not on a local or remote branch\n\n  That can be seen when using either \"git status\", \"git diff[-files]\"\n  & \"git diff[-index] HEAD\" (and with \"git gui\" & gitk).\n\n\nWhat the patch doesn't do (yet):\n\n* It still breaks tests t7400-submodule-basic.sh &\n  t7407-submodule-foreach.sh.\n\n* It doesn't give detailed output when doing a \"git diff* -p\" with or\n  without the --submodule option. It should show something like\n\n    diff --git a/sub b/sub\n    index 5431f52..3f35670 160000\n    --- a/sub\n    +++ b/sub\n    @@ -1 +1 @@\n    -Subproject commit 5431f529197f3831cdfbba1354a819a79f948f6f\n    +Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty\n\n  for \"git diff* -p\" (notice the \"-dirty\" in the last line) or\n\n      Submodule sub contains untracked files\n      Submodule sub contains modified files\n      Submodule sub contains a HEAD not on any branch\n      Submodule sub 5431f52..3f35670:\n      > commit message 1\n\n  when using the --submodule option of the diff family.\n\n* This behavior is not configurable but activated by default. A config\n  option is needed here.\n\n* It doesn't give optimal performance:\n\n  - Apart from the fact that checking submodules this way will always\n    be slower than ignoring their changes as git does until now, doing\n    two run_command() calls for each submodule is not going to help at\n    all (especially when running on Windows).\n\n  - AFAICS the check for a detached HEAD would be faster for the most\n    probable case if it would check against remotes/origin/master first.\n    And it could stop when the first branch was found instead of\n    continuing to look for others too as \"git branch --contains\" does.\n\n  - Similar for the test for a dirty working directory, no need to have\n    the full list of new and modified files, it could stop at the first\n    one it finds.\n\n  - If no detailed output is wanted the examination of HEAD and the\n    working directory is not necessary when the HEAD and the commit in\n    the index of the superproject already don't match.\n\n\nWhat do you think?\n\n\n\n\n diff-lib.c                  |    7 +++-\n submodule.c                 |  103 +++++++++++++++++++++++++++++++++++++++++++\n submodule.h                 |    1 +\n t/t7506-status-submodule.sh |   45 ++++++++++++++++++-\n 4 files changed, 154 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 1c7e652..323305a 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -159,7 +159,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\tcontinue;\n \t\t}\n\n-\t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n+\t\tif ((ce_uptodate(ce) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n\n \t\t/* If CE_VALID is set, don't look at workdir for file removal */\n@@ -176,6 +176,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tcontinue;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n+\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\t/* TODO: This should not be executed when the submodule is changed\n+\t\t\t * and only short ouptut is wanted for performance reasons. */\n+\t\t\tchanged |= is_submodule_modified(ce->name);\n+\t\t}\n \t\tif (!changed) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\ndiff --git a/submodule.c b/submodule.c\nindex 86aad65..b35f1b3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -4,6 +4,7 @@\n #include \"diff.h\"\n #include \"commit.h\"\n #include \"revision.h\"\n+#include \"run-command.h\"\n\n int add_submodule_odb(const char *path)\n {\n@@ -112,3 +113,105 @@ void show_submodule_summary(FILE *f, const char *path,\n \t}\n \tstrbuf_release(&sb);\n }\n+\n+static int is_submodule_head_detached(const char *path)\n+{\n+\tint retval, len;\n+\tstruct child_process branch;\n+\tconst char *argv[] = {\n+\t\t\"branch\",\n+\t\t\"-a\",\n+\t\t\"--contains\",\n+\t\t\"HEAD\",\n+\t\tNULL,\n+\t};\n+\tchar *env[3];\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf, \"GIT_WORK_TREE=%s\", path);\n+\tenv[0] = strbuf_detach(&buf, NULL);\n+\tstrbuf_addf(&buf, \"GIT_DIR=%s/.git\", path);\n+\tenv[1] = strbuf_detach(&buf, NULL);\n+\tenv[2] = NULL;\n+\n+\tmemset(&branch, 0, sizeof(branch));\n+\tbranch.argv = argv;\n+\tbranch.env = (const char *const *)env;\n+\tbranch.git_cmd = 1;\n+\tbranch.no_stdin = 1;\n+\tbranch.out = -1;\n+\tif (start_command(&branch))\n+\t\tdie(\"Could not run git branch -a --contains HEAD\");\n+\n+\tlen = strbuf_read(&buf, branch.out, 1024);\n+\tclose(branch.out);\n+\n+\tif (finish_command(&branch))\n+\t\tdie(\"git branch -a --contains HEAD failed\");\n+\n+\tretval = (strncmp(buf.buf, \"* (no branch)\", 13) == 0);\n+\n+\tfree(env[0]);\n+\tfree(env[1]);\n+\tstrbuf_release(&buf);\n+\treturn retval;\n+}\n+\n+static int is_submodule_working_directory_dirty(const char *path)\n+{\n+\tint len;\n+\tstruct child_process branch;\n+\tconst char *argv[] = {\n+\t\t\"status\",\n+\t\t\"--porcelain\",\n+\t\tNULL,\n+\t};\n+\tchar *env[3];\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf, \"GIT_WORK_TREE=%s\", path);\n+\tenv[0] = strbuf_detach(&buf, NULL);\n+\tstrbuf_addf(&buf, \"GIT_DIR=%s/.git\", path);\n+\tenv[1] = strbuf_detach(&buf, NULL);\n+\tenv[2] = NULL;\n+\n+\tmemset(&branch, 0, sizeof(branch));\n+\tbranch.argv = argv;\n+\tbranch.env = (const char *const *)env;\n+\tbranch.git_cmd = 1;\n+\tbranch.no_stdin = 1;\n+\tbranch.out = -1;\n+\tif (start_command(&branch))\n+\t\tdie(\"Could not run git status --porcelain\");\n+\n+\tlen = strbuf_read(&buf, branch.out, 1024);\n+\tclose(branch.out);\n+\n+\tif (finish_command(&branch))\n+\t\tdie(\"git status --porcelain failed\");\n+\n+\tfree(env[0]);\n+\tfree(env[1]);\n+\tstrbuf_release(&buf);\n+\treturn len != 0;\n+}\n+\n+int is_submodule_modified(const char *path)\n+{\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buffer, \"%s/.git/\", path);\n+\tif (!is_directory(buffer.buf)) {\n+\t\tstrbuf_release(&buffer);\n+\t\treturn 0;\n+\t}\n+\tstrbuf_release(&buffer);\n+\n+\tif (is_submodule_head_detached(path))\n+\t\treturn 1;\n+\n+\tif (is_submodule_working_directory_dirty(path))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex 4c0269d..0773121 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -4,5 +4,6 @@\n void show_submodule_summary(FILE *f, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n \t\tconst char *del, const char *add, const char *reset);\n+int is_submodule_modified(const char *path);\n\n #endif\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 3ca17ab..509754a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -10,8 +10,12 @@ test_expect_success 'setup' '\n \t: >bar &&\n \tgit add bar &&\n \tgit commit -m \" Add bar\" &&\n+\t: >foo &&\n+\tgit add foo &&\n+\tgit commit -m \" Add foo\" &&\n \tcd .. &&\n-\tgit add sub &&\n+\techo output > .gitignore\n+\tgit add sub .gitignore &&\n \tgit commit -m \"Add submodule sub\"\n '\n\n@@ -23,6 +27,45 @@ test_expect_success 'commit --dry-run -a clean' '\n \tgit commit --dry-run -a |\n \tgrep \"nothing to commit\"\n '\n+\n+echo \"changed\" > sub/foo\n+test_expect_success 'status with modified file in submodule' '\n+\tgit status | grep \"modified:   sub\"\n+'\n+test_expect_success 'status with modified file in submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-EOF\n+ M sub\n+EOF\n+'\n+(cd sub && git checkout foo)\n+\n+echo \"content\" > sub/new-file\n+test_expect_success 'status with untracked file in submodule' '\n+\tgit status | grep \"modified:   sub\"\n+'\n+test_expect_success 'status with untracked file in submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-EOF\n+ M sub\n+EOF\n+'\n+rm sub/new-file\n+\n+(cd sub && 2>/dev/null\n+old_head=$(cat .git/refs/heads/master) &&\n+git reset --hard HEAD^ &&\n+git checkout $old_head 2>/dev/null)\n+test_expect_success 'status with detatched HEAD in submodule' '\n+\tgit status | grep \"modified:   sub\"\n+'\n+test_expect_success 'status with detatched HEAD in submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-EOF\n+ M sub\n+EOF\n+'\n+\n test_expect_success 'rm submodule contents' '\n \trm -rf sub/* sub/.git\n '\n-- \n1.6.6.203.g6b27d.dirty\n"},{"id":"131327","messageId":"7vtyusb6rv.fsf@alter.siamese.dyndns.org","threadId":"22176","inReplyTo":"4B4BA096.5000909@web.de","subject":"Re: [RFC PATCH (WIP)] Show a dirty working tree and a detached HEAD in status for submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T22:45:08Z","receivedAt":"2010-01-11T22:45:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Until now a submodule only showed up as changed in the supermodule when\n> the last commit in the submodule differed from the one in the index or\n> the last commit of the superproject. A dirty working tree or a detached\n> HEAD in a submodule were just ignored when looking at it from the\n> superproject.\n>\n> This patch shows these changes when using git status or one of the diff\n> commands which compare against the working tree in the superproject.\n>\n> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n> ---\n>\n>\n> This is the first version of a patch letting git status and the git\n> diff family show dirty working directories and a detached HEAD in\n> directories. It is not intended to be merged in its present form but\n> to be used as a starting point for discussion if this is going in\n> the right direction.\n>\n>\n> What the patch does:\n>\n> * It makes git show submodules as modified in the superproject when\n>   one or more of these conditions are met:\n>\n>     a) The submodule contains untracked files\n>     b) The submodule contains modified files\n>     c) The submodules HEAD is not on a local or remote branch\n>\n>   That can be seen when using either \"git status\", \"git diff[-files]\"\n>   & \"git diff[-index] HEAD\" (and with \"git gui\" & gitk).\n\nIf the submodule is checked out, _and_ if the HEAD there, either detached\nor not, does not agree with what the \"other\" one records (i.e. the commit\nrecorded in an entry in the index, or in the tree, that you are comparing\nyour work tree against), then it also should be considered modified.  I\ndon't think your (a)-(c) cover this case.\n\nAlso I don't understand why you want to treat (c) any specially at all.\nEven if (c) is something we _should_ report, please do not call that as\n\"detached\" in its implementation.  \"detached HEAD\" has a very precise\ntechnical meaning, and can point at the same commit as a local or a remote\ntracking branch, which is very different from the definition your\nimplementation seems to use.\n\n> * This behavior is not configurable but activated by default. A config\n>   option is needed here.\n\nI doubt it.\n\nMy gut feeling is that this should be _always_ on for a submodule\ndirectory that has been \"submodule init/update\".  The user is interested\nin that particular submodule, and any change to it should be reported for\nboth classes of users.  Theose who meant to use the submodule read-only\nneed to be able to notice that they accidentally made the submodule dirty\nbefore making a commit in the superproject.  Those who wanted to work in\nsubmodule needs to know if the state is in sync with what they expect\nbefore making a commit in the superproject.\n\nThat of course is provided if the unconditional check does not trigger for\nsubmodules that the user hasn't \"submodue init\"ed; I think you did that\ncorrectly at the beginning of your is_submodule_modified() implementation.\n\n> +static int is_submodule_head_detached(const char *path)\n> +{\n\nI don't understand why you should care which branch the submodule happens\nto be on, as long as the next commit you make in the superproject records\nthe commit that is checked out in the submodule.\n\nOf course you may want to be careful when \"pushing\" the superproject\nresults out (i.e. you would want to push out the history leading to that\ncommit at the submodule HEAD in the submodule history), so that the people\nwho are pulling from the repository you are pushing into will have\neverything available.\n\nBut the thing is, in a distributed environment, the submodule HEAD being\nat the tip of _some_ branch (either local or remote) you have doesn't mean\nanything to help them.  IOW, for protect others, you would need a check\nwhen you _push out_ (either in 'push' or on the receiving end).\n\nSo I'd suggest dropping this condition in \"status/diff\" that is about\npreparing to make the next commit in your _local_ history.\n\nIf \"must be reachable from somewhere\" is a condition worth caring about in\nsome context other than \"status/diff\", you can do an equivalent of:\n\n    $ git rev-parse HEAD --not --all | git rev-list --stdin\n\nand see if anything comes out (in which case you have commits that are not\nreachable from any of your refs other than the detached HEAD).\n\nBut that is not \"is HEAD detached?\"; it is something else.  \"Dangling\",\nperhaps, as that is how \"git fsck\" call commits that are not reachable\nfrom any of your refs (\"fsck\" considers HEAD a part of refs, so it is not\nstrictly correct but it is much closer).\n"},{"id":"131385","messageId":"4B4CA13F.6020505@web.de","threadId":"22176","inReplyTo":"7vtyusb6rv.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH (WIP)] Show a dirty working tree and a detached HEAD in status for submodule","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-12T16:20:15Z","receivedAt":"2010-01-12T16:20:15Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 11.01.2010 23:45, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>> * It makes git show submodules as modified in the superproject when\n>>   one or more of these conditions are met:\n>>\n>>     a) The submodule contains untracked files\n>>     b) The submodule contains modified files\n>>     c) The submodules HEAD is not on a local or remote branch\n>>\n>>   That can be seen when using either \"git status\", \"git diff[-files]\"\n>>   & \"git diff[-index] HEAD\" (and with \"git gui\" & gitk).\n> \n> If the submodule is checked out, _and_ if the HEAD there, either detached\n> or not, does not agree with what the \"other\" one records (i.e. the commit\n> recorded in an entry in the index, or in the tree, that you are comparing\n> your work tree against), then it also should be considered modified.  I\n> don't think your (a)-(c) cover this case.\n\nRight, i did not to add the current (and unchanged) behavior to this\nlist, i just wrote down the new cases (and these new cases only come\ninto play when the submodule has been checked out).\n\n\n> Also I don't understand why you want to treat (c) any specially at all.\n\nTo avoid possible loss of commits.\n\nBefore doing something like \"git checkout -f\" or \"git reset --hard\", it\nis a good idea to check via \"git status\" if you have local changes. I\nhope checkout and reset will recurse into submodules in the near future.\nwhen they do, all commits in the submodule which are not on any branch\nare lost (at least when the reflog expired). Or the remote branch the\nuser thinks the submodule is tracking has been deleted or rebased. You\nmight want to know that before e.g. committing it in the superproject.\n\nMaybe compare it to new or modified files in a git repo: They don't\nnecessarily pose a problem when committing, you might be able to push\nand clone the repo somewhere else and nothing breaks. But you wanna\nknow about these new and modifies files, in case you just forgot to add\nthem. So i think the HEAD of a submodule not on any branch is a bit like\na new or modified file in a regular repo, both will not show up in a\ndifferent repo than yours unless you do something about it. And a\nmodification is lost by a checkout or reset just as the dangling commits\nwill be.\n\nYes, this test can't provide 100% safety against loss of commits, but at\nleast we should try to warn if we can detect it. Does it give false\npositives (saying the submodules HEAD is dangling when it shouldn't)?\nI doubt it. Does it give false negatives? Yes, but we can't do anything\nabout that due to the distributed nature of git.\n\n\n> Even if (c) is something we _should_ report, please do not call that as\n> \"detached\" in its implementation.\n\nCorrect, that term is misleading in this context. Maybe call it\nsomething like \"The submodule contains a HEAD not on any branch\"\nthen? Or \"The submodule has a dangling HEAD\"?\n\n\n>> * This behavior is not configurable but activated by default. A config\n>>   option is needed here.\n> \n> I doubt it.\n> \n> My gut feeling is that this should be _always_ on for a submodule\n> directory that has been \"submodule init/update\".  The user is interested\n> in that particular submodule, and any change to it should be reported for\n> both classes of users.  Theose who meant to use the submodule read-only\n> need to be able to notice that they accidentally made the submodule dirty\n> before making a commit in the superproject.  Those who wanted to work in\n> submodule needs to know if the state is in sync with what they expect\n> before making a commit in the superproject.\n\nYes, me too thinks it should default to on for every initialized\nsubmodule.\n\nBut this is a major change in behavior, so it might be a good idea to be\nable to turn it off (e.g. if it breaks scripts). Maybe a config option\nreally isn't such a bright idea, but what about having something like a\n\"--no-dirty-submodules\" command line option?\n\n\n> That of course is provided if the unconditional check does not trigger for\n> submodules that the user hasn't \"submodue init\"ed; I think you did that\n> correctly at the beginning of your is_submodule_modified() implementation.\n\nYes, that's what that test is for. Will add a comment there.\n\n\n> But the thing is, in a distributed environment, the submodule HEAD being\n> at the tip of _some_ branch (either local or remote) you have doesn't mean\n> anything to help them.  IOW, for protect others, you would need a check\n> when you _push out_ (either in 'push' or on the receiving end).\n\nThis is something on my TODO list: Add a change to \"git push\" to assert\nthat all HEADs of initialized submodules lie on a /remote/ branch before\ndoing the push in the superproject.\n\n\n> So I'd suggest dropping this condition in \"status/diff\" that is about\n> preparing to make the next commit in your _local_ history.\n\nI would rather have this patch merged without c) than not at all. But i\nthink it is a worthwhile and rather cheap test. And i would prefer to\nchange the default behavior of \"git status\" only once now and not again\nlater.\n"},{"id":"131438","messageId":"7vbpgyqy4a.fsf@alter.siamese.dyndns.org","threadId":"22176","inReplyTo":"4B4CA13F.6020505@web.de","subject":"Re: [RFC PATCH (WIP)] Show a dirty working tree and a detached HEAD in status for submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T07:09:57Z","receivedAt":"2010-01-13T07:09:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> To avoid possible loss of commits.\n> ... Or the remote branch the\n> user thinks the submodule is tracking has been deleted or rebased. You\n> might want to know that before e.g. committing it in the superproject.\n\nThe interaction between recursive reset/checkout and gc in submodule\nrepository is something I didn't think about thoroughly, but I think you\nare solving the issue in a wrong place at a wrong level.\n\nWe have so far neglected the object reachability analysis across\nrepositories.  When people use \"clone --reference\" to take advantage of\nalternate object store, we tell them not to use any repository that\nrewinds its branches as reference.  Putting it the other way around, we\ntell owners of repositories that somebody borrows objects from not to run\ngc, because objects they themselves do not use may be used by others who\nborrow from them.  But we don't _enforce_ it; it is merely a social\nconvention.  We might want to do something about it, perhaps by having\ntyped back-pointers from a borrowed repository to borrowing repositories.\n\nI think commits in submodules that are pointed at by _some_ superproject\ncommits are in the same situation.  Just like some objects in a borrowed\nrepository may have to be protected by refs in borrowing repositories,\nsome commits in a submodule repository (here I am only talking about\nrepositories that serve as distribution points---a private repository of a\nuser who is not interested in that particular submodule is excluded from\nthis discussion and may not follow the same rules) may have to be\nprotected by commits reachable by refs in its superproject repository.\n\nThe right solution to your \"gc might soon remove objects from submodule\nrepository even when commits in the superproject uses them\" may be to\nteach \"gc\" run in the submodule repository to behave better.  After all,\neven if the commit were reachable from some of the refs in the submodule\nwhen you created a commit that points at it at the superproject level,\nnext \"git submodule update\" might rewind that ref in the submodule that\nreached the commit and make it unreachable; your check to be careful at\ncommit time wouldn't buy you much, even if the check were 100% accurate.\n\nAlso in _your_ repository you may have the submodule commit with your\ncheck at the time you make commit in the superproject, but if you forget\nto push it out from the submodule before you push the commit out from the\nsuperproject, other people will suffer from the same issue you are trying\nto guard against.\n\nIn any case, this \"where in the submodule's history does the commit being\nrecorded lie\" does not have anything to do with \"diff/status\" that lets\nyou know \"has it been _modified_?\" at the level of the superproject.  As\nyou mentioned, it is more similar to \"Untracked files\" section in \"status\"\noutput of a flat project.  It doesn't belong to \"diff\", nor to \"Changed\nbut not updated\" section.\n\n>>> * This behavior is not configurable but activated by default. A config\n>>>   option is needed here.\n>> \n>> I doubt it.\n>> \n>> My gut feeling is that this should be _always_ on for a submodule\n>> directory that has been \"submodule init/update\".  The user is interested\n>> in that particular submodule, and any change to it should be reported for\n>> both classes of users.  Those who meant to use the submodule read-only\n>> need to be able to notice that they accidentally made the submodule dirty\n>> before making a commit in the superproject.  Those who wanted to work in\n>> submodule needs to know if the state is in sync with what they expect\n>> before making a commit in the superproject.\n>\n> Yes, me too thinks it should default to on for every initialized\n> submodule.\n>\n> But this is a major change in behavior, so it might be a good idea to be\n> able to turn it off (e.g. if it breaks scripts).\n\nIf scripts are broken by this change, I think it is actually a good thing.\n\nThey've been operating happily as if everything is clean when some\nsubmodule directories are *not*, and your patch starts showing something\ncloser to the reality, the changes they should care about.  If on the\nother hand the scripts are willing to commit the index while leaving local\nmodifications behind, they will not be checking with \"diff\" output\n(instead they will be checking \"diff --cached\") and you won't change the\noutput with your patch for that codepath, so they will keep working\nhappily.\n\n>> But the thing is, in a distributed environment, the submodule HEAD being\n>> at the tip of _some_ branch (either local or remote) you have doesn't mean\n>> anything to help them.  IOW, for protect others, you would need a check\n>> when you _push out_ (either in 'push' or on the receiving end).\n\nThe right place to do this check is at the receiving end. Just like they\ncan be (and by default are) configured to reject a non ff push into the\nrepository, the receiving end of the superproject can inspect the incoming\nhistory and make sure that the commits bound at submodule paths are all\navailable to others that might want to fetch those superproject commits\nfrom it in associated submodule repositories.  It would forbid people from\nfirst pushing superproject and then pushing submodule projects, but I\nthink that is a better workflow anyway (you make sure prerequisites are\navailable in the submodule repositories to others first, and then make the\nsuperproject commit that depends on the submodules).\n\nYou may also need to check at the receiving end when pushing into the\nsubmodule repository; if the push is a non ff one, it _might_ lose commits\nthat are still needed by the associated superproject.\n\n>> So I'd suggest dropping this condition in \"status/diff\" that is about\n>> preparing to make the next commit in your _local_ history.\n>\n> I would rather have this patch merged without c) than not at all. But i\n> think it is a worthwhile and rather cheap test.\n\nDid I ever say \"drop (c) because it is *too expensive*\"?\n\nI suggested to drop it because it is a _pointless_ check in the context of\nthe codepath, even though I was too polite to put it that bluntly in the\nmessage you are responding to.\n\nI think this topic of yours overall is going in the right direction.  I\nhave no issue with the intent to show submodules with local changes in\n\"Changed but not updated\" section of status, or do the \"+sha1-dirty\" thing\nin diff.\n\nThanks.\n"},{"id":"131508","messageId":"4B4E1817.1070202@web.de","threadId":"22176","inReplyTo":"7vbpgyqy4a.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-13T18:59:35Z","receivedAt":"2010-01-13T18:59:35Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Until now a submodule only then showed up as modified in the supermodule\nwhen the last commit in the submodule differed from the one in the index\nor the diffed against commit of the superproject. A dirty work tree\ncontaining new untracked or modified files in a submodule was\nundetectable when looking at it from the superproject.\n\nNow git status and git diff (against the work tree) in the superproject\nwill also display submodules as modified when they contain untracked or\nmodified files, even if the compared ref matches the HEAD of the\nsubmodule.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\nThanks for your review, here is the updated patch. Changes to the RFC\nversion are:\n\n  - Removed check for a dangling HEAD (now the testsuite runs fine)\n  - Reworded the commit message\n  - Inlined is_submodule_working_directory_dirty() into\n    is_submodule_modified()\n  - The new code will only be called when refs did match (when they\n    didn't the submodule will already show up as modified)\n\nWhat do you think?\n\n\n diff-lib.c                  |    4 ++-\n submodule.c                 |   49 +++++++++++++++++++++++++++++++++++++++++++\n submodule.h                 |    1 +\n t/t7506-status-submodule.sh |   31 ++++++++++++++++++++++++++-\n 4 files changed, 83 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 1c7e652..6918920 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -159,7 +159,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\tcontinue;\n \t\t}\n\n-\t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n+\t\tif ((ce_uptodate(ce) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n\n \t\t/* If CE_VALID is set, don't look at workdir for file removal */\n@@ -176,6 +176,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tcontinue;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n+\t\tif (S_ISGITLINK(ce->ce_mode) && !changed)\n+\t\t\tchanged = is_submodule_modified(ce->name);\n \t\tif (!changed) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\ndiff --git a/submodule.c b/submodule.c\nindex 86aad65..3f851de 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -4,6 +4,7 @@\n #include \"diff.h\"\n #include \"commit.h\"\n #include \"revision.h\"\n+#include \"run-command.h\"\n\n int add_submodule_odb(const char *path)\n {\n@@ -112,3 +113,51 @@ void show_submodule_summary(FILE *f, const char *path,\n \t}\n \tstrbuf_release(&sb);\n }\n+\n+int is_submodule_modified(const char *path)\n+{\n+\tint len;\n+\tstruct child_process cp;\n+\tconst char *argv[] = {\n+\t\t\"status\",\n+\t\t\"--porcelain\",\n+\t\tNULL,\n+\t};\n+\tchar *env[3];\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf, \"%s/.git/\", path);\n+\tif (!is_directory(buf.buf)) {\n+\t\tstrbuf_release(&buf);\n+\t\t/* The submodule is not checked out, so it is not modified */\n+\t\treturn 0;\n+\n+\t}\n+\tstrbuf_reset(&buf);\n+\n+\tstrbuf_addf(&buf, \"GIT_WORK_TREE=%s\", path);\n+\tenv[0] = strbuf_detach(&buf, NULL);\n+\tstrbuf_addf(&buf, \"GIT_DIR=%s/.git\", path);\n+\tenv[1] = strbuf_detach(&buf, NULL);\n+\tenv[2] = NULL;\n+\n+\tmemset(&cp, 0, sizeof(cp));\n+\tcp.argv = argv;\n+\tcp.env = (const char *const *)env;\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.out = -1;\n+\tif (start_command(&cp))\n+\t\tdie(\"Could not run git status --porcelain\");\n+\n+\tlen = strbuf_read(&buf, cp.out, 1024);\n+\tclose(cp.out);\n+\n+\tif (finish_command(&cp))\n+\t\tdie(\"git status --porcelain failed\");\n+\n+\tfree(env[0]);\n+\tfree(env[1]);\n+\tstrbuf_release(&buf);\n+\treturn len != 0;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex 4c0269d..0773121 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -4,5 +4,6 @@\n void show_submodule_summary(FILE *f, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n \t\tconst char *del, const char *add, const char *reset);\n+int is_submodule_modified(const char *path);\n\n #endif\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 3ca17ab..47e205b 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -10,8 +10,12 @@ test_expect_success 'setup' '\n \t: >bar &&\n \tgit add bar &&\n \tgit commit -m \" Add bar\" &&\n+\t: >foo &&\n+\tgit add foo &&\n+\tgit commit -m \" Add foo\" &&\n \tcd .. &&\n-\tgit add sub &&\n+\techo output > .gitignore\n+\tgit add sub .gitignore &&\n \tgit commit -m \"Add submodule sub\"\n '\n\n@@ -23,6 +27,31 @@ test_expect_success 'commit --dry-run -a clean' '\n \tgit commit --dry-run -a |\n \tgrep \"nothing to commit\"\n '\n+\n+echo \"changed\" > sub/foo\n+test_expect_success 'status with modified file in submodule' '\n+\tgit status | grep \"modified:   sub\"\n+'\n+test_expect_success 'status with modified file in submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-EOF\n+ M sub\n+EOF\n+'\n+(cd sub && git checkout foo)\n+\n+echo \"content\" > sub/new-file\n+test_expect_success 'status with untracked file in submodule' '\n+\tgit status | grep \"modified:   sub\"\n+'\n+test_expect_success 'status with untracked file in submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-EOF\n+ M sub\n+EOF\n+'\n+rm sub/new-file\n+\n test_expect_success 'rm submodule contents' '\n \trm -rf sub/* sub/.git\n '\n-- \n1.6.6.203.g28a8ba.dirty\n"},{"id":"131541","messageId":"7v6375lkpj.fsf@alter.siamese.dyndns.org","threadId":"22176","inReplyTo":"4B4E1817.1070202@web.de","subject":"Re: [PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T22:10:48Z","receivedAt":"2010-01-13T22:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Thanks for your review, here is the updated patch. Changes to the RFC\n> version are:\n>\n>   - Removed check for a dangling HEAD (now the testsuite runs fine)\n>   - Reworded the commit message\n>   - Inlined is_submodule_working_directory_dirty() into\n>     is_submodule_modified()\n>   - The new code will only be called when refs did match (when they\n>     didn't the submodule will already show up as modified)\n\nLooking good.\n\nI had to squash in '#include \"submodule.h\"' in diff-lib.c just after it\nincludes \"refs.h\", though.\n\nAnd a patch to add:\n\n>> * It doesn't give detailed output when doing a \"git diff* -p\" with or\n>>   without the --submodule option. It should show something like\n>> \n>>     diff --git a/sub b/sub\n>>     index 5431f52..3f35670 160000\n>>     --- a/sub\n>>     +++ b/sub\n>>     @@ -1 +1 @@\n>>     -Subproject commit 5431f529197f3831cdfbba1354a819a79f948f6f\n>>     +Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty\n>> \n\nwould look like the attached.\n\nI think a reasonable next step would be\n\n - Move the check for your condition (c) that we dropped from this round\n   to wt-status.c;\n\n - Add wt_status_print_dangling_submodules() to wt-status.c, and use the\n   above logic to produce a section \"Submodules with Dangling HEAD\" or\n   something.\n\n - Call it in wt_status_print(), immediately before we check s->verbose\n   and show the patch text under -v option.  \"git status\" now will warn\n   about the condition (c).\n\n - Add a similar wt_shortstatus_print_dangling_submodules() and call it at\n   the end of wt_shortstatus_print().\n\n - Update is_submodule_modified() in your patch thats reads the output\n   from \"status --porcelain\", to *ignore* information about dangling\n   submodules.  As we discussed, dangling submodules may be something the\n   user cares about, but that is not something \"diff\" should.\n\n-- >8 --\nSubject: Teach diff that modified submodule directory is dirty\n\nA diff run in superproject only compares the name of the commit object\nbound at the submodule paths.  When we compare with a work tree and the\nchecked out submodule directory is dirty (e.g. has either staged or\nunstaged changes, or has new files the user forgot to add to the index),\nshow the work tree side as \"dirty\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c                    |    9 ++++++-\n t/t4027-diff-submodule.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 55 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 04beb26..750c066 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2029,9 +2029,14 @@ static int populate_from_stdin(struct diff_filespec *s)\n static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n {\n \tint len;\n-\tchar *data = xmalloc(100);\n+\tchar *data = xmalloc(100), *dirty = \"\";\n+\n+\t/* Are we looking at the work tree? */\n+\tif (!s->sha1_valid && is_submodule_modified(s->path))\n+\t\tdirty = \"-dirty\";\n+\n \tlen = snprintf(data, 100,\n-\t\t\"Subproject commit %s\\n\", sha1_to_hex(s->sha1));\n+\t\t       \"Subproject commit %s%s\\n\", sha1_to_hex(s->sha1), dirty);\n \ts->data = data;\n \ts->size = len;\n \ts->should_free = 1;\ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex 5cf8924..bf8c980 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -32,7 +32,8 @@ 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 $_z40 M\tsub\" >expect &&\n+\tsubtip=$3 subprev=$2\n '\n \n test_expect_success 'git diff --raw HEAD' '\n@@ -50,6 +51,52 @@ test_expect_success 'git diff-files --raw' '\n \ttest_cmp expect actual.files\n '\n \n+expect_from_to () {\n+\tprintf \"%sSubproject commit %s\\n+Subproject commit %s\\n\" \\\n+\t\t\"-\" \"$1\" \"$2\"\n+}\n+\n+test_expect_success 'git diff HEAD' '\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subtip $subprev &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n+test_expect_success 'git diff HEAD with dirty submodule (work tree)' '\n+\techo >>sub/world &&\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subtip $subprev-dirty &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n+test_expect_success 'git diff HEAD with dirty submodule (index)' '\n+\t(\n+\t\tcd sub &&\n+\t\tgit reset --hard &&\n+\t\techo >>world &&\n+\t\tgit add world\n+\t) &&\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subtip $subprev-dirty &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n+test_expect_success 'git diff HEAD with dirty submodule (untracked)' '\n+\t(\n+\t\tcd sub &&\n+\t\tgit reset --hard &&\n+\t\tgit clean -qfdx &&\n+\t\t>cruft\n+\t) &&\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subtip $subprev-dirty &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n test_expect_success 'git diff (empty submodule dir)' '\n \t: >empty &&\n \trm -rf sub/* sub/.git &&\n"},{"id":"131616","messageId":"4B4ED68D.1060402@web.de","threadId":"22176","inReplyTo":"7v6375lkpj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-14T08:32:13Z","receivedAt":"2010-01-14T08:32:13Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.01.2010 23:10, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> I had to squash in '#include \"submodule.h\"' in diff-lib.c just after it\n> includes \"refs.h\", though.\n\nSorry, i seem to repeatedly have missed the compiler warning :-(\n\n\n> And a patch to add:\n> \n>>> * It doesn't give detailed output when doing a \"git diff* -p\" with or\n>>>   without the --submodule option. It should show something like\n>>>\n>>>     diff --git a/sub b/sub\n>>>     index 5431f52..3f35670 160000\n>>>     --- a/sub\n>>>     +++ b/sub\n>>>     @@ -1 +1 @@\n>>>     -Subproject commit 5431f529197f3831cdfbba1354a819a79f948f6f\n>>>     +Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty\n>>>\n> \n> would look like the attached.\n\nThanks!\n\n\n> I think a reasonable next step would be\n> \n>  - Move the check for your condition (c) that we dropped from this round\n>    to wt-status.c;\n> \n>  - Add wt_status_print_dangling_submodules() to wt-status.c, and use the\n>    above logic to produce a section \"Submodules with Dangling HEAD\" or\n>    something.\n> \n>  - Call it in wt_status_print(), immediately before we check s->verbose\n>    and show the patch text under -v option.  \"git status\" now will warn\n>    about the condition (c).\n> \n>  - Add a similar wt_shortstatus_print_dangling_submodules() and call it at\n>    the end of wt_shortstatus_print().\n> \n>  - Update is_submodule_modified() in your patch thats reads the output\n>    from \"status --porcelain\", to *ignore* information about dangling\n>    submodules.  As we discussed, dangling submodules may be something the\n>    user cares about, but that is not something \"diff\" should.\n\nGreat, i will send patches when i have something to show.\n"},{"id":"131679","messageId":"4B4F8EF1.3080709@web.de","threadId":"22176","inReplyTo":"7v6375lkpj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-14T21:38:57Z","receivedAt":"2010-01-14T21:38:57Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.01.2010 23:10, schrieb Junio C Hamano:\n> And a patch to add:\n> \n>>> * It doesn't give detailed output when doing a \"git diff* -p\" with or\n>>>   without the --submodule option. It should show something like\n>>>\n>>>     diff --git a/sub b/sub\n>>>     index 5431f52..3f35670 160000\n>>>     --- a/sub\n>>>     +++ b/sub\n>>>     @@ -1 +1 @@\n>>>     -Subproject commit 5431f529197f3831cdfbba1354a819a79f948f6f\n>>>     +Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty\n>>>\n> \n> would look like the attached.\n\nYour patch did not show submodules as dirty when the refs were identical.\nThe following patch fixes that and extends the test to catch that too.\n\n-- >8 --\nSubject: Show a modified submodule directory as dirty even if the refs match\n\nWhen the submodules HEAD and the ref committed in the HEAD of the\nsuperproject were the same, \"git diff[-index] HEAD\" did not show the\nsubmodule as dirty when it should.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n diff-lib.c                |    3 ++-\n t/t4027-diff-submodule.sh |   35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 5ce226b..9cdf6da 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -233,7 +233,8 @@ static int get_stat_data(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, 0);\n-\t\tif (changed) {\n+\t\tif (changed\n+\t\t    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\tsha1 = null_sha1;\n \t\t}\ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex bf8c980..83c1914 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -97,6 +97,41 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked)' '\n \ttest_cmp expect.body actual.body\n '\n\n+test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)' '\n+\tgit commit -m \"x\" sub &&\n+\techo >>sub/world &&\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subprev $subprev-dirty &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n+test_expect_success 'git diff HEAD with dirty submodule (index, refs match)' '\n+\t(\n+\t\tcd sub &&\n+\t\tgit reset --hard &&\n+\t\techo >>world &&\n+\t\tgit add world\n+\t) &&\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subprev $subprev-dirty &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n+test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' '\n+\t(\n+\t\tcd sub &&\n+\t\tgit reset --hard &&\n+\t\tgit clean -qfdx &&\n+\t\t>cruft\n+\t) &&\n+\tgit diff HEAD >actual &&\n+\tsed -e \"1,/^@@/d\" actual >actual.body &&\n+\texpect_from_to >expect.body $subprev $subprev-dirty &&\n+\ttest_cmp expect.body actual.body\n+'\n+\n test_expect_success 'git diff (empty submodule dir)' '\n \t: >empty &&\n \trm -rf sub/* sub/.git &&\n-- \n1.6.6.294.g1f7f2.dirty\n"},{"id":"131686","messageId":"7v3a288em2.fsf@alter.siamese.dyndns.org","threadId":"22176","inReplyTo":"4B4F8EF1.3080709@web.de","subject":"Re: [PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-14T23:13:09Z","receivedAt":"2010-01-14T23:13:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Subject: Show a modified submodule directory as dirty even if the refs match\n>\n> When the submodules HEAD and the ref committed in the HEAD of the\n> superproject were the same, \"git diff[-index] HEAD\" did not show the\n> submodule as dirty when it should.\n>\n> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n> ---\n>  diff-lib.c                |    3 ++-\n>  t/t4027-diff-submodule.sh |   35 +++++++++++++++++++++++++++++++++++\n>  2 files changed, 37 insertions(+), 1 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 5ce226b..9cdf6da 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -233,7 +233,8 @@ static int get_stat_data(struct cache_entry *ce,\n>  \t\t\treturn -1;\n>  \t\t}\n>  \t\tchanged = ce_match_stat(ce, &st, 0);\n> -\t\tif (changed) {\n> +\t\tif (changed\n> +\t\t    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {\n\nYou had a check in your previous patch that decides to call or skip\ndiff_change() based on is_submodule_modified() for diff-files, but forgot\nto have the same for diff-index, which this patch does.  Perhaps we want\nto squash this into 4519d9c (Show submodules as modified when they contain\na dirty work tree, 2010-01-13).\n\nThe existing code is a bit unfortunate; by the time we come to the output\nroutine, the information we found from is_submodule_modified() is lost;\nthat is why my \"would look like this\" patch calls is_submodule_modified().\n\nWe may want to add one parameter to diff_change() and diff_addremove(), to\ntell them if the work-tree side (if we are comparing something with the\nwork tree) is a modified submodule, and add one bit to the diff_filespec\nstructure to record that in diff_change() and diff_addremove() (obviously\nonly when adding).  That way, my \"would looks like this\" patch needs to\ncheck the result of is_submodule_modified() the front-ends left in the\nfilespec, instead of running it again.\n"},{"id":"131695","messageId":"4B4FB5A5.7080401@web.de","threadId":"22176","inReplyTo":"7v3a288em2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-15T00:24:05Z","receivedAt":"2010-01-15T00:24:05Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 15.01.2010 00:13, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>> Subject: Show a modified submodule directory as dirty even if the refs match\n>>\n>> When the submodules HEAD and the ref committed in the HEAD of the\n>> superproject were the same, \"git diff[-index] HEAD\" did not show the\n>> submodule as dirty when it should.\n>>\n>> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n>> ---\n>>  diff-lib.c                |    3 ++-\n>>  t/t4027-diff-submodule.sh |   35 +++++++++++++++++++++++++++++++++++\n>>  2 files changed, 37 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/diff-lib.c b/diff-lib.c\n>> index 5ce226b..9cdf6da 100644\n>> --- a/diff-lib.c\n>> +++ b/diff-lib.c\n>> @@ -233,7 +233,8 @@ static int get_stat_data(struct cache_entry *ce,\n>>  \t\t\treturn -1;\n>>  \t\t}\n>>  \t\tchanged = ce_match_stat(ce, &st, 0);\n>> -\t\tif (changed) {\n>> +\t\tif (changed\n>> +\t\t    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {\n> \n> You had a check in your previous patch that decides to call or skip\n> diff_change() based on is_submodule_modified() for diff-files, but forgot\n> to have the same for diff-index, which this patch does.  Perhaps we want\n> to squash this into 4519d9c (Show submodules as modified when they contain\n> a dirty work tree, 2010-01-13).\n\nOf course you are right, the change you quoted should have been in my\npatch in the first place. So squashing it seems to be the right thing to\ndo (but AFAICS the tests i added might be a problem, as they use\nexpect_from_to() which your intermediate patch added. Maybe squash these\ntests into your patch and the diff you quoted above into mine?).\n\n\n> The existing code is a bit unfortunate; by the time we come to the output\n> routine, the information we found from is_submodule_modified() is lost;\n> that is why my \"would look like this\" patch calls is_submodule_modified().\n> \n> We may want to add one parameter to diff_change() and diff_addremove(), to\n> tell them if the work-tree side (if we are comparing something with the\n> work tree) is a modified submodule, and add one bit to the diff_filespec\n> structure to record that in diff_change() and diff_addremove() (obviously\n> only when adding).  That way, my \"would looks like this\" patch needs to\n> check the result of is_submodule_modified() the front-ends left in the\n> filespec, instead of running it again.\n\nGood idea, i've been already exploring this line of thought too and came\nto the same conclusion (i noticed that when calling plain \"git diff\" in a\nrepo with submodules, is_submodule_modified() gets called *three* times\nfor each submodule, which is not /that/ good for performance ;-). But i\nintended to do this optimization in a subsequent patch (and in preparation\nfor \"git diff --submodule\" being able to print /how/ the submodule is\ndirty without having to scan it again).\n"},{"id":"131718","messageId":"20100115223221.6117@nanako3.lavabit.com","threadId":"22176","inReplyTo":"7vbpgyqy4a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Show submodules as modified when they contain a dirty work tree","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2010-01-15T13:32:21Z","receivedAt":"2010-01-15T13:32:21Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Jens Lehmann <Jens.Lehmann@web.de>\n\n> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\n> index 3ca17ab..47e205b 100755\n> --- a/t/t7506-status-submodule.sh\n> +++ b/t/t7506-status-submodule.sh\n> @@ -10,8 +10,12 @@ test_expect_success 'setup' '\n>  \t: >bar &&\n>  \tgit add bar &&\n>  \tgit commit -m \" Add bar\" &&\n> +\t: >foo &&\n> +\tgit add foo &&\n> +\tgit commit -m \" Add foo\" &&\n>  \tcd .. &&\n> -\tgit add sub &&\n> +\techo output > .gitignore\n> +\tgit add sub .gitignore &&\n>  \tgit commit -m \"Add submodule sub\"\n>  '\n\nThis is not a new problem you introduced, but if some commands\nbefore 'cd ..' fails, the next test will run in 'sub'. Other\ntests run operations inside () to avoid this problem.\n\n> @@ -23,6 +27,31 @@ test_expect_success 'commit --dry-run -a clean' '\n>  \tgit commit --dry-run -a |\n>  \tgrep \"nothing to commit\"\n>  '\n> +\n> +echo \"changed\" > sub/foo\n\nHave it inside the next test_expect_success.\n\n> +test_expect_success 'status with modified file in submodule' '\n> +\tgit status | grep \"modified:   sub\"\n> +'\n\nTo catch failure from 'git status' this is better written like this.\n    git status >output &&\n    grep \"modified:   sub\" output\n\n> +test_expect_success 'status with modified file in submodule (porcelain)' '\n> +\tgit status --porcelain >output &&\n> +\tdiff output - <<-EOF\n> + M sub\n> +EOF\n> +'\n\nIf you use -EOF you may want to align it with tab to make it\neasier to read. The one in t7005-editor.sh is a good example\n(t7401 is a bad example to imitate).\n\n> +(cd sub && git checkout foo)\n> +\n> +echo \"content\" > sub/new-file\n\nMove this part to the next test_expect_success to catch broken\ncheckout.\n\n> +test_expect_success 'status with untracked file in submodule' '\n> +\tgit status | grep \"modified:   sub\"\n> +'\n\nSame comment as before.\n\n> +test_expect_success 'status with untracked file in submodule (porcelain)' '\n> +\tgit status --porcelain >output &&\n> +\tdiff output - <<-EOF\n> + M sub\n> +EOF\n> +'\n\nSame comment as before.\n\n> +rm sub/new-file\n> +\n\nDo you need this? If so, move it inside the next\ntest_expect_success.\n\n>  test_expect_success 'rm submodule contents' '\n>  \trm -rf sub/* sub/.git\n>  '\n> -- \n> 1.6.6.203.g28a8ba.dirty\n\nThe following can be squashed to 4519d9cf092a173ac7b0a5570b0d5d602086ecf2\n\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 47e205b..253c334 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -5,63 +5,87 @@ test_description='git status for submodule'\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-\ttest_create_repo sub\n-\tcd sub &&\n-\t: >bar &&\n-\tgit add bar &&\n-\tgit commit -m \" Add bar\" &&\n-\t: >foo &&\n-\tgit add foo &&\n-\tgit commit -m \" Add foo\" &&\n-\tcd .. &&\n-\techo output > .gitignore\n+\ttest_create_repo sub &&\n+\t(\n+\t\tcd sub &&\n+\t\t: >bar &&\n+\t\tgit add bar &&\n+\t\tgit commit -m \" Add bar\" &&\n+\t\t: >foo &&\n+\t\tgit add foo &&\n+\t\tgit commit -m \" Add foo\"\n+\t) &&\n+\techo output > .gitignore &&\n \tgit add sub .gitignore &&\n \tgit commit -m \"Add submodule sub\"\n '\n \n test_expect_success 'status clean' '\n-\tgit status |\n-\tgrep \"nothing to commit\"\n+\tgit status >output &&\n+\tgrep \"nothing to commit\" output\n '\n+\n test_expect_success 'commit --dry-run -a clean' '\n-\tgit commit --dry-run -a |\n-\tgrep \"nothing to commit\"\n+\ttest_must_fail git commit --dry-run -a >output &&\n+\tgrep \"nothing to commit\" output\n '\n \n-echo \"changed\" > sub/foo\n test_expect_success 'status with modified file in submodule' '\n-\tgit status | grep \"modified:   sub\"\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub\" output\n '\n+\n test_expect_success 'status with modified file in submodule (porcelain)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub\" output\n+'\n+\n+test_expect_success 'status with added file in submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status --porcelain >output &&\n-\tdiff output - <<-EOF\n- M sub\n-EOF\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n '\n-(cd sub && git checkout foo)\n \n-echo \"content\" > sub/new-file\n test_expect_success 'status with untracked file in submodule' '\n-\tgit status | grep \"modified:   sub\"\n+\t(cd sub && git reset --hard) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub\" output\n '\n+\n test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tgit status --porcelain >output &&\n-\tdiff output - <<-EOF\n- M sub\n-EOF\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n '\n-rm sub/new-file\n \n test_expect_success 'rm submodule contents' '\n \trm -rf sub/* sub/.git\n '\n+\n test_expect_success 'status clean (empty submodule dir)' '\n-\tgit status |\n-\tgrep \"nothing to commit\"\n+\tgit status >output &&\n+\tgrep \"nothing to commit\" output\n '\n+\n test_expect_success 'status -a clean (empty submodule dir)' '\n-\tgit commit --dry-run -a |\n-\tgrep \"nothing to commit\"\n+\ttest_must_fail git commit --dry-run -a >output &&\n+\tgrep \"nothing to commit\" output\n '\n \n test_done\n\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"131956","messageId":"4B535E97.1020809@web.de","threadId":"22176","inReplyTo":"7v3a288em2.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Performance optimization for detection of modified submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-17T19:01:43Z","receivedAt":"2010-01-17T19:01:43Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"In the worst case is_submodule_modified() got called three times for\neach submodule. The information we got from scanning the whole\nsubmodule tree the first time should not be thrown away.\n\nA new parameter has been added to diff_change() and diff_addremove(),\nthe information is stored in a new member of struct diff_filespec. Its\nvalue is then reused instead of calling is_submodule_modified() again.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nAm 15.01.2010 00:13, schrieb Junio C Hamano:\n> The existing code is a bit unfortunate; by the time we come to the output\n> routine, the information we found from is_submodule_modified() is lost;\n> that is why my \"would look like this\" patch calls is_submodule_modified().\n>\n> We may want to add one parameter to diff_change() and diff_addremove(), to\n> tell them if the work-tree side (if we are comparing something with the\n> work tree) is a modified submodule, and add one bit to the diff_filespec\n> structure to record that in diff_change() and diff_addremove() (obviously\n> only when adding).  That way, my \"would looks like this\" patch needs to\n> check the result of is_submodule_modified() the front-ends left in the\n> filespec, instead of running it again.\n\nSo here is my first attempt of implementing your proposal. The test suite\nruns fine, but a few more eyeballs would really be appreciated as i am not\nvery familiar with the code and its corner cases (See diff_change(), is it\nsufficient to only set \"two->dirty_submodule\", even if the REVERSE_DIFF\noption is set? Apart from that i am not so sure about the four changes to\ntree-diff.c).\n\nI think we could skip the call to is_submodule_modified() in\nrun_diff_files() and get_stat_data() when the changed flag is already\nset and only short output (without calling diff_populate_gitlink(), e.g.\n\"git status -s\" or \"git diff-files\") is requested. What do you think\nabout doing that in a seperate patch?\n\n\n\n diff-lib.c  |   42 +++++++++++++++++++++++++++---------------\n diff.c      |   11 +++++++----\n diff.h      |    8 ++++----\n diffcore.h  |    1 +\n revision.c  |    4 ++--\n tree-diff.c |    8 ++++----\n 6 files changed, 45 insertions(+), 29 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 29c5915..5b18d86 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -73,6 +73,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tunsigned int oldmode, newmode;\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tint changed;\n+\t\tint dirty_submodule = 0;\n\n \t\tif (DIFF_OPT_TST(&revs->diffopt, QUICK) &&\n \t\t\tDIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n@@ -173,12 +174,14 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tif (silent_on_removed)\n \t\t\t\tcontinue;\n \t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n-\t\t\t\t       ce->sha1, ce->name);\n+\t\t\t\t       ce->sha1, ce->name, 0);\n \t\t\tcontinue;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n-\t\tif (S_ISGITLINK(ce->ce_mode) && !changed)\n-\t\t\tchanged = is_submodule_modified(ce->name);\n+\t\tif (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name)) {\n+\t\t\tchanged = 1;\n+\t\t\tdirty_submodule = 1;\n+\t\t}\n \t\tif (!changed) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n@@ -188,7 +191,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n \t\t\t    ce->sha1, (changed ? null_sha1 : ce->sha1),\n-\t\t\t    ce->name);\n+\t\t\t    ce->name, dirty_submodule);\n\n \t}\n \tdiffcore_std(&revs->diffopt);\n@@ -204,16 +207,18 @@ 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 const unsigned char *sha1, unsigned int mode)\n+\t\t\t\t const unsigned char *sha1, unsigned int mode,\n+\t\t\t\t int dirty_submodule)\n {\n \tdiff_addremove(&revs->diffopt, prefix[0], mode,\n-\t\t       sha1, ce->name);\n+\t\t       sha1, ce->name, dirty_submodule);\n }\n\n static int get_stat_data(struct cache_entry *ce,\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+\t\t\t int cached, int match_missing,\n+\t\t\t int *dirty_submodule)\n {\n \tconst unsigned char *sha1 = ce->sha1;\n \tunsigned int mode = ce->ce_mode;\n@@ -233,8 +238,11 @@ static int get_stat_data(struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, 0);\n-\t\tif (changed\n-\t\t    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {\n+\t\tif (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name)) {\n+\t\t\tchanged = 1;\n+\t\t\t*dirty_submodule = 1;\n+\t\t}\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}\n@@ -251,15 +259,17 @@ static void show_new_file(struct rev_info *revs,\n {\n \tconst unsigned char *sha1;\n \tunsigned int mode;\n+\tint dirty_submodule = 0;\n\n \t/*\n \t * New file in the index: it might actually be different in\n \t * the working copy.\n \t */\n-\tif (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0)\n+\tif (get_stat_data(new, &sha1, &mode, cached, match_missing,\n+\t    &dirty_submodule) < 0)\n \t\treturn;\n\n-\tdiff_index_show_file(revs, \"+\", new, sha1, mode);\n+\tdiff_index_show_file(revs, \"+\", new, sha1, mode, dirty_submodule);\n }\n\n static int show_modified(struct rev_info *revs,\n@@ -270,11 +280,13 @@ static int show_modified(struct rev_info *revs,\n {\n \tunsigned int mode, oldmode;\n \tconst unsigned char *sha1;\n+\tint dirty_submodule = 0;\n\n-\tif (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {\n+\tif (get_stat_data(new, &sha1, &mode, cached, match_missing,\n+\t\t\t  &dirty_submodule) < 0) {\n \t\tif (report_missing)\n \t\t\tdiff_index_show_file(revs, \"-\", old,\n-\t\t\t\t\t     old->sha1, old->ce_mode);\n+\t\t\t\t\t     old->sha1, old->ce_mode, 0);\n \t\treturn -1;\n \t}\n\n@@ -309,7 +321,7 @@ static int show_modified(struct rev_info *revs,\n \t\treturn 0;\n\n \tdiff_change(&revs->diffopt, oldmode, mode,\n-\t\t    old->sha1, sha1, old->name);\n+\t\t    old->sha1, sha1, old->name, dirty_submodule);\n \treturn 0;\n }\n\n@@ -356,7 +368,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t * Something removed from the tree?\n \t */\n \tif (!idx) {\n-\t\tdiff_index_show_file(revs, \"-\", tree, tree->sha1, tree->ce_mode);\n+\t\tdiff_index_show_file(revs, \"-\", tree, tree->sha1, tree->ce_mode, 0);\n \t\treturn;\n \t}\n\ndiff --git a/diff.c b/diff.c\nindex 012b3d3..490a7ec 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2032,7 +2032,7 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n \tchar *data = xmalloc(100), *dirty = \"\";\n\n \t/* Are we looking at the work tree? */\n-\tif (!s->sha1_valid && is_submodule_modified(s->path))\n+\tif (!s->sha1_valid && s->dirty_submodule)\n \t\tdirty = \"-dirty\";\n\n \tlen = snprintf(data, 100,\n@@ -3736,7 +3736,7 @@ int diff_result_code(struct diff_options *opt, int status)\n void diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n-\t\t    const char *concatpath)\n+\t\t    const char *concatpath, int dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n\n@@ -3768,8 +3768,10 @@ void diff_addremove(struct diff_options *options,\n\n \tif (addremove != '+')\n \t\tfill_filespec(one, sha1, mode);\n-\tif (addremove != '-')\n+\tif (addremove != '-') {\n \t\tfill_filespec(two, sha1, mode);\n+\t\ttwo->dirty_submodule = dirty_submodule;\n+\t}\n\n \tdiff_queue(&diff_queued_diff, one, two);\n \tif (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n@@ -3780,7 +3782,7 @@ void diff_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n-\t\t const char *concatpath)\n+\t\t const char *concatpath, int dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n\n@@ -3803,6 +3805,7 @@ void diff_change(struct diff_options *options,\n \ttwo = alloc_filespec(concatpath);\n \tfill_filespec(one, old_sha1, old_mode);\n \tfill_filespec(two, new_sha1, new_mode);\n+\ttwo->dirty_submodule = dirty_submodule;\n\n \tdiff_queue(&diff_queued_diff, one, two);\n \tif (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\ndiff --git a/diff.h b/diff.h\nindex 06a9a88..13596c2 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -14,12 +14,12 @@ typedef void (*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n-\t\t const char *fullpath);\n+\t\t const char *fullpath, int dirty_submodule);\n\n typedef void (*add_remove_fn_t)(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n-\t\t    const char *fullpath);\n+\t\t    const char *fullpath, int dirty_submodule);\n\n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n@@ -177,13 +177,13 @@ extern void diff_addremove(struct diff_options *,\n \t\t\t   int addremove,\n \t\t\t   unsigned mode,\n \t\t\t   const unsigned char *sha1,\n-\t\t\t   const char *fullpath);\n+\t\t\t   const char *fullpath, int dirty_submodule);\n\n extern void diff_change(struct diff_options *,\n \t\t\tunsigned mode1, unsigned mode2,\n \t\t\tconst unsigned char *sha1,\n \t\t\tconst unsigned char *sha2,\n-\t\t\tconst char *fullpath);\n+\t\t\tconst char *fullpath, int dirty_submodule);\n\n extern void diff_unmerge(struct diff_options *,\n \t\t\t const char *path,\ndiff --git a/diffcore.h b/diffcore.h\nindex 5b63458..66687c3 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -42,6 +42,7 @@ struct diff_filespec {\n #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)\n \tunsigned should_free : 1; /* data should be free()'ed */\n \tunsigned should_munmap : 1; /* data should be munmap()'ed */\n+\tunsigned dirty_submodule : 1;  /* For submodules: its work tree is dirty */\n\n \tstruct userdiff_driver *driver;\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\ndiff --git a/revision.c b/revision.c\nindex 25fa14d..e95cc41 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -268,7 +268,7 @@ static int tree_difference = REV_TREE_SAME;\n static void file_add_remove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n-\t\t    const char *fullpath)\n+\t\t    const char *fullpath, int dirty_submodule)\n {\n \tint diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;\n\n@@ -281,7 +281,7 @@ static void file_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n-\t\t const char *fullpath)\n+\t\t const char *fullpath, int dirty_submodule)\n {\n \ttree_difference = REV_TREE_DIFFERENT;\n \tDIFF_OPT_SET(options, HAS_CHANGES);\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 7d745b4..2ef0c77 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -68,7 +68,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n \t\t\tnewbase[baselen + pathlen1] = 0;\n \t\t\topt->change(opt, mode1, mode2,\n-\t\t\t\t    sha1, sha2, newbase);\n+\t\t\t\t    sha1, sha2, newbase, 0);\n \t\t\tnewbase[baselen + pathlen1] = '/';\n \t\t}\n \t\tretval = diff_tree_sha1(sha1, sha2, newbase, opt);\n@@ -77,7 +77,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \t}\n\n \tfullname = malloc_fullname(base, baselen, path1, pathlen1);\n-\topt->change(opt, mode1, mode2, sha1, sha2, fullname);\n+\topt->change(opt, mode1, mode2, sha1, sha2, fullname, 0);\n \tfree(fullname);\n \treturn 0;\n }\n@@ -241,7 +241,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n\n \t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n \t\t\tnewbase[baselen + pathlen] = 0;\n-\t\t\topt->add_remove(opt, *prefix, mode, sha1, newbase);\n+\t\t\topt->add_remove(opt, *prefix, mode, sha1, newbase, 0);\n \t\t\tnewbase[baselen + pathlen] = '/';\n \t\t}\n\n@@ -252,7 +252,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \t\tfree(newbase);\n \t} else {\n \t\tchar *fullname = malloc_fullname(base, baselen, path, pathlen);\n-\t\topt->add_remove(opt, prefix[0], mode, sha1, fullname);\n+\t\topt->add_remove(opt, prefix[0], mode, sha1, fullname, 0);\n \t\tfree(fullname);\n \t}\n }\n-- \n1.6.6.327.g4c0c1.dirty\n"},{"id":"131963","messageId":"7vska4xzzf.fsf@alter.siamese.dyndns.org","threadId":"22176","inReplyTo":"4B535E97.1020809@web.de","subject":"Re: [PATCH] Performance optimization for detection of modified submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-17T20:01:24Z","receivedAt":"2010-01-17T20:01:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> So here is my first attempt of implementing your proposal. The test suite\n> runs fine, but a few more eyeballs would really be appreciated as i am not\n> very familiar with the code and its corner cases (See diff_change(), is it\n> sufficient to only set \"two->dirty_submodule\", even if the REVERSE_DIFF\n> option is set? Apart from that i am not so sure about the four changes to\n> tree-diff.c).\n\nThe effect of REVERSE_DIFF bit is contained in the output layer.  The\norder frontends (e.g. diff-files and diff-lib.c::run_diff_files()) feed\nthe entries from two hierarchies is not affected.\n\nThe current callers of addremove() may always give the work tree side as\nthe second one, but the API is meant to be usable by any other new callers\nand for some of them feeding the work tree side as the first one _might_\nbe more sensible (we are talking about futureproofing, so by definition we\nwon't know).  It might even be the case where an unanticipated new caller\nmight be comparing two trees both living in the work tree (hence you might\nrequire two independent dirty_submodule bits to the call to show which\nside is dirty, and such a caller may say \"both sides are dirty\").\n\nSo it would be most future-proof if you add two independent \"dirty\" bits\nto the API if you are changing it: \"is the left side of the comparision a\ndirty submodule?\" and \"is the right side ...?\".  Especially I don't think\nassuming \"setting two->dirty is enough for the current implementation\" is\nthe right way going forward.\n\n> I think we could skip the call to is_submodule_modified() in\n> run_diff_files() and get_stat_data() when the changed flag is already\n> set and only short output (without calling diff_populate_gitlink(), e.g.\n> \"git status -s\" or \"git diff-files\") is requested.\n\nI am puzzled by your \"we could skip\"; isn't it what you already have done\nin this patch?  More importantly, I think that is the whole point of the\nchange to diff API this patch brings in.\n\n> What do you think\n> about doing that in a seperate patch?\n\nDoing these in this same patch like you did is better, as it demonstrates\nhow the callers benefit by the addition of these new bits to the API.\n"},{"id":"131964","messageId":"4B537432.3020201@web.de","threadId":"22176","inReplyTo":"7vska4xzzf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Performance optimization for detection of modified submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-17T20:33:54Z","receivedAt":"2010-01-17T20:33:54Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 17.01.2010 21:01, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>> So here is my first attempt of implementing your proposal. The test suite\n>> runs fine, but a few more eyeballs would really be appreciated as i am not\n>> very familiar with the code and its corner cases (See diff_change(), is it\n>> sufficient to only set \"two->dirty_submodule\", even if the REVERSE_DIFF\n>> option is set? Apart from that i am not so sure about the four changes to\n>> tree-diff.c).\n> \n> The effect of REVERSE_DIFF bit is contained in the output layer.  The\n> order frontends (e.g. diff-files and diff-lib.c::run_diff_files()) feed\n> the entries from two hierarchies is not affected.\n> \n> The current callers of addremove() may always give the work tree side as\n> the second one, but the API is meant to be usable by any other new callers\n> and for some of them feeding the work tree side as the first one _might_\n> be more sensible (we are talking about futureproofing, so by definition we\n> won't know).  It might even be the case where an unanticipated new caller\n> might be comparing two trees both living in the work tree (hence you might\n> require two independent dirty_submodule bits to the call to show which\n> side is dirty, and such a caller may say \"both sides are dirty\").\n> \n> So it would be most future-proof if you add two independent \"dirty\" bits\n> to the API if you are changing it: \"is the left side of the comparision a\n> dirty submodule?\" and \"is the right side ...?\".  Especially I don't think\n> assuming \"setting two->dirty is enough for the current implementation\" is\n> the right way going forward.\n\nThanks for your explanation, will provide two independent dirty_submodule\nbits there.\n\n\n>> I think we could skip the call to is_submodule_modified() in\n>> run_diff_files() and get_stat_data() when the changed flag is already\n>> set and only short output (without calling diff_populate_gitlink(), e.g.\n>> \"git status -s\" or \"git diff-files\") is requested.\n> \n> I am puzzled by your \"we could skip\"; isn't it what you already have done\n> in this patch?  More importantly, I think that is the whole point of the\n> change to diff API this patch brings in.\n\nThis patch was about calling is_submodule_modified() /only once/, either\nin run_diff_files() or in get_stat_data(), and reuse the result. But i\nthink we don't have to call it /at all/ when for example executing \"git\nstatus -s\" and the HEAD of the submodule does not match the commit in the\nindex of the superproject. This information is enough to display an 'M'.\nNo need to check the submodule work tree for dirtyness, as it won't\nchange the output of the command anymore.\n"}]}