Unusual behavior from git describe

Unusual behavior from git describe

[Allan Caffee]

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

Re: Unusual behavior from git describe

[Sverre Rabbelier]

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?

-- 
Cheers,

Sverre Rabbelier

Re: Unusual behavior from git describe

[Allan Caffee]

Hey,

On Fri, Jul 29, 2011 at 5:55 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> 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

Re: Unusual behavior from git describe

[Sverre Rabbelier]

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

Re: Unusual behavior from git describe

[Allan Caffee]

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"));

Re: Unusual behavior from git describe

[Jeff King]

On Sat, Jul 30, 2011 at 12:23:30PM -0400, Allan Caffee wrote:

> 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

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

[Allan Caffee]

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

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

[Jeff King]

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.

  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

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

[Junio C Hamano]

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);

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

[Allan Caffee]

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:
> 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