threads / discuss / 27958

Unusual behavior from git describe

Subject: Unusual behavior from git describe

## tl;dr

10 messages between Jul 29, 2011 and Aug 2, 2011.

replies: 9people: 4as markdown or json

Allan Caffee· Jul 29, 2011, 21:46 UTC · lore

I've encountered some strange behavior from git describe. Immediately after building my python project with setuptools tools git describe seems to think that the working tree is dirty. However after running git status (and seeing that nothing has changed) and running describe again the dirtyness seems to have disappeared. Any ideas as to what might be going on here?

git is version 1.7.3.2 on Mac OSX.  The full transcript is below.

Thanks in advance, Allan

% git describe --tags --dirty v0.0.3

% python ./setup.py sdist running sdist running egg_info writing requirements to flaskapi.egg-info/requires.txt writing flaskapi.egg-info/PKG-INFO writing top-level names to flaskapi.egg-info/top_level.txt writing dependency_links to flaskapi.egg-info/dependency_links.txt writing entry points to flaskapi.egg-info/entry_points.txt reading manifest file 'flaskapi.egg-info/SOURCES.txt' reading manifest template 'MANIFEST.in' writing manifest file 'flaskapi.egg-info/SOURCES.txt' warning: sdist: standard file not found: should have one of README, README.txt warning: sdist: missing required meta-data: url creating flaskapi-0.0.3 creating flaskapi-0.0.3/flaskapi creating flaskapi-0.0.3/flaskapi.egg-info creating flaskapi-0.0.3/flaskapi/apidoc creating flaskapi-0.0.3/flaskapi/testing creating flaskapi-0.0.3/tests creating flaskapi-0.0.3/tests/integration creating flaskapi-0.0.3/tests/unit creating flaskapi-0.0.3/tests/unit/testing making hard links in flaskapi-0.0.3... hard linking MANIFEST.in -> flaskapi-0.0.3 hard linking RELEASE-VERSION -> flaskapi-0.0.3 hard linking fabfile.py -> flaskapi-0.0.3 hard linking setup.cfg -> flaskapi-0.0.3 hard linking setup.py -> flaskapi-0.0.3 hard linking version.py -> flaskapi-0.0.3 hard linking flaskapi/__init__.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi/app.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi/base_view.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi/errors.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi/helpers.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi/make_app.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi/registered_view_metaclass.py -> flaskapi-0.0.3/flaskapi hard linking flaskapi.egg-info/PKG-INFO -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi.egg-info/SOURCES.txt -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi.egg-info/dependency_links.txt -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi.egg-info/entry_points.txt -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi.egg-info/not-zip-safe -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi.egg-info/requires.txt -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi.egg-info/top_level.txt -> flaskapi-0.0.3/flaskapi.egg-info hard linking flaskapi/apidoc/__init__.py -> flaskapi-0.0.3/flaskapi/apidoc hard linking flaskapi/testing/__init__.py -> flaskapi-0.0.3/flaskapi/testing hard linking flaskapi/testing/base_api_test.py -> flaskapi-0.0.3/flaskapi/testing hard linking flaskapi/testing/json_helpers.py -> flaskapi-0.0.3/flaskapi/testing hard linking flaskapi/testing/json_requester.py -> flaskapi-0.0.3/flaskapi/testing hard linking tests/__init__.py -> flaskapi-0.0.3/tests hard linking tests/integration/__init__.py -> flaskapi-0.0.3/tests/integration hard linking tests/unit/__init__.py -> flaskapi-0.0.3/tests/unit hard linking tests/unit/test_base_view.py -> flaskapi-0.0.3/tests/unit hard linking tests/unit/test_errors.py -> flaskapi-0.0.3/tests/unit hard linking tests/unit/test_helpers.py -> flaskapi-0.0.3/tests/unit hard linking tests/unit/test_make_app.py -> flaskapi-0.0.3/tests/unit hard linking tests/unit/test_registered_view_metaclass.py -> flaskapi-0.0.3/tests/unit hard linking tests/unit/testing/__init__.py -> flaskapi-0.0.3/tests/unit/testing hard linking tests/unit/testing/test_base_api_test.py -> flaskapi-0.0.3/tests/unit/testing hard linking tests/unit/testing/test_json_helpers.py -> flaskapi-0.0.3/tests/unit/testing copying setup.cfg -> flaskapi-0.0.3 Writing flaskapi-0.0.3/setup.cfg tar -cf dist/flaskapi-0.0.3.tar flaskapi-0.0.3 gzip -f9 dist/flaskapi-0.0.3.tar tar -cf dist/flaskapi-0.0.3.tar flaskapi-0.0.3 gzip -f9 dist/flaskapi-0.0.3.tar removing 'flaskapi-0.0.3' (and everything under it)

% git describe --tags --dirty v0.0.3-dirty

% git status # On branch master nothing to commit (working directory clean)

% git describe --tags --dirty v0.0.3

Sverre Rabbelier· Jul 29, 2011, 21:55 UTC · re: Allan Caffee · lore

Re: Unusual behavior from git describe

Heya,
On Fri, Jul 29, 2011 at 23:46, Allan Caffee <allan.caffee@gmail.com> wrote:
Show 9 quoted lines
> % git describe --tags --dirty
> v0.0.3-dirty
>
> % git status
> # On branch master
> nothing to commit (working directory clean)
>
> % git describe --tags --dirty
> v0.0.3

Perhaps git describe does not update the index (properly?), which 'git status' then does, correcting it?

-- 
Cheers,

Sverre Rabbelier
Allan Caffee· Jul 30, 2011, 13:29 UTC · re: Sverre Rabbelier · lore

Re: Unusual behavior from git describe

Hey,
On Fri, Jul 29, 2011 at 5:55 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 15 quoted lines
> Heya,
>
> On Fri, Jul 29, 2011 at 23:46, Allan Caffee <allan.caffee@gmail.com> wrote:
>> % git describe --tags --dirty
>> v0.0.3-dirty
>>
>> % git status
>> # On branch master
>> nothing to commit (working directory clean)
>>
>> % git describe --tags --dirty
>> v0.0.3
>
> Perhaps git describe does not update the index (properly?), which 'git
> status' then does, correcting it?

I suppose that makes sense. But what about building a package, which doesn't change any tracked files or add any (non-ignored) untracked files, would cause the index to appear dirty in the first place?

-- Allan

Sverre Rabbelier· Jul 30, 2011, 13:32 UTC · re: Allan Caffee · lore

Re: Unusual behavior from git describe

Heya,
On Sat, Jul 30, 2011 at 15:29, Allan Caffee <allan.caffee@gmail.com> wrote:
> I suppose that makes sense.  But what about building a package, which
> doesn't change any tracked files or add any (non-ignored) untracked
> files, would cause the index to appear dirty in the first place?

Does it perhaps touch some of the tracked files? That way it would make sense git at first thinks it's dirty (since the lstat info changed), but then 'git status' will actually check the contents of the file and notice that they're equal? Just guessing here though.

-- 
Cheers,

Sverre Rabbelier
Allan Caffee· Jul 30, 2011, 16:23 UTC · re: Sverre Rabbelier · lore

Re: Unusual behavior from git describe

On Sat, Jul 30, 2011 at 9:32 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Does it perhaps touch some of the tracked files? That way it would
> make sense git at first thinks it's dirty (since the lstat info
> changed), but then 'git status' will actually check the contents of
> the file and notice that they're equal? Just guessing here though.

Sounds like you're on the right track. git diff-index reveals that the index is stale

========>8==============>8========= :100644 100644 781975ec321be574e0b016c9e699804430a4cefc 0000000000000000000000000000000000000000 M MANIFEST.in :100644 100644 bddeac47f9d67e8496a7e1d76f3e024e644ee332 0000000000000000000000000000000000000000 M fabfile.py ... :100644 100644 4f09e2dbb7b0b7aeb0063468bd1931e5c969d2d6 0000000000000000000000000000000000000000 M flaskapi/__init__.py :100644 100644 e4cc0de216a9c5a68ca87650bbe0be24f327df27 0000000000000000000000000000000000000000 M version.py ===============>8==============>8==

It looks like this was caused by setuptools hardlinking files into a temp directory and then deleting the links, as demonstrated by:

% git diff-index HEAD -- % ln MANIFEST.in file && rm file % git diff-index HEAD -- :100644 100644 781975ec321be574e0b016c9e699804430a4cefc 0000000000000000000000000000000000000000 M MANIFEST.in

I've tried adding a call to refresh_index() in describe.c but it doesn't seem to have any effect on the results. (Patch below.) Any idea what the proper fix is for this?

-- Allan

diff --git a/builtin/describe.c b/builtin/describe.c
index 66fc291..73e98ed 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -462,8 +462,11 @@ int cmd_describe(int argc, const char **argv,
const char *prefix)
                die(_("No names found, cannot describe anything."));

        if (argc == 0) {
-               if (dirty &&
!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args,
prefix))
-                       dirty = NULL;
+               if (dirty) {
+                   refresh_index(&the_index,
REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+                   if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) -
1, diff_index_args, prefix))
+                           dirty = NULL;
+               }
                describe("HEAD", 1);
        } else if (dirty) {
                die(_("--dirty is incompatible with committishes"));
Jeff King· Jul 31, 2011, 06:20 UTC · re: Allan Caffee · lore

Re: Unusual behavior from git describe

On Sat, Jul 30, 2011 at 12:23:30PM -0400, Allan Caffee wrote:
Show 11 quoted lines
> On Sat, Jul 30, 2011 at 9:32 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> > Does it perhaps touch some of the tracked files? That way it would
> > make sense git at first thinks it's dirty (since the lstat info
> > changed), but then 'git status' will actually check the contents of
> > the file and notice that they're equal? Just guessing here though.
> 
> Sounds like you're on the right track.  git diff-index reveals that
> the index is stale
> [...]
> It looks like this was caused by setuptools hardlinking files into a
> temp directory and then deleting the links, as demonstrated by:

Yeah, that would modify the file's ctime, which is part of what git uses to check whether its stat-cache is fresh.

The problem is that we call the diff-index plumbing to determine the dirty state, but it expects the index to have been refreshed already. Describe is probably porcelain-ish enough that it should be doing the refresh for the user and writing the result out (at least if the --dirty flag is passed, as otherwise it doesn't care), just as porcelains like "diff" and "status" do.

> I've tried adding a call to refresh_index() in describe.c but it
> doesn't seem to have any effect on the results. (Patch below.)  Any
> idea what the proper fix is for this?

You call refresh_index, but you never actually load the index in the first place. So nothing gets refreshed. If you add a call to read_cache just beforehand, it works as you expect.

However, if describe is going to the trouble to refresh the index, it should probably actually write out the result. In that case, you would want to emulate what cmd_status does in builtin/commit.c, which writes out the new index via update_index_if_able.

-Peff
Allan Caffee· Aug 1, 2011, 01:52 UTC · re: Jeff King · lore

[PATCH] describe: Refresh the index when run with --dirty

When running git describe --dirty the index should be refreshed. Previously the cached index would cause describe to think that the index was dirty when, in reality, it was just stale.

The issue was exposed by python setuptools which hardlinks files into another
directory when building a distribution.
---
 builtin/describe.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 66fc291..792af76 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -24,6 +24,7 @@ static int longformat;
 static int abbrev = -1; /* unspecified */
 static int max_candidates = 10;
 static struct hash_table names;
+static struct lock_file index_lock; /* real index */
 static int have_util;
 static const char *pattern;
 static int always;
@@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one)
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
+	int fd;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "contains",   &contains, "find the tag that comes after the commit"),
 		OPT_BOOLEAN(0, "debug",      &debug, "debug search strategy on stderr"),
@@ -462,8 +464,16 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		die(_("No names found, cannot describe anything."));
 
 	if (argc == 0) {
-		if (dirty && !cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))
-			dirty = NULL;
+		if (dirty) {
+			read_cache();
+			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+			fd = hold_locked_index(&index_lock, 0);
+			if (0 <= fd)
+				update_index_if_able(&the_index, &index_lock);
+
+			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))
+				dirty = NULL;
+		}
 		describe("HEAD", 1);
 	} else if (dirty) {
 		die(_("--dirty is incompatible with committishes"));

-- 
1.7.3.2
Jeff King· Aug 1, 2011, 03:51 UTC · re: Allan Caffee · lore

Re: [PATCH] describe: Refresh the index when run with --dirty

On Sun, Jul 31, 2011 at 09:52:41PM -0400, Allan Caffee wrote:
Show 6 quoted lines
> When running git describe --dirty the index should be refreshed.  Previously
> the cached index would cause describe to think that the index was dirty when,
> in reality, it was just stale.
> 
> The issue was exposed by python setuptools which hardlinks files into another
> directory when building a distribution.
Overall, looks good to me. A few minor nits, though:
Show 9 quoted lines
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 66fc291..792af76 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -24,6 +24,7 @@ static int longformat;
>  static int abbrev = -1; /* unspecified */
>  static int max_candidates = 10;
>  static struct hash_table names;
> +static struct lock_file index_lock; /* real index */

This line was presumably copied straight from builtin/commit.c. You can drop the "real index" comment here. Commit may deal with multiple indices, which is what this comment was clarifying, but here it doesn't make any sense.

Show 6 quoted lines
>  static int always;
> @@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one)
>  int cmd_describe(int argc, const char **argv, const char *prefix)
>  {
>  	int contains = 0;
> +	int fd;

If a variable is only going to be used for one deep conditional, IMHO it's nice to declare it inside the conditional block, so readers of the code don't have to wonder under what conditions fd is valid.

Show 6 quoted lines
> +		if (dirty) {
> +			read_cache();
> +			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
> +			fd = hold_locked_index(&index_lock, 0);
> +			if (0 <= fd)
> +				update_index_if_able(&the_index, &index_lock);
A few questions about this read_cache call:
  1. Should this actually be:
          if (read_cache() < 0)
                  die("unable to read cache");
     ? I notice that cmd_status also does not check the error code. But
     it seems like if we fail to read, we would then potentially write
     out a bogus index. Probably unlikely, as failure to read probably
     implies failure to write.
  2. Should the read and refresh happen while we hold the lock?
     Otherwise our read-modify-update is not atomic, and we risk
     overwriting another index writer. Again, cmd_status suffers from
     the same problem, so this is not something you are introducing.
  3. Is there any reason not to use the multi-threaded
     read_cache_preload here?
-Peff
Junio C Hamano· Aug 2, 2011, 21:59 UTC · re: Allan Caffee · lore

Re: [PATCH] describe: Refresh the index when run with --dirty

Thanks.

Here is a minor fix-up on top, that can be squashed in a re-roll (if you plan to do one).

diff --git a/builtin/describe.c b/builtin/describe.c
index 792af76..9f63067 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -24,7 +24,6 @@ static int longformat;
 static int abbrev = -1; /* unspecified */
 static int max_candidates = 10;
 static struct hash_table names;
-static struct lock_file index_lock; /* real index */
 static int have_util;
 static const char *pattern;
 static int always;
@@ -400,7 +399,6 @@ static void describe(const char *arg, int last_one)
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
-	int fd;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "contains",   &contains, "find the tag that comes after the commit"),
 		OPT_BOOLEAN(0, "debug",      &debug, "debug search strategy on stderr"),
@@ -465,13 +463,18 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 	if (argc == 0) {
 		if (dirty) {
-			read_cache();
-			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+			static struct lock_file index_lock;
+			int fd;
+
+			read_cache_preload(NULL);
+			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,
+				      NULL, NULL, NULL);
 			fd = hold_locked_index(&index_lock, 0);
 			if (0 <= fd)
 				update_index_if_able(&the_index, &index_lock);
 
-			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))
+			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
+					    diff_index_args, prefix))
 				dirty = NULL;
 		}
 		describe("HEAD", 1);
Allan Caffee· Aug 2, 2011, 22:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] describe: Refresh the index when run with --dirty

On Tue, Aug 2, 2011 at 5:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> Here is a minor fix-up on top, that can be squashed in a re-roll (if you
> plan to do one).
Thanks for the patch.  I'll squash this into v2.
On Sun, Jul 31, 2011 at 11:51 PM, Jeff King <peff@peff.net> wrote:
Show 55 quoted lines
> On Sun, Jul 31, 2011 at 09:52:41PM -0400, Allan Caffee wrote:
>
>> When running git describe --dirty the index should be refreshed.  Previously
>> the cached index would cause describe to think that the index was dirty when,
>> in reality, it was just stale.
>>
>> The issue was exposed by python setuptools which hardlinks files into another
>> directory when building a distribution.
>
> Overall, looks good to me. A few minor nits, though:
>
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index 66fc291..792af76 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -24,6 +24,7 @@ static int longformat;
>>  static int abbrev = -1; /* unspecified */
>>  static int max_candidates = 10;
>>  static struct hash_table names;
>> +static struct lock_file index_lock; /* real index */
>
> This line was presumably copied straight from builtin/commit.c. You can
> drop the "real index" comment here. Commit may deal with multiple
> indices, which is what this comment was clarifying, but here it doesn't
> make any sense.
>
>>  static int always;
>> @@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one)
>>  int cmd_describe(int argc, const char **argv, const char *prefix)
>>  {
>>       int contains = 0;
>> +     int fd;
>
> If a variable is only going to be used for one deep conditional, IMHO
> it's nice to declare it inside the conditional block, so readers of the
> code don't have to wonder under what conditions fd is valid.
>
>> +             if (dirty) {
>> +                     read_cache();
>> +                     refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
>> +                     fd = hold_locked_index(&index_lock, 0);
>> +                     if (0 <= fd)
>> +                             update_index_if_able(&the_index, &index_lock);
>
> A few questions about this read_cache call:
>
>  1. Should this actually be:
>
>          if (read_cache() < 0)
>                  die("unable to read cache");
>
>     ? I notice that cmd_status also does not check the error code. But
>     it seems like if we fail to read, we would then potentially write
>     out a bogus index. Probably unlikely, as failure to read probably
>     implies failure to write.

It definitely seems like writing out a bogus index would be bad, but even if both the read *and* the write fail we would still be potentially mislabeling it as "dirty" if we failed to it in the first place. It seems like, since they explicitly requested --dirty, we ought to give up here since we can't accurately respond.

>  2. Should the read and refresh happen while we hold the lock?
>     Otherwise our read-modify-update is not atomic, and we risk
>     overwriting another index writer. Again, cmd_status suffers from
>     the same problem, so this is not something you are introducing.

Yeah it definitely is a race condition as far as I can tell. Should this be changed in cmd_status (builtin/commit.c:1227-1232) as well?

-- Allan

← back to recent threads