GIT-VERSION-GEN gives "-dirty" when file metadata changed

GIT-VERSION-GEN gives "-dirty" when file metadata changed

[Christian Jaeger]

Hello,

Today I've created custom Debian packages from Git for the first time (yes I know there are Debian packages already, I'm doing it so that I can patch Git and still have the convenience of a package system), using the 1.6.0.rc2 checkout, and using my normal procedure to build debian source packages (running "dpkg-buildpackage -uc -us -b -rfakeroot" as non-root user). The resulting binary reported for --version the string "1.6.0.rc2-dirty"; I wondered why, since I didn't have uncommitted changes neither in the working dir nor in the index. I found that the GIT-VERSION-GEN script would check for a clean working directory by checking that "git diff-index --name-only HEAD --" does not report any files, and since this is now running under the control of the fakeroot process, all files had owner 
 and group 0, whereas in reality (when I made the checkout) they had a non-root uid/gid. This made diff-index report all files, and hence give the "-dirty" version.

I'll followup this mail with two variants of a patch which runs "git update-index --refresh" before that check, which solves the issue. Patch A just does it always, patch B does it only if the metadata check failed; I've created the latter with the idea in mind that update-index might be too costly in some situation (here it's fast but I don't know about people without much RAM).

Perhaps not many people are building Git with the help of fakeroot, but I don't see why the patch would hurt either, and it seems to me like it's implementing the correct behaviour (metadata changes could also happen should anyone or some build process move or copy the files to another place before building, or similar). I don't know whether the Debian Git package maintainer had another solution, but maybe his packages are simply being built as root without the help of "fakeroot" (cc to him for information).

Christian.

[PATCH A] GIT-VERSION-GEN: refresh the index before judging a working dir to be dirty

[Christian Jaeger]

When building under the control of the "fakeroot" tool [*], as is the
case when building a Debian package using "dpkg-buildpackage
-rfakeroot", GIT-VERSION-GEN appended "-dirty" to the version number;
this happens because "git diff-index --name-only HEAD --" would report
all files as changed if they have a non-root owner/group, since they
appear as owned by root under fakeroot, leading to non-empty
output. Refreshing the index first makes the decision based on content
changes only.

[*] http://fakeroot.alioth.debian.org/

Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>
---
 GIT-VERSION-GEN |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index cb7cd4b..e6ff486 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -16,6 +16,7 @@ elif test -d .git -o -f .git &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
+		git update-index --refresh
 		test -z "$(git diff-index --name-only HEAD --)" ||
 		VN="$VN-dirty" ;;
 	esac
-- 
1.6.0.rc2.1.g7e734

[PATCH B] GIT-VERSION-GEN: refresh the index before judging a working dir to be dirty

[Christian Jaeger]

When building under the control of the "fakeroot" tool [*], as is the
case when building a Debian package using "dpkg-buildpackage
-rfakeroot", GIT-VERSION-GEN appended "-dirty" to the version number;
this happens because "git diff-index --name-only HEAD --" would report
all files as changed if they have a non-root owner/group, since they
appear as owned by root under fakeroot, leading to non-empty
output. Refreshing the index first makes the decision based on content
changes only.

[*] http://fakeroot.alioth.debian.org/

Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>
---
 GIT-VERSION-GEN |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index cb7cd4b..fb3e2d8 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -17,6 +17,12 @@ elif test -d .git -o -f .git &&
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
 		test -z "$(git diff-index --name-only HEAD --)" ||
+		{
+			# some metadata of files has changed; what
+			# about the contents?
+			git update-index --refresh
+			test -z "$(git diff-index --name-only HEAD --)"
+		} ||
 		VN="$VN-dirty" ;;
 	esac
 then
-- 
1.6.0.rc2.1.g7e734

Re: GIT-VERSION-GEN gives "-dirty" when file metadata changed

[Junio C Hamano]

Christian Jaeger <christian@jaeger.mine.nu> writes:

> Today I've created custom Debian packages from Git for the first time (yes I know there are Debian packages already, I'm doing it so that I can patch Git and still have the convenience of a package system),

I personally think that _you_ are responsible for doing the refresh
yourself after becoming root, if you checkout as yourself and then build
as root (or use fakeroot to build as if it is built as root).

By the way "man fakeroot" says...

       -u, --unknown-is-real
              Use the real ownership of files previously unknown  to  fakeroot
              instead of pretending they are owned by root:root.


which sounds like a sensible thing to do (I would even imagine that would
be a sensible default for fakeroot in general), and I would imagine that
would help.

Not that an extra update-index --refresh would be a huge performance hit,
but I hesitate to take a patch that adds something that should
conceptually be unnecessary.

Re: GIT-VERSION-GEN gives "-dirty" when file metadata changed

[Christian Jaeger]

Junio C Hamano wrote:
> Christian Jaeger <christian@jaeger.mine.nu> writes:
>
>   
>> Today I've created custom Debian packages from Git for the first time (yes I know there are Debian packages already, I'm doing it so that I can patch Git and still have the convenience of a package system),
>>     
>
> I personally think that _you_ are responsible for doing the refresh
> yourself after becoming root, if you checkout as yourself and then build
> as root (or use fakeroot to build as if it is built as root).
>
> By the way "man fakeroot" says...
>
>        -u, --unknown-is-real
>               Use the real ownership of files previously unknown  to  fakeroot
>               instead of pretending they are owned by root:root.
>
>
> which sounds like a sensible thing to do (I would even imagine that would
> be a sensible default for fakeroot in general), and I would imagine that
> would help.
>   

That's true, running "dpkg-buildpackage -uc -us -b -r'fakeroot -u'" 
makes the dirty bit go away.

Although my guess is that most users who haven't read this thread will 
run into the same issue until they understand the reason after some half 
or full hour of debugging or so.

Also I don't see why I should keep in mind to run the refresh 
explicitely if any changes happened (are there any users who are using 
Git to report metadata changes to them (occasionally) which aren't 
changes that would be stored in Git when running commit?).

> Not that an extra update-index --refresh would be a huge performance hit,
> but I hesitate to take a patch that adds something that should
> conceptually be unnecessary.
>   

Isn't conceptually of interest whether the *contents* of the files have 
changed (or a metadata piece that matters to Git)? As mentioned, even 
just moving the sources to another partition using "mv" after checkout 
but before running "make" will give a binary that is "dirty", and the 
user might be confused and led into wrong conclusions or needless 
investigations.

I realize that also some git porcellain does not fall back to checking 
the contents, for example the current gitk will report the working dir 
as having "local uncommitted changes" (which in fact did confuse me when 
it happened to me, IIRC because of "mv"-ing a checkout, and left a 
feeling of slight brokenness). Still at the more relevant places like 
"git commit" there will of course be the content check.

I personally think it would be cleaner to always only report changes if 
really changes which can be stored in Git have happened. Not only in 
GIT-VERSION-GEN but also in gitk and maybe some other places. Isn't the 
metadata checking only used as a performance optimization? It would be 
sensible to report changes if metadata has changed that is actually 
being stored in Git, i.e. the exec bit, of course (and then no content 
check would be necessary).

Christian.