{"thread":{"id":"22347","subject":"What's cooking in git.git (Jan 2010, #07; Fri, 22)","startedAt":"2010-01-23T03:28:21Z","lastAt":"2010-01-25T18:05:23Z","messageCount":8,"participants":["Junio C Hamano","Jakub Narebski","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"132449","messageId":"7vtyudfqju.fsf@alter.siamese.dyndns.org","threadId":"22347","inReplyTo":null,"subject":"What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-23T03:28:21Z","receivedAt":"2010-01-23T03:28:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here are the topics that have been cooking.  Commits prefixed with '-' are\nonly in 'pu' while commits prefixed with '+' are in 'next'.  The ones\nmarked with '.' do not appear in any of the integration branches, but I am\nstill holding onto them.\n\n--------------------------------------------------\n[Graduated to \"master\"]\n\n* jc/conflict-marker-size (2010-01-16) 8 commits\n  (merged to 'next' on 2010-01-18 at f1f6023)\n + rerere: honor conflict-marker-size attribute\n + rerere: prepare for customizable conflict marker length\n + conflict-marker-size: new attribute\n + rerere: use ll_merge() instead of using xdl_merge()\n + merge-tree: use ll_merge() not xdl_merge()\n + xdl_merge(): allow passing down marker_size in xmparam_t\n + xdl_merge(): introduce xmparam_t for merge specific parameters\n + git_attr(): fix function signature\n\n* ag/maint-apply-too-large-p (2010-01-17) 1 commit\n  (merged to 'next' on 2010-01-18 at 8bd106a)\n + builtin-apply.c: Skip filenames without enough components\n\n* ag/patch-header-verify (2010-01-18) 1 commit\n  (merged to 'next' on 2010-01-18 at 2cd0ddc)\n + builtin-apply.c: fix the --- and +++ header filename consistency check\n\n* bw/cvsimport (2010-01-19) 3 commits\n  (merged to 'next' on 2010-01-19 at 63f4c8d)\n + cvsimport: standarize system() calls to external git tools\n + cvsimport: standarize open() calls to external git tools\n + cvsimport: modernize callouts to git subcommands\n\n* jc/checkout-merge-base (2010-01-19) 1 commit\n  (merged to 'next' on 2010-01-19 at 3665110)\n + Fix \"checkout A...\" synonym for \"checkout A...HEAD\" on Windows\n\n* jc/maint-refresh-index-is-optional-for-status (2010-01-19) 1 commit\n + status: don't require the repository to be writable\n\n* nd/status-partial-refresh (2010-01-17) 2 commits\n  (merged to 'next' on 2010-01-19 at 64f0c0b)\n + rm: only refresh entries that we may touch\n  (merged to 'next' on 2010-01-16 at f77bc8f)\n + status: only touch path we may need to check\n\n* ap/merge-backend-opts (2008-07-18) 7 commits\n  (merged to 'next' on 2010-01-18 at cb1f6b7)\n + Document that merge strategies can now take their own options\n + Extend merge-subtree tests to test -Xsubtree=dir.\n + Make \"subtree\" part more orthogonal to the rest of merge-recursive.\n + pull: Fix parsing of -X<option>\n + Teach git-pull to pass -X<option> to git-merge\n + git merge -X<option>\n + git-merge-file --ours, --theirs\n\n* jc/maint-limit-note-output (2010-01-21) 2 commits\n  (merged to 'next' on 2010-01-21 at bcb80b9)\n + Fix \"log --oneline\" not to show notes\n  (merged to 'next' on 2010-01-20 at 526bfcc)\n + Fix \"log\" family not to be too agressive about showing notes\n\n* nd/ls-files-sparse-fix (2010-01-20) 1 commit\n  (merged to 'next' on 2010-01-20 at 0f61dbc)\n + Fix memory corruption when .gitignore does not end by \\n\n\n* il/branch-set-upstream (2010-01-18) 2 commits\n  (merged to 'next' on 2010-01-18 at b9b0993)\n + branch: warn and refuse to set a branch as a tracking branch of itself.\n + Add branch --set-upstream\n\n* il/remote-updates (2010-01-18) 1 commit\n  (merged to 'next' on 2010-01-18 at 5c3e805)\n + Add git remote set-url\n\n* il/rev-glob (2010-01-22) 3 commits\n  (merged to 'next' on 2010-01-21 at 453a21c)\n + Documentation: improve description of --glob=pattern and friends\n  (merged to 'next' on 2010-01-20 at 928ba0a)\n + rev-parse --branches/--tags/--remotes=pattern\n + rev-parse --glob\n\nThis is a re-rolled \"--namespace=\" one.\n\n* jl/submodule-diff (2010-01-18) 4 commits\n  (merged to 'next' on 2010-01-20 at 95cb513)\n + Performance optimization for detection of modified submodules\n  (merged to 'next' on 2010-01-17 at 525075b)\n + git status: Show uncommitted submodule changes too when enabled\n  (merged to 'next' on 2010-01-16 at 0a99e3c)\n + Teach diff that modified submodule directory is dirty\n + Show submodules as modified when they contain a dirty work tree\n\n* js/refer-upstream (2010-01-19) 3 commits\n  (merged to 'next' on 2010-01-20 at 5a5547a)\n + Teach @{upstream} syntax to strbuf_branchanme()\n + t1506: more test for @{upstream} syntax\n + Introduce <branch>@{upstream} notation\n\nUpdated to teach the new syntax to commands like \"checkout\" and \"merge\"\nthat want to behave better when they know what were given was a branch\nname, not a random SHA-1.\n\n* jc/branch-d (2009-12-29) 1 commit\n  (merged to 'next' on 2010-01-10 at 61a14b7)\n + branch -d: base the \"already-merged\" safety on the branch it merges with\n\n--------------------------------------------------\n[Will merge to 'master' after a bit more cooking in 'next']\n\n* jc/fix-tree-walk (2009-09-14) 7 commits\n  (merged to 'next' on 2010-01-13 at 1c01b87)\n + read-tree --debug-unpack\n + unpack-trees.c: look ahead in the index\n + unpack-trees.c: prepare for looking ahead in the index\n + Aggressive three-way merge: fix D/F case\n + traverse_trees(): handle D/F conflict case sanely\n + more D/F conflict tests\n + tests: move convenience regexp to match object names to test-lib.sh\n\nResurrected from \"Ejected\" category.  This is fix for a tricky codepath\nand testing and improving before it hits 'master' is greatly appreciated.\n(I have been using this in my private build for some time).\n\n--------------------------------------------------\n[Cooking]\n\n* jh/notes (2010-01-17) 20 commits\n . builtin-gc: Teach the new --notes option to garbage-collect notes\n . Notes API: gc_notes(): Prune notes that belong to non-existing objects\n . t3305: Verify that removing notes triggers automatic fanout consolidation\n . builtin-notes: Teach -d option for deleting existing notes\n . Teach builtin-notes to remove empty notes\n . Teach notes code to properly preserve non-notes in the notes tree\n . t3305: Verify that adding many notes with git-notes triggers increased fanout\n . t3301: Verify successful annotation of non-commits\n . Builtin-ify git-notes\n . Refactor notes concatenation into a flexible interface for combining notes\n . Notes API: Allow multiple concurrent notes trees with new struct notes_tree\n . Notes API: write_notes_tree(): Store the notes tree in the database\n . Notes API: for_each_note(): Traverse the entire notes tree with a callback\n . Notes API: get_note(): Return the note annotating the given object\n . Notes API: remove_note(): Remove note objects from the notes tree structure\n . Notes API: add_note(): Add note objects to the internal notes tree structure\n . Notes API: init_notes(): Initialize the notes tree from the given notes ref\n . Add tests for checking correct handling of $GIT_NOTES_REF and core.notesRef\n . Notes API: get_commit_notes() -> format_note() + remove the commit restriction\n . Minor non-functional fixes to notes.c\n\nTentatively ejected, as its tests conflict with tests in a higher priority\nfix.\n\n* jh/gitweb-cached (2010-01-13) 9 commits\n - gitweb: File based caching layer (from git.kernel.org)\n - gitweb: Convert output to using indirect file handle\n - gitweb: cleanup error message produced by undefined $site_header\n - gitweb: add a get function to compliment print_sort_th\n - gitweb: add a get function to compliment print_local_time\n - gitweb: Makefile improvements\n - gitweb: Add option to force version match\n - gitweb: change die_error to take \"extra\" argument for extended die information\n - gitweb: Load checking\n\nReplaced with a re-roll.  Update to t9500 is probably needed.\n\n* jc/grep-author-all-match-implicit (2010-01-17) 1 commit\n - \"log --author=me --grep=it\" should find intersection, not union\n"},{"id":"132477","messageId":"m3my05ax5v.fsf@localhost.localdomain","threadId":"22347","inReplyTo":"7vtyudfqju.fsf@alter.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-01-23T11:18:17Z","receivedAt":"2010-01-23T11:18:17Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> * jh/gitweb-cached (2010-01-13) 9 commits\n>  - gitweb: File based caching layer (from git.kernel.org)\n>  - gitweb: Convert output to using indirect file handle\n>  - gitweb: cleanup error message produced by undefined $site_header\n>  - gitweb: add a get function to compliment print_sort_th\n>  - gitweb: add a get function to compliment print_local_time\n>  - gitweb: Makefile improvements\n>  - gitweb: Add option to force version match\n>  - gitweb: change die_error to take \"extra\" argument for extended die information\n>  - gitweb: Load checking\n> \n> Replaced with a re-roll.  Update to t9500 is probably needed.\n\nI have sent proof of concept replacements for the last two\npatches... and those do include update to t9500 (although caching is\ntested only rudimentally).\n\nI don't know if J.H. is working on replacement patches; I am planning\non re-rolling cleaned up version of split file based caching layer on\ntop of his changes / his re-roll.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"132490","messageId":"4B5B25C6.70604@web.de","threadId":"22347","inReplyTo":"7vtyudfqju.fsf@alter.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-23T16:37:26Z","receivedAt":"2010-01-23T16:37:26Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 23.01.2010 04:28, schrieb Junio C Hamano:\n> * jl/submodule-diff (2010-01-18) 4 commits\n>   (merged to 'next' on 2010-01-20 at 95cb513)\n>  + Performance optimization for detection of modified submodules\n>   (merged to 'next' on 2010-01-17 at 525075b)\n>  + git status: Show uncommitted submodule changes too when enabled\n>   (merged to 'next' on 2010-01-16 at 0a99e3c)\n>  + Teach diff that modified submodule directory is dirty\n>  + Show submodules as modified when they contain a dirty work tree\n\nWhat about adding the following patch to this series?\nWithout it i see a performance regression for people who use\n\"git diff* --ignore-submodules\".\n\nA patch that teaches \"git diff --submodule\" to display if the submodule\ncontains new untracked and/or modified files is also almost ready. Would\nyou consider it for inclusion into 1.7.0 too or shall i wait until after\nthe release?\n\n------8<-----\nSubject: [PATCH] git diff: Don't test submodule dirtiness with --ignore-submodules\n\nThe diff family suppresses the output of submodule changes when\nrequested but checks them nonetheless. But since recently submodules\nget examined for their dirtiness, which is rather expensive. There is\nno need to do that when the --ignore-submodules option is used, as\nthe gathered information is never used anyway.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n diff-lib.c |   15 ++++++++++-----\n 1 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ec2e2ac..e896b9c 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -161,7 +161,10 @@ 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) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce))\n+\t\tif ((ce_uptodate(ce)\n+\t\t     && (!S_ISGITLINK(ce->ce_mode)\n+\t\t\t || DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES)))\n+\t\t    || 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@@ -179,6 +182,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n \t\tif (S_ISGITLINK(ce->ce_mode)\n+\t\t    && !DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES)\n \t\t    && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH))\n \t\t    && is_submodule_modified(ce->name)) {\n \t\t\tchanged = 1;\n@@ -220,7 +224,7 @@ 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 unsigned *dirty_submodule, int output_format)\n+\t\t\t unsigned *dirty_submodule, struct diff_options *diffopt)\n {\n \tconst unsigned char *sha1 = ce->sha1;\n \tunsigned int mode = ce->ce_mode;\n@@ -241,7 +245,8 @@ static int get_stat_data(struct cache_entry *ce,\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, 0);\n \t\tif (S_ISGITLINK(ce->ce_mode)\n-\t\t    && (!changed || (output_format & DIFF_FORMAT_PATCH))\n+\t\t    && !DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)\n+\t\t    && (!changed || (diffopt->output_format & DIFF_FORMAT_PATCH))\n \t\t    && is_submodule_modified(ce->name)) {\n \t\t\tchanged = 1;\n \t\t\t*dirty_submodule = 1;\n@@ -270,7 +275,7 @@ static void show_new_file(struct rev_info *revs,\n \t * the working copy.\n \t */\n \tif (get_stat_data(new, &sha1, &mode, cached, match_missing,\n-\t    &dirty_submodule, revs->diffopt.output_format) < 0)\n+\t    &dirty_submodule, &revs->diffopt) < 0)\n \t\treturn;\n\n \tdiff_index_show_file(revs, \"+\", new, sha1, mode, dirty_submodule);\n@@ -287,7 +292,7 @@ static int show_modified(struct rev_info *revs,\n \tunsigned dirty_submodule = 0;\n\n \tif (get_stat_data(new, &sha1, &mode, cached, match_missing,\n-\t\t\t  &dirty_submodule, revs->diffopt.output_format) < 0) {\n+\t\t\t  &dirty_submodule, &revs->diffopt) < 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, 0);\n-- \n1.6.6.1.558.g5c480.dirty\n"},{"id":"132497","messageId":"7v7hr8d1xg.fsf@alter.siamese.dyndns.org","threadId":"22347","inReplyTo":"4B5B25C6.70604@web.de","subject":"Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-23T20:03:07Z","receivedAt":"2010-01-23T20:03:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> A patch that teaches \"git diff --submodule\" to display if the submodule\n> contains new untracked and/or modified files is also almost ready.\n\nHow does \"git submodule summary\" show them?  If it doesn't, then I don't\nthink we would want to show them, either, as my understanding is that a\nlonger-term plan is to use \"diff --submodule\" in git-gui to replace it.\n\n> Would\n> you consider it for inclusion into 1.7.0 too or shall i wait until after\n> the release?\n\nIf a feature is not in 'master' already, I think it is too late to be\nincluded in 1.7.0, if that is what you are asking.  But if you start the\nusual cycle of working on, asking others to review and polishing it before\nthe release, it would give us better designed and more tested version in\n1.7.1, no?\n\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index ec2e2ac..e896b9c 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -161,7 +161,10 @@ 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) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce))\n> +\t\tif ((ce_uptodate(ce)\n> +\t\t     && (!S_ISGITLINK(ce->ce_mode)\n> +\t\t\t || DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES)))\n> +\t\t    || ce_skip_worktree(ce))\n>  \t\t\tcontinue;\n\nI think this is sensible; the frontend knows that it doesn't care about submodules.\n\n> @@ -179,6 +182,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\t}\n>  \t\tchanged = ce_match_stat(ce, &st, ce_option);\n>  \t\tif (S_ISGITLINK(ce->ce_mode)\n> +\t\t    && !DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES)\n>  \t\t    && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH))\n>  \t\t    && is_submodule_modified(ce->name)) {\n>  \t\t\tchanged = 1;\n\nLikewise, but with one \"hmph\".  This codepath deals with a path that is a\nsubmodule in the index (the work tree may still have submodule, removed\nit, or replaced it with a regular file).  However, in the codepath that\nfollows this up to the call to diff_change(), is_submodule_modified() is\nnot called if the work tree has a submodule in a path that used to be\nsomething else.  Do we want one?\n\n> @@ -220,7 +224,7 @@ 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 unsigned *dirty_submodule, int output_format)\n> +\t\t\t unsigned *dirty_submodule, struct diff_options *diffopt)\n>  {\n>  \tconst unsigned char *sha1 = ce->sha1;\n>  \tunsigned int mode = ce->ce_mode;\n\nBelow the context of this hunk, we seem to do this:\n\n\tif (!cached && !ce_uptodate(ce)) {\n        \t...\n                if gitlink then call is_submodule_modified()\n\t}\n\nBut isn't it inconsistent with hunk at the beginning of this patch (ll 161-170)\nthat essentially says \"entries that is ce_uptodate() is Ok, but if it is a\ngitlink we need to look deeper\"?  Why isn't this function looking deeper\neven when we see that the submodule entry is ce_uptodate()?\n\n    Side note: the lack of ce_skip_worktree() check is Ok.  The callers of\n    get_stat_data() are show_new_file() and show_mododified() but they are\n    never called from their sole caller, do_oneway_diff(), on a skipped\n    worktree entry.\n\n> @@ -241,7 +245,8 @@ static int get_stat_data(struct cache_entry *ce,\n>  \t\t}\n>  \t\tchanged = ce_match_stat(ce, &st, 0);\n>  \t\tif (S_ISGITLINK(ce->ce_mode)\n> -\t\t    && (!changed || (output_format & DIFF_FORMAT_PATCH))\n> +\t\t    && !DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)\n> +\t\t    && (!changed || (diffopt->output_format & DIFF_FORMAT_PATCH))\n>  \t\t    && is_submodule_modified(ce->name)) {\n>  \t\t\tchanged = 1;\n>  \t\t\t*dirty_submodule = 1;\n\nThis hunk by itself is Ok, but I am still puzzled about a case where you\nhave \"seemingly clean because ce_uptodate() says so, but submodule work\ntree or index might be dirty\" case, in which this code won't trigger.\n"},{"id":"132522","messageId":"7veilg9rqp.fsf@alter.siamese.dyndns.org","threadId":"22347","inReplyTo":"7v7hr8d1xg.fsf@alter.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-24T02:11:26Z","receivedAt":"2010-01-24T02:11:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> ...\n>> @@ -220,7 +224,7 @@ 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 unsigned *dirty_submodule, int output_format)\n>> +\t\t\t unsigned *dirty_submodule, struct diff_options *diffopt)\n>>  {\n>>  \tconst unsigned char *sha1 = ce->sha1;\n>>  \tunsigned int mode = ce->ce_mode;\n>\n> Below the context of this hunk, we seem to do this:\n>\n> \tif (!cached && !ce_uptodate(ce)) {\n>         \t...\n>                 if gitlink then call is_submodule_modified()\n> \t}\n>\n> But isn't it inconsistent with hunk at the beginning of this patch (ll 161-170)\n> that essentially says \"entries that is ce_uptodate() is Ok, but if it is a\n> gitlink we need to look deeper\"?  Why isn't this function looking deeper\n> even when we see that the submodule entry is ce_uptodate()?\n>\n>     Side note: the lack of ce_skip_worktree() check is Ok.  The callers of\n>     get_stat_data() are show_new_file() and show_mododified() but they are\n>     never called from their sole caller, do_oneway_diff(), on a skipped\n>     worktree entry.\n\nI think we need to clarify the rule for ce_uptodate(ce).\n\nThe rule has always been that an entry that is ce_uptodate(ce) is _known_\nto be clean, and nobody should have to dig deeper to double check.  We\nshould keep it that way.\n\nWe at least need to fix preload_index() not to mark any gitlink entries\nwith ce_mark_uptodate(ce), as your series changes the definition of a\ndirty submodule.  It used to be that if a submodule is checked out and its\nHEAD matches what is recorded in the index of the superproject, then the\nsubmodule is clean.  The checks made in preload_thread() is consistent\nwith this definition.\n\nWith the update, we consider that having local changes in the submodule\n(either to the index or to the work tree files) makes it modified (which\nby the way is a right definition, and prevents people from forgetting to\ncommit in the submodule and updating the superproject index with the new\nsubmodule commit, before commiting the state in the superproject).\n\nBecause the checks in preload_thread() does not cover this kind of change\nthat is_submodule_modified() reports, it shouldn't mark a gitlink entry as\nce_uptodate(ce).\n\nAnother possibility is to run the is_submodule_modified() check inside\nie_match_stat(), but (1) I don't know if that function is thread-safe, and\n(2) I don't think we would want to do it in preload-index time (it is\nrather expensive).\n\nThe reason preload_index() passes CE_MATCH_RACY_IS_DIRTY to ie_match_stat() \nis to avoid doing a rather expensive ce_modified_check_fs() --- it avoids\nthe overhead and leaves the expensive check to the true callers that care\nif the entry is really clean.  In the same sense, even if we were to run\nis_submodule_modified() there, we would want to avoid doing so when we are\nrunning ie_match_stat() from preload codepath.\n\nWe need to also see if there other codepaths that call\nce_mark_uptodate(ce) on a gitlink that hasn't been checked with\nis_submodule_modified(), and eliminate them.\n\nThen we can fix the \"even though ce_uptodate(ce) says this entry is clean,\nif it is gitlink, we need to double check\" insanity around ll 164 of\ndiff-lib.c.  We should be able to trust ce_uptodate(ce) even for gitlinks.\n\nWhat do you think?\n"},{"id":"132526","messageId":"7vljfn9b4j.fsf@alter.siamese.dyndns.org","threadId":"22347","inReplyTo":"7veilg9rqp.fsf@alter.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-24T08:10:20Z","receivedAt":"2010-01-24T08:10:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think we need to clarify the rule for ce_uptodate(ce).\n\nA first step to do so may look like this.\n\n-- >8 --\nSubject: Make ce_uptodate() trustworthy again\n\nThe rule has always been that a cache entry that is ce_uptodate(ce)\nmeans that we already have checked the work tree entity and we know\nthere is no change in the work tree compared to the index, and nobody\nshould have to double check.  Note that false ce_uptodate(ce) does not\nmean it is known to be dirty---it only means we don't know if it is\nclean.\n\nThere are a few codepaths (refresh-index and preload-index are among\nthem) that mark a cache entry as up-to-date based solely on the return\nvalue from ie_match_stat(); this function uses lstat() to see if the\nwork tree entity has been touched, and for a submodule entry, if its\nHEAD points at the same commit as the commit recorded in the index of\nthe superproject (a submodule that is not even cloned is considered\nclean).\n\nA submodule is no longer considered unmodified merely because its HEAD\nmatches the index of the superproject these days, in order to prevent\npeople from forgetting to commit in the submodule and updating the\nsuperproject index with the new submodule commit, before commiting the\nstate in the superproject.  However, the patch to do so didn't update\nthe codepath that marks cache entries up-to-date based on the updated\ndefinition and instead worked it around by saying \"we don't trust the\nreturn value of ce_uptodate() for submodules.\"\n\nThis makes ce_uptodate() trustworthy again by not marking submodule\nentries up-to-date.\n\nThe next step _could_ be to introduce a few \"in-core\" flag bits to\ncache_entry structure to record \"this entry is _known_ to be dirty\",\ncall is_submodule_modified() from ie_match_stat(), and use these new\nbits to avoid running this rather expensive check more than once, but\nthat can be a separate patch.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff-lib.c      |    2 +-\n preload-index.c |    2 ++\n read-cache.c    |    6 ++++--\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 23e180e..c6c425e 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -161,7 +161,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) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce))\n+\t\tif (ce_uptodate(ce) || 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 */\ndiff --git a/preload-index.c b/preload-index.c\nindex 9289933..e3d0bda 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -47,6 +47,8 @@ static void *preload_thread(void *_data)\n \n \t\tif (ce_stage(ce))\n \t\t\tcontinue;\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n \t\tif (ce_uptodate(ce))\n \t\t\tcontinue;\n \t\tif (!ce_path_match(ce, p->pathspec))\ndiff --git a/read-cache.c b/read-cache.c\nindex 79938bf..309b77a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -612,7 +612,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tif (alias && !ce_stage(alias) && !ie_match_stat(istate, alias, st, ce_option)) {\n \t\t/* Nothing changed, really */\n \t\tfree(ce);\n-\t\tce_mark_uptodate(alias);\n+\t\tif (!S_ISGITLINK(alias->ce_mode))\n+\t\t\tce_mark_uptodate(alias);\n \t\talias->ce_flags |= CE_ADDED;\n \t\treturn 0;\n \t}\n@@ -1050,7 +1051,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\t\t * because CE_UPTODATE flag is in-core only;\n \t\t\t * we are not going to write this change out.\n \t\t\t */\n-\t\t\tce_mark_uptodate(ce);\n+\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tce_mark_uptodate(ce);\n \t\t\treturn ce;\n \t\t}\n \t}\n"},{"id":"132539","messageId":"4B5C547C.7000604@web.de","threadId":"22347","inReplyTo":"7v7hr8d1xg.fsf@alter.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-24T14:09:00Z","receivedAt":"2010-01-24T14:09:00Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 23.01.2010 21:03, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>> A patch that teaches \"git diff --submodule\" to display if the submodule\n>> contains new untracked and/or modified files is also almost ready.\n> \n> How does \"git submodule summary\" show them?  If it doesn't, then I don't\n> think we would want to show them, either, as my understanding is that a\n> longer-term plan is to use \"diff --submodule\" in git-gui to replace it.\n\nShawn applied the patch doing that yesterday, now gitk and git gui both\nuse \"git diff --submodule\" and not \"git submodule summary\" anymore. And\nit would be really nice if both of these tools would show the dirtiness\nof submodules in 1.7.0 (and after 1.7.0 i plan to teach \"git submodule\nsummary\" about dirtiness too).\n\n\n>> Would\n>> you consider it for inclusion into 1.7.0 too or shall i wait until after\n>> the release?\n> \n> If a feature is not in 'master' already, I think it is too late to be\n> included in 1.7.0, if that is what you are asking.  But if you start the\n> usual cycle of working on, asking others to review and polishing it before\n> the release, it would give us better designed and more tested version in\n> 1.7.1, no?\n\nSure, I'm fine with having the full functionality in 1.7.1. But a part of\nit looks like a fix to me, let me explain:\n\nRight now \"git diff --submodule\" doesn't show the dirty status of a\nsubmodule at all (like it does when using it without that option and\nhaving paid the cost to get the necessary information). So IMHO something\nlike the patch below should go into 1.7.0 to fix that. When applied the\noutput looks like this:\n\nSubmodule sub 3f35670..3f35670-dirty:\n\nwhich is now consistent with the output of \"git diff\" without that option:\n\ndiff --git a/sub b/sub\n--- a/sub\n+++ b/sub\n@@ -1 +1 @@\n-Subproject commit 3f356705649b5d566d97ff843cf193359229a453\n+Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty\n\n\nThis is a stripped down version of the patch i had in mind. I would post\nthe rest after 1.7.0 to give detailed output about the reasons a submodule\nis dirty.\n\nWhat do you think?\n\n\n-- >8 --\nSubject: Teach diff --submodule that modified submodule directory is dirty\n\nSince commit 8e08b4 git diff does append \"-dirty\" to the work tree side\nif the working directory of a submodule contains new or modified files.\nLets do the same when the --submodule option is used.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n diff.c                    |    2 +-\n submodule.c               |    3 ++\n submodule.h               |    1 +\n t/t4041-diff-submodule.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 72 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f130a36..381cc8d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1615,7 +1615,7 @@ static void builtin_diff(const char *name_a,\n \t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n \t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n-\t\t\t\tone->sha1, two->sha1,\n+\t\t\t\tone->sha1, two->sha1, two->dirty_submodule,\n \t\t\t\tdel, add, reset);\n \t\treturn;\n \t}\ndiff --git a/submodule.c b/submodule.c\nindex f657bee..ca0527f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -36,6 +36,7 @@ static int add_submodule_odb(const char *path)\n\n void show_submodule_summary(FILE *f, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n+\t\tunsigned dirty_submodule,\n \t\tconst char *del, const char *add, const char *reset)\n {\n \tstruct rev_info rev;\n@@ -85,6 +86,8 @@ void show_submodule_summary(FILE *f, const char *path,\n \tif (!fast_backward && !fast_forward)\n \t\tstrbuf_addch(&sb, '.');\n \tstrbuf_addf(&sb, \"%s\", find_unique_abbrev(two, DEFAULT_ABBREV));\n+\tif (dirty_submodule)\n+\t\tstrbuf_add(&sb, \"-dirty\", 6);\n \tif (message)\n \t\tstrbuf_addf(&sb, \" %s\\n\", message);\n \telse\ndiff --git a/submodule.h b/submodule.h\nindex 0773121..2336965 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -3,6 +3,7 @@\n\n void show_submodule_summary(FILE *f, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n+\t\tunsigned dirty_submodule,\n \t\tconst char *del, const char *add, const char *reset);\n int is_submodule_modified(const char *path);\n\ndiff --git a/t/t4041-diff-submodule.sh b/t/t4041-diff-submodule.sh\nindex 5bb4fed..4643054 100755\n--- a/t/t4041-diff-submodule.sh\n+++ b/t/t4041-diff-submodule.sh\n@@ -191,6 +191,73 @@ EOF\n \"\n\n commit_file sm1 &&\n+test_expect_success 'submodule is up to date' \"\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+EOF\n+\"\n+\n+test_expect_success 'submodule contains untracked content' \"\n+\techo new > sm1/new-file &&\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head6-dirty:\n+EOF\n+\"\n+\n+test_expect_success 'submodule contains untracked and modifed content' \"\n+\techo new > sm1/foo6 &&\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head6-dirty:\n+EOF\n+\"\n+\n+test_expect_success 'submodule contains modifed content' \"\n+\trm -f sm1/new-file &&\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head6-dirty:\n+EOF\n+\"\n+\n+(cd sm1; git commit -mchange foo6 >/dev/null) &&\n+head8=$(cd sm1; git rev-parse --verify HEAD | cut -c1-7) &&\n+test_expect_success 'submodule is modified' \"\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head8:\n+  > change\n+EOF\n+\"\n+\n+test_expect_success 'modified submodule contains untracked content' \"\n+\techo new > sm1/new-file &&\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head8-dirty:\n+  > change\n+EOF\n+\"\n+\n+test_expect_success 'modified submodule contains untracked and modifed content' \"\n+\techo modification >> sm1/foo6 &&\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head8-dirty:\n+  > change\n+EOF\n+\"\n+\n+test_expect_success 'modified submodule contains modifed content' \"\n+\trm -f sm1/new-file &&\n+\tgit diff-index -p --submodule=log HEAD >actual &&\n+\tdiff actual - <<-EOF\n+Submodule sm1 $head6..$head8-dirty:\n+  > change\n+EOF\n+\"\n+\n rm -rf sm1\n test_expect_success 'deleted submodule' \"\n \tgit diff-index -p --submodule=log HEAD >actual &&\n-- \n1.6.6.1.559.g4914\n"},{"id":"132614","messageId":"7vwrz62h7g.fsf_-_@alter.siamese.dyndns.org","threadId":"22347","inReplyTo":"4B5C547C.7000604@web.de","subject":"[RFC/H] \"git diff --submodule\" showing submodule work tree dirtiness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-25T18:05:23Z","receivedAt":"2010-01-25T18:05:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Right now \"git diff --submodule\" doesn't show the dirty status of a\n> submodule at all (like it does when using it without that option and\n> having paid the cost to get the necessary information). So IMHO something\n> like the patch below should go into 1.7.0 to fix that. When applied the\n> output looks like this:\n>\n> Submodule sub 3f35670..3f35670-dirty:\n>\n> which is now consistent with the output of \"git diff\" without that option:\n>\n> diff --git a/sub b/sub\n> --- a/sub\n> +++ b/sub\n> @@ -1 +1 @@\n> -Subproject commit 3f356705649b5d566d97ff843cf193359229a453\n> +Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty\n\nI think this is sensible; I queued it to 'pu' to give chance to submodule\nusers comment on it, but I am inclined to say it should be part of 1.7.0\nfor consistency.\n"}]}