{"thread":{"id":"22271","subject":"[PATCH v2] Performance optimization for detection of modified submodules","startedAt":"2010-01-18T20:26:18Z","lastAt":"2010-01-19T18:09:57Z","messageCount":4,"participants":["Jens Lehmann","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"132045","messageId":"4B54C3EA.9090200@web.de","threadId":"22271","inReplyTo":null,"subject":"[PATCH v2] Performance optimization for detection of modified submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-18T20:26:18Z","receivedAt":"2010-01-18T20:26:18Z","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\nNew parameters have 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\nWhen no explicit \"-dirty\" is needed in the output the call to\nis_submodule_modified() is not necessary when the submodules HEAD\nalready disagrees with the ref of the superproject, as this alone\nmarks it as modified. To achieve that, get_stat_data() got an extra\nargument.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nChanged since the first version:\n\n- diff_change() now has two dirty_submodule arguments, one for\n  each side.\n\n- If the submodules HEAD does not match the ref it is compared\n  against, is_submodule_modified() is only called when the\n  patch formatted output is requested.\n\n- Changed the type of the new dirty_submodule flags to unsigned.\n\n- Updated the commit message\n\n\n diff-lib.c  |   46 +++++++++++++++++++++++++++++++---------------\n diff.c      |   15 +++++++++++----\n diff.h      |   10 ++++++----\n diffcore.h  |    1 +\n revision.c  |    5 +++--\n tree-diff.c |    8 ++++----\n 6 files changed, 56 insertions(+), 29 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 29c5915..ec2e2ac 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\tunsigned 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,16 @@ 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)\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+\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 +193,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, 0, dirty_submodule);\n\n \t}\n \tdiffcore_std(&revs->diffopt);\n@@ -204,16 +209,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 unsigned 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 unsigned *dirty_submodule, int output_format)\n {\n \tconst unsigned char *sha1 = ce->sha1;\n \tunsigned int mode = ce->ce_mode;\n@@ -233,8 +240,13 @@ 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)\n+\t\t    && (!changed || (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+\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 +263,17 @@ static void show_new_file(struct rev_info *revs,\n {\n \tconst unsigned char *sha1;\n \tunsigned int mode;\n+\tunsigned 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, revs->diffopt.output_format) < 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 +284,13 @@ static int show_modified(struct rev_info *revs,\n {\n \tunsigned int mode, oldmode;\n \tconst unsigned char *sha1;\n+\tunsigned 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, revs->diffopt.output_format) < 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 +325,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, 0, dirty_submodule);\n \treturn 0;\n }\n\n@@ -356,7 +372,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..f130a36 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, unsigned 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,8 @@ 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,\n+\t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n\n@@ -3793,6 +3796,8 @@ void diff_change(struct diff_options *options,\n \t\tconst unsigned char *tmp_c;\n \t\ttmp = old_mode; old_mode = new_mode; new_mode = tmp;\n \t\ttmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;\n+\t\ttmp = old_dirty_submodule; old_dirty_submodule = new_dirty_submodule;\n+\t\t\tnew_dirty_submodule = tmp;\n \t}\n\n \tif (options->prefix &&\n@@ -3803,6 +3808,8 @@ 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+\tone->dirty_submodule = old_dirty_submodule;\n+\ttwo->dirty_submodule = new_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..2ef3341 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -14,12 +14,13 @@ 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,\n+\t\t unsigned old_dirty_submodule, unsigned new_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, unsigned 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 +178,14 @@ 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, unsigned 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,\n+\t\t\tunsigned dirty_submodule1, unsigned dirty_submodule2);\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..769cfd4 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, unsigned dirty_submodule)\n {\n \tint diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;\n\n@@ -281,7 +281,8 @@ 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,\n+\t\t unsigned old_dirty_submodule, unsigned new_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..fe9f52c 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, 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, 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.444.g4e34e.dirty\n"},{"id":"132061","messageId":"7v1vhmg975.fsf@alter.siamese.dyndns.org","threadId":"22271","inReplyTo":"4B54C3EA.9090200@web.de","subject":"Re: [PATCH v2] Performance optimization for detection of modified submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-19T01:44:14Z","receivedAt":"2010-01-19T01:44:14Z","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> - Changed the type of the new dirty_submodule flags to unsigned.\n\nWhy?  An unsigned used in place of a bool raises an eyebrow as it is more\ncommon to use \"int\" (the most natural type on the platform).  Going\nagainst tradition makes readers waste time wondering if there were some\nother reason why the code couldn't use \"int\" and had to use \"unsigned\"\n(e.g. \"Hmmpphh, it looked like a mere boolean but the use of 'unsigned'\nsuggests there might be something deeper going on.  Is this used as a\nbitfield?  Does this count and cannot go negative?\").\n\n> @@ -173,12 +174,16 @@ 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\nRemoved from work tree; cannot be dirty---good.\n\n> +\t\tif (S_ISGITLINK(ce->ce_mode)\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> +\t\t\tdirty_submodule = 1;\n> +\t\t}\n\nIf the index records a submodule commit, and the commit checked out in the\nsubmodule is different from that commit, ce_compare_gitlink() called from\nce_match_stat() would have already said \"changed\".  If we want \"-dirty\",\nwe need to call is_submodule_modified(), but otherwise we don't.  Looks\ngood.\n\nDoes DIFF_FORMAT_PATCH cover all cases where we may want a reliable value\nin \"dirty_submodule\" here?  Should use of \"--submodule=log\" affect this\ndecision?\n\n> @@ -188,7 +193,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, 0, dirty_submodule);\n\nLHS is always index and RHS is work tree whose dirtiness is in\ndirty_submodule; good.\n\n> @@ -204,16 +209,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 unsigned 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\nMental note to myself.  prefix[0] is either '-' (removed from the work\ntree) or '+' (added to the work tree).  Added submodule could be\nimmediately dirty.\n\n> @@ -251,15 +263,17 @@ static void show_new_file(struct rev_info *revs,\n>  {\n> ...\n> -\tdiff_index_show_file(revs, \"+\", new, sha1, mode);\n> +\tdiff_index_show_file(revs, \"+\", new, sha1, mode, dirty_submodule);\n\nAnd this caller handles that case correctly; good.\n\n> @@ -270,11 +284,13 @@ static int show_modified(struct rev_info *revs,\n>  {\n>  \tunsigned int mode, oldmode;\n>  \tconst unsigned char *sha1;\n> +\tunsigned 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, revs->diffopt.output_format) < 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\nAgain, removed from work tree cannot be dirty; good.\n\n> @@ -309,7 +325,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, 0, dirty_submodule);\n>  \treturn 0;\n>  }\n\nComparing between a tree entry and either an index entry or a file in the\nwork tree.  When cached (i.e. not looking at work tree), get_stat_data()\ndoesn't touch dirty_submodule, so this won't make noises about submodule\ndirtyness, which is good.\n\n> @@ -356,7 +372,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\nRemoved from the work tree and canont be dirty; good.\n\n> diff --git a/diff.c b/diff.c\n> index 012b3d3..f130a36 100644\n> --- a/diff.c\n> +++ b/diff.c\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, unsigned dirty_submodule)\n\nA removed submodule cannot be dirty, so we can get away with only one\ndirty_submodule that always refers to the RHS (i.e. \"two\"); I see.\n\n> diff --git a/diffcore.h b/diffcore.h\n> index 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\nBy the way, we might want to consolidate these bitfields into a single \n\n\tunsigned should_free:1,\n        \t should_munmap:1,\n                 dirty_submodule:1;\n\nin a separate clean-up patch, after all the dust settles.\n"},{"id":"132088","messageId":"4B556ED2.7020904@web.de","threadId":"22271","inReplyTo":"7v1vhmg975.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Performance optimization for detection of modified submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-01-19T08:35:30Z","receivedAt":"2010-01-19T08:35:30Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 19.01.2010 02:44, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>> - Changed the type of the new dirty_submodule flags to unsigned.\n> \n> Why?  An unsigned used in place of a bool raises an eyebrow as it is more\n> common to use \"int\" (the most natural type on the platform).  Going\n> against tradition makes readers waste time wondering if there were some\n> other reason why the code couldn't use \"int\" and had to use \"unsigned\"\n> (e.g. \"Hmmpphh, it looked like a mere boolean but the use of 'unsigned'\n> suggests there might be something deeper going on.  Is this used as a\n> bitfield?  Does this count and cannot go negative?\").\n\nI am working on a subsequent patch where dirty_submodule is used as a\nbitfield to store more detailed information about the dirtiness. That is\nthen used by \"git diff --submodule\" to tell the user if the submodule\ncontains untracked and/or modified files.\n\nBut i can revert that part if you want.\n\n\n>> +\t\tif (S_ISGITLINK(ce->ce_mode)\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>> +\t\t\tdirty_submodule = 1;\n>> +\t\t}\n> \n> If the index records a submodule commit, and the commit checked out in the\n> submodule is different from that commit, ce_compare_gitlink() called from\n> ce_match_stat() would have already said \"changed\".  If we want \"-dirty\",\n> we need to call is_submodule_modified(), but otherwise we don't.  Looks\n> good.\n> \n> Does DIFF_FORMAT_PATCH cover all cases where we may want a reliable value\n> in \"dirty_submodule\" here?  Should use of \"--submodule=log\" affect this\n> decision?\n\nIn my testing \"git diff --submodule=log\" has DIFF_FORMAT_PATCH set too.\n\n\n>> @@ -204,16 +209,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 unsigned 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> Mental note to myself.  prefix[0] is either '-' (removed from the work\n> tree) or '+' (added to the work tree).  Added submodule could be\n> immediately dirty.\n\nShould i add a comment there?\n\n\n>> diff --git a/diffcore.h b/diffcore.h\n>> index 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> By the way, we might want to consolidate these bitfields into a single \n> \n> \tunsigned should_free:1,\n>         \t should_munmap:1,\n>                  dirty_submodule:1;\n> \n> in a separate clean-up patch, after all the dust settles.\n\nOkay.\n"},{"id":"132124","messageId":"7vpr569dai.fsf@alter.siamese.dyndns.org","threadId":"22271","inReplyTo":"4B556ED2.7020904@web.de","subject":"Re: [PATCH v2] Performance optimization for detection of modified submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-19T18:09:57Z","receivedAt":"2010-01-19T18: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> I am working on a subsequent patch where dirty_submodule is used as a\n> bitfield to store more detailed information about the dirtiness. That is\n> then used by \"git diff --submodule\" to tell the user if the submodule\n> contains untracked and/or modified files.\n>\n> But i can revert that part if you want.\n\n\"Will be used as bitfield in later patches\" is a justification that was\nmissing; with that I think the patch is Ok as-is.\n\n>> Mental note to myself.  prefix[0] is either '-' (removed from the work\n>> tree) or '+' (added to the work tree).  Added submodule could be\n>> immediately dirty.\n>\n> Should i add a comment there?\n\nMeh, that was just me thinking aloud.\n\nThanks.\n"}]}