{"thread":{"id":"27958","subject":"Unusual behavior from git describe","startedAt":"2011-07-29T21:46:45Z","lastAt":"2011-08-02T22:38:08Z","messageCount":10,"participants":["Allan Caffee","Sverre Rabbelier","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"172362","messageId":"CA+jCPNcOe_dd6fsHDvWtoXEQE+xyd=aaSbfjpjQ8UfyFnvXTfg@mail.gmail.com","threadId":"27958","inReplyTo":null,"subject":"Unusual behavior from git describe","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2011-07-29T21:46:45Z","receivedAt":"2011-07-29T21:46:45Z","isPatch":false,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"I've encountered some strange behavior from git describe. Immediately\nafter building my python project with setuptools tools git describe\nseems to think that the working tree is dirty. However after running\ngit status (and seeing that nothing has changed) and running describe\nagain the dirtyness seems to have disappeared. Any ideas as to what\nmight be going on here?\n\ngit is version 1.7.3.2 on Mac OSX. The full transcript is below.\n\nThanks in advance,\nAllan\n\n% git describe --tags --dirty\nv0.0.3\n\n% python ./setup.py sdist\nrunning sdist\nrunning egg_info\nwriting requirements to flaskapi.egg-info/requires.txt\nwriting flaskapi.egg-info/PKG-INFO\nwriting top-level names to flaskapi.egg-info/top_level.txt\nwriting dependency_links to flaskapi.egg-info/dependency_links.txt\nwriting entry points to flaskapi.egg-info/entry_points.txt\nreading manifest file 'flaskapi.egg-info/SOURCES.txt'\nreading manifest template 'MANIFEST.in'\nwriting manifest file 'flaskapi.egg-info/SOURCES.txt'\nwarning: sdist: standard file not found: should have one of README, README.txt\nwarning: sdist: missing required meta-data: url\ncreating flaskapi-0.0.3\ncreating flaskapi-0.0.3/flaskapi\ncreating flaskapi-0.0.3/flaskapi.egg-info\ncreating flaskapi-0.0.3/flaskapi/apidoc\ncreating flaskapi-0.0.3/flaskapi/testing\ncreating flaskapi-0.0.3/tests\ncreating flaskapi-0.0.3/tests/integration\ncreating flaskapi-0.0.3/tests/unit\ncreating flaskapi-0.0.3/tests/unit/testing\nmaking hard links in flaskapi-0.0.3...\nhard linking MANIFEST.in -> flaskapi-0.0.3\nhard linking RELEASE-VERSION -> flaskapi-0.0.3\nhard linking fabfile.py -> flaskapi-0.0.3\nhard linking setup.cfg -> flaskapi-0.0.3\nhard linking setup.py -> flaskapi-0.0.3\nhard linking version.py -> flaskapi-0.0.3\nhard linking flaskapi/__init__.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi/app.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi/base_view.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi/errors.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi/helpers.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi/make_app.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi/registered_view_metaclass.py -> flaskapi-0.0.3/flaskapi\nhard linking flaskapi.egg-info/PKG-INFO -> flaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi.egg-info/SOURCES.txt -> flaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi.egg-info/dependency_links.txt ->\nflaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi.egg-info/entry_points.txt ->\nflaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi.egg-info/not-zip-safe -> flaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi.egg-info/requires.txt -> flaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi.egg-info/top_level.txt -> flaskapi-0.0.3/flaskapi.egg-info\nhard linking flaskapi/apidoc/__init__.py -> flaskapi-0.0.3/flaskapi/apidoc\nhard linking flaskapi/testing/__init__.py -> flaskapi-0.0.3/flaskapi/testing\nhard linking flaskapi/testing/base_api_test.py ->\nflaskapi-0.0.3/flaskapi/testing\nhard linking flaskapi/testing/json_helpers.py -> flaskapi-0.0.3/flaskapi/testing\nhard linking flaskapi/testing/json_requester.py ->\nflaskapi-0.0.3/flaskapi/testing\nhard linking tests/__init__.py -> flaskapi-0.0.3/tests\nhard linking tests/integration/__init__.py -> flaskapi-0.0.3/tests/integration\nhard linking tests/unit/__init__.py -> flaskapi-0.0.3/tests/unit\nhard linking tests/unit/test_base_view.py -> flaskapi-0.0.3/tests/unit\nhard linking tests/unit/test_errors.py -> flaskapi-0.0.3/tests/unit\nhard linking tests/unit/test_helpers.py -> flaskapi-0.0.3/tests/unit\nhard linking tests/unit/test_make_app.py -> flaskapi-0.0.3/tests/unit\nhard linking tests/unit/test_registered_view_metaclass.py ->\nflaskapi-0.0.3/tests/unit\nhard linking tests/unit/testing/__init__.py -> flaskapi-0.0.3/tests/unit/testing\nhard linking tests/unit/testing/test_base_api_test.py ->\nflaskapi-0.0.3/tests/unit/testing\nhard linking tests/unit/testing/test_json_helpers.py ->\nflaskapi-0.0.3/tests/unit/testing\ncopying setup.cfg -> flaskapi-0.0.3\nWriting flaskapi-0.0.3/setup.cfg\ntar -cf dist/flaskapi-0.0.3.tar flaskapi-0.0.3\ngzip -f9 dist/flaskapi-0.0.3.tar\ntar -cf dist/flaskapi-0.0.3.tar flaskapi-0.0.3\ngzip -f9 dist/flaskapi-0.0.3.tar\nremoving 'flaskapi-0.0.3' (and everything under it)\n\n% git describe --tags --dirty\nv0.0.3-dirty\n\n% git status\n# On branch master\nnothing to commit (working directory clean)\n\n% git describe --tags --dirty\nv0.0.3\n"},{"id":"172363","messageId":"CAGdFq_hYiBoqNmNtBKBqNN4XLLKwxDMHJfAUwdHB_iCcya=DOQ@mail.gmail.com","threadId":"27958","inReplyTo":"CA+jCPNcOe_dd6fsHDvWtoXEQE+xyd=aaSbfjpjQ8UfyFnvXTfg@mail.gmail.com","subject":"Re: Unusual behavior from git describe","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-29T21:55:07Z","receivedAt":"2011-07-29T21:55:07Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Fri, Jul 29, 2011 at 23:46, Allan Caffee wrote:\n> % git describe --tags --dirty\n> v0.0.3-dirty\n>\n> % git status\n> # On branch master\n> nothing to commit (working directory clean)\n>\n> % git describe --tags --dirty\n> v0.0.3\n\nPerhaps git describe does not update the index (properly?), which 'git\nstatus' then does, correcting it?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"172386","messageId":"CA+jCPNfwwhM8R-bB_VnwpaijSMf3BNydH35SqZt3dRb-P1AOmg@mail.gmail.com","threadId":"27958","inReplyTo":"CAGdFq_hYiBoqNmNtBKBqNN4XLLKwxDMHJfAUwdHB_iCcya=DOQ@mail.gmail.com","subject":"Re: Unusual behavior from git describe","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2011-07-30T13:29:58Z","receivedAt":"2011-07-30T13:29:58Z","isPatch":false,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"Hey,\n\nOn Fri, Jul 29, 2011 at 5:55 PM, Sverre Rabbelier wrote:\n> Heya,\n>\n> On Fri, Jul 29, 2011 at 23:46, Allan Caffee wrote:\n>> % git describe --tags --dirty\n>> v0.0.3-dirty\n>>\n>> % git status\n>> # On branch master\n>> nothing to commit (working directory clean)\n>>\n>> % git describe --tags --dirty\n>> v0.0.3\n>\n> Perhaps git describe does not update the index (properly?), which 'git\n> status' then does, correcting it?\n\nI suppose that makes sense. But what about building a package, which\ndoesn't change any tracked files or add any (non-ignored) untracked\nfiles, would cause the index to appear dirty in the first place?\n\n--\nAllan\n"},{"id":"172387","messageId":"CAGdFq_imU3_=E1LK-AG33Tj70iOJBTmt2_qdUKVHL9DVW2yJRQ@mail.gmail.com","threadId":"27958","inReplyTo":"CA+jCPNfwwhM8R-bB_VnwpaijSMf3BNydH35SqZt3dRb-P1AOmg@mail.gmail.com","subject":"Re: Unusual behavior from git describe","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-30T13:32:48Z","receivedAt":"2011-07-30T13:32:48Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Jul 30, 2011 at 15:29, Allan Caffee wrote:\n> I suppose that makes sense.  But what about building a package, which\n> doesn't change any tracked files or add any (non-ignored) untracked\n> files, would cause the index to appear dirty in the first place?\n\nDoes it perhaps touch some of the tracked files? That way it would\nmake sense git at first thinks it's dirty (since the lstat info\nchanged), but then 'git status' will actually check the contents of\nthe file and notice that they're equal? Just guessing here though.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"172404","messageId":"CA+jCPNfsQ2oqDzTpJL4ck0vNBJnfXwb+aaSrzStrs55kq+CTHA@mail.gmail.com","threadId":"27958","inReplyTo":"CAGdFq_imU3_=E1LK-AG33Tj70iOJBTmt2_qdUKVHL9DVW2yJRQ@mail.gmail.com","subject":"Re: Unusual behavior from git describe","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2011-07-30T16:23:30Z","receivedAt":"2011-07-30T16:23:30Z","isPatch":false,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"On Sat, Jul 30, 2011 at 9:32 AM, Sverre Rabbelier wrote:\n> Does it perhaps touch some of the tracked files? That way it would\n> make sense git at first thinks it's dirty (since the lstat info\n> changed), but then 'git status' will actually check the contents of\n> the file and notice that they're equal? Just guessing here though.\n\nSounds like you're on the right track. git diff-index reveals that\nthe index is stale\n\n========>8==============>8=========\n:100644 100644 781975ec321be574e0b016c9e699804430a4cefc\n0000000000000000000000000000000000000000 M\tMANIFEST.in\n:100644 100644 bddeac47f9d67e8496a7e1d76f3e024e644ee332\n0000000000000000000000000000000000000000 M\tfabfile.py\n...\n:100644 100644 4f09e2dbb7b0b7aeb0063468bd1931e5c969d2d6\n0000000000000000000000000000000000000000 M\tflaskapi/__init__.py\n:100644 100644 e4cc0de216a9c5a68ca87650bbe0be24f327df27\n0000000000000000000000000000000000000000 M\tversion.py\n===============>8==============>8==\n\nIt looks like this was caused by setuptools hardlinking files into a\ntemp directory and then deleting the links, as demonstrated by:\n\n% git diff-index HEAD --\n% ln MANIFEST.in file && rm file\n% git diff-index HEAD --\n:100644 100644 781975ec321be574e0b016c9e699804430a4cefc\n0000000000000000000000000000000000000000 M\tMANIFEST.in\n\nI've tried adding a call to refresh_index() in describe.c but it\ndoesn't seem to have any effect on the results. (Patch below.) Any\nidea what the proper fix is for this?\n\n--\nAllan\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 66fc291..73e98ed 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -462,8 +462,11 @@ int cmd_describe(int argc, const char **argv,\nconst char *prefix)\n die(_(\"No names found, cannot describe anything.\"));\n\n if (argc == 0) {\n- if (dirty &&\n!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args,\nprefix))\n- dirty = NULL;\n+ if (dirty) {\n+ refresh_index(&the_index,\nREFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n+ if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) -\n1, diff_index_args, prefix))\n+ dirty = NULL;\n+ }\n describe(\"HEAD\", 1);\n } else if (dirty) {\n die(_(\"--dirty is incompatible with committishes\"));\n"},{"id":"172423","messageId":"20110731062055.GB14384@sigill.intra.peff.net","threadId":"27958","inReplyTo":"CA+jCPNfsQ2oqDzTpJL4ck0vNBJnfXwb+aaSrzStrs55kq+CTHA@mail.gmail.com","subject":"Re: Unusual behavior from git describe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-31T06:20:55Z","receivedAt":"2011-07-31T06:20:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 30, 2011 at 12:23:30PM -0400, Allan Caffee wrote:\n\n> On Sat, Jul 30, 2011 at 9:32 AM, Sverre Rabbelier wrote:\n> > Does it perhaps touch some of the tracked files? That way it would\n> > make sense git at first thinks it's dirty (since the lstat info\n> > changed), but then 'git status' will actually check the contents of\n> > the file and notice that they're equal? Just guessing here though.\n> \n> Sounds like you're on the right track. git diff-index reveals that\n> the index is stale\n> [...]\n> It looks like this was caused by setuptools hardlinking files into a\n> temp directory and then deleting the links, as demonstrated by:\n\nYeah, that would modify the file's ctime, which is part of what git uses\nto check whether its stat-cache is fresh.\n\nThe problem is that we call the diff-index plumbing to determine the\ndirty state, but it expects the index to have been refreshed already.\nDescribe is probably porcelain-ish enough that it should be doing the\nrefresh for the user and writing the result out (at least if the --dirty\nflag is passed, as otherwise it doesn't care), just as porcelains like\n\"diff\" and \"status\" do.\n\n> I've tried adding a call to refresh_index() in describe.c but it\n> doesn't seem to have any effect on the results. (Patch below.) Any\n> idea what the proper fix is for this?\n\nYou call refresh_index, but you never actually load the index in the\nfirst place. So nothing gets refreshed. If you add a call to read_cache\njust beforehand, it works as you expect.\n\nHowever, if describe is going to the trouble to refresh the index, it\nshould probably actually write out the result. In that case, you would\nwant to emulate what cmd_status does in builtin/commit.c, which writes\nout the new index via update_index_if_able.\n\n-Peff\n"},{"id":"172485","messageId":"1312163561-77072-1-git-send-email-allan.caffee@gmail.com","threadId":"27958","inReplyTo":"20110731062055.GB14384@sigill.intra.peff.net","subject":"[PATCH] describe: Refresh the index when run with --dirty","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2011-08-01T01:52:41Z","receivedAt":"2011-08-01T01:52:41Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"When running git describe --dirty the index should be refreshed. Previously\nthe cached index would cause describe to think that the index was dirty when,\nin reality, it was just stale.\n\nThe issue was exposed by python setuptools which hardlinks files into another\ndirectory when building a distribution.\n---\n builtin/describe.c | 14 ++++++++++++--\n 1 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 66fc291..792af76 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -24,6 +24,7 @@ static int longformat;\n static int abbrev = -1; /* unspecified */\n static int max_candidates = 10;\n static struct hash_table names;\n+static struct lock_file index_lock; /* real index */\n static int have_util;\n static const char *pattern;\n static int always;\n@@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one)\n int cmd_describe(int argc, const char **argv, const char *prefix)\n {\n \tint contains = 0;\n+\tint fd;\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"contains\", &contains, \"find the tag that comes after the commit\"),\n \t\tOPT_BOOLEAN(0, \"debug\", &debug, \"debug search strategy on stderr\"),\n@@ -462,8 +464,16 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"No names found, cannot describe anything.\"));\n \n \tif (argc == 0) {\n-\t\tif (dirty && !cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))\n-\t\t\tdirty = NULL;\n+\t\tif (dirty) {\n+\t\t\tread_cache();\n+\t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n+\t\t\tfd = hold_locked_index(&index_lock, 0);\n+\t\t\tif (0 <= fd)\n+\t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n+\n+\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))\n+\t\t\t\tdirty = NULL;\n+\t\t}\n \t\tdescribe(\"HEAD\", 1);\n \t} else if (dirty) {\n \t\tdie(_(\"--dirty is incompatible with committishes\"));\n-- \n1.7.3.2\n"},{"id":"172496","messageId":"20110801035153.GA2207@sigill.intra.peff.net","threadId":"27958","inReplyTo":"1312163561-77072-1-git-send-email-allan.caffee@gmail.com","subject":"Re: [PATCH] describe: Refresh the index when run with --dirty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-01T03:51:54Z","receivedAt":"2011-08-01T03:51:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 31, 2011 at 09:52:41PM -0400, Allan Caffee wrote:\n\n> When running git describe --dirty the index should be refreshed. Previously\n> the cached index would cause describe to think that the index was dirty when,\n> in reality, it was just stale.\n> \n> The issue was exposed by python setuptools which hardlinks files into another\n> directory when building a distribution.\n\nOverall, looks good to me. A few minor nits, though:\n\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 66fc291..792af76 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -24,6 +24,7 @@ static int longformat;\n> static int abbrev = -1; /* unspecified */\n> static int max_candidates = 10;\n> static struct hash_table names;\n> +static struct lock_file index_lock; /* real index */\n\nThis line was presumably copied straight from builtin/commit.c. You can\ndrop the \"real index\" comment here. Commit may deal with multiple\nindices, which is what this comment was clarifying, but here it doesn't\nmake any sense.\n\n> static int always;\n> @@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one)\n> int cmd_describe(int argc, const char **argv, const char *prefix)\n> {\n> \tint contains = 0;\n> +\tint fd;\n\nIf a variable is only going to be used for one deep conditional, IMHO\nit's nice to declare it inside the conditional block, so readers of the\ncode don't have to wonder under what conditions fd is valid.\n\n> +\t\tif (dirty) {\n> +\t\t\tread_cache();\n> +\t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n> +\t\t\tfd = hold_locked_index(&index_lock, 0);\n> +\t\t\tif (0 <= fd)\n> +\t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n\nA few questions about this read_cache call:\n\n 1. Should this actually be:\n\n if (read_cache() < 0)\n die(\"unable to read cache\");\n\n ? I notice that cmd_status also does not check the error code. But\n it seems like if we fail to read, we would then potentially write\n out a bogus index. Probably unlikely, as failure to read probably\n implies failure to write.\n\n 2. Should the read and refresh happen while we hold the lock?\n Otherwise our read-modify-update is not atomic, and we risk\n overwriting another index writer. Again, cmd_status suffers from\n the same problem, so this is not something you are introducing.\n\n 3. Is there any reason not to use the multi-threaded\n read_cache_preload here?\n\n-Peff\n"},{"id":"172702","messageId":"7v1ux3eapk.fsf@alter.siamese.dyndns.org","threadId":"27958","inReplyTo":"1312163561-77072-1-git-send-email-allan.caffee@gmail.com","subject":"Re: [PATCH] describe: Refresh the index when run with --dirty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T21:59:35Z","receivedAt":"2011-08-02T21:59:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\nHere is a minor fix-up on top, that can be squashed in a re-roll (if you\nplan to do one).\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 792af76..9f63067 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -24,7 +24,6 @@ static int longformat;\n static int abbrev = -1; /* unspecified */\n static int max_candidates = 10;\n static struct hash_table names;\n-static struct lock_file index_lock; /* real index */\n static int have_util;\n static const char *pattern;\n static int always;\n@@ -400,7 +399,6 @@ static void describe(const char *arg, int last_one)\n int cmd_describe(int argc, const char **argv, const char *prefix)\n {\n \tint contains = 0;\n-\tint fd;\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"contains\", &contains, \"find the tag that comes after the commit\"),\n \t\tOPT_BOOLEAN(0, \"debug\", &debug, \"debug search strategy on stderr\"),\n@@ -465,13 +463,18 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \tif (argc == 0) {\n \t\tif (dirty) {\n-\t\t\tread_cache();\n-\t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n+\t\t\tstatic struct lock_file index_lock;\n+\t\t\tint fd;\n+\n+\t\t\tread_cache_preload(NULL);\n+\t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n+\t\t\t\t NULL, NULL, NULL);\n \t\t\tfd = hold_locked_index(&index_lock, 0);\n \t\t\tif (0 <= fd)\n \t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n \n-\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))\n+\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n+\t\t\t\t\t diff_index_args, prefix))\n \t\t\t\tdirty = NULL;\n \t\t}\n \t\tdescribe(\"HEAD\", 1);\n"},{"id":"172717","messageId":"CA+jCPNeQ-ry9Cq2fL8sbnnuzBQO4MP=VkXrXTJT3H9zSFtQYSw@mail.gmail.com","threadId":"27958","inReplyTo":"7v1ux3eapk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] describe: Refresh the index when run with --dirty","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2011-08-02T22:38:08Z","receivedAt":"2011-08-02T22:38:08Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"On Tue, Aug 2, 2011 at 5:59 PM, Junio C Hamano wrote:\n> Thanks.\n>\n> Here is a minor fix-up on top, that can be squashed in a re-roll (if you\n> plan to do one).\n\nThanks for the patch. I'll squash this into v2.\n\nOn Sun, Jul 31, 2011 at 11:51 PM, Jeff King wrote:\n> On Sun, Jul 31, 2011 at 09:52:41PM -0400, Allan Caffee wrote:\n>\n>> When running git describe --dirty the index should be refreshed.  Previously\n>> the cached index would cause describe to think that the index was dirty when,\n>> in reality, it was just stale.\n>>\n>> The issue was exposed by python setuptools which hardlinks files into another\n>> directory when building a distribution.\n>\n> Overall, looks good to me. A few minor nits, though:\n>\n>> diff --git a/builtin/describe.c b/builtin/describe.c\n>> index 66fc291..792af76 100644\n>> --- a/builtin/describe.c\n>> +++ b/builtin/describe.c\n>> @@ -24,6 +24,7 @@ static int longformat;\n>>  static int abbrev = -1; /* unspecified */\n>>  static int max_candidates = 10;\n>>  static struct hash_table names;\n>> +static struct lock_file index_lock; /* real index */\n>\n> This line was presumably copied straight from builtin/commit.c. You can\n> drop the \"real index\" comment here. Commit may deal with multiple\n> indices, which is what this comment was clarifying, but here it doesn't\n> make any sense.\n>\n>>  static int always;\n>> @@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one)\n>>  int cmd_describe(int argc, const char **argv, const char *prefix)\n>>  {\n>>       int contains = 0;\n>> +     int fd;\n>\n> If a variable is only going to be used for one deep conditional, IMHO\n> it's nice to declare it inside the conditional block, so readers of the\n> code don't have to wonder under what conditions fd is valid.\n>\n>> +             if (dirty) {\n>> +                     read_cache();\n>> +                     refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n>> +                     fd = hold_locked_index(&index_lock, 0);\n>> +                     if (0 <= fd)\n>> +                             update_index_if_able(&the_index, &index_lock);\n>\n> A few questions about this read_cache call:\n>\n>  1. Should this actually be:\n>\n>          if (read_cache() < 0)\n>                  die(\"unable to read cache\");\n>\n>     ? I notice that cmd_status also does not check the error code. But\n>     it seems like if we fail to read, we would then potentially write\n>     out a bogus index. Probably unlikely, as failure to read probably\n>     implies failure to write.\n\nIt definitely seems like writing out a bogus index would be bad, but\neven if both the read *and* the write fail we would still be\npotentially mislabeling it as \"dirty\" if we failed to it in the first\nplace. It seems like, since they explicitly requested --dirty, we\nought to give up here since we can't accurately respond.\n\n>  2. Should the read and refresh happen while we hold the lock?\n>     Otherwise our read-modify-update is not atomic, and we risk\n>     overwriting another index writer. Again, cmd_status suffers from\n>     the same problem, so this is not something you are introducing.\n\nYeah it definitely is a race condition as far as I can tell. Should\nthis be changed in cmd_status (builtin/commit.c:1227-1232) as well?\n\n--\nAllan\n"}]}