{"thread":{"id":"14877","subject":"GIT-VERSION-GEN gives \"-dirty\" when file metadata changed","startedAt":"2008-08-07T15:16:00Z","lastAt":"2008-08-08T08:55:50Z","messageCount":5,"participants":["Christian Jaeger","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"86434","messageId":"14341e3a48ec86021f933361af9b02b542cc7c04.1218137290.git.christian@jaeger.mine.nu","threadId":"14877","inReplyTo":"sjj6zt28jy9qy7y8@jaeger.mine.nu","subject":"[PATCH A] GIT-VERSION-GEN: refresh the index before judging a working dir to be dirty","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-07T15:16:00Z","receivedAt":"2008-08-07T15:16:00Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"When building under the control of the \"fakeroot\" tool [*], as is the\ncase when building a Debian package using \"dpkg-buildpackage\n-rfakeroot\", GIT-VERSION-GEN appended \"-dirty\" to the version number;\nthis happens because \"git diff-index --name-only HEAD --\" would report\nall files as changed if they have a non-root owner/group, since they\nappear as owned by root under fakeroot, leading to non-empty\noutput. Refreshing the index first makes the decision based on content\nchanges only.\n\n[*] http://fakeroot.alioth.debian.org/\n\nSigned-off-by: Christian Jaeger \n---\n GIT-VERSION-GEN | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex cb7cd4b..e6ff486 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -16,6 +16,7 @@ elif test -d .git -o -f .git &&\n \tcase \"$VN\" in\n \t*$LF*) (exit 1) ;;\n \tv[0-9]*)\n+\t\tgit update-index --refresh\n \t\ttest -z \"$(git diff-index --name-only HEAD --)\" ||\n \t\tVN=\"$VN-dirty\" ;;\n \tesac\n-- \n1.6.0.rc2.1.g7e734\n"},{"id":"86435","messageId":"a921f9287bf93b1f4de21968ee02a06fe69198a8.1218137290.git.christian@jaeger.mine.nu","threadId":"14877","inReplyTo":"sjj6zt28jy9qy7y8@jaeger.mine.nu","subject":"[PATCH B] GIT-VERSION-GEN: refresh the index before judging a working dir to be dirty","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-07T17:59:25Z","receivedAt":"2008-08-07T17:59:25Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"When building under the control of the \"fakeroot\" tool [*], as is the\ncase when building a Debian package using \"dpkg-buildpackage\n-rfakeroot\", GIT-VERSION-GEN appended \"-dirty\" to the version number;\nthis happens because \"git diff-index --name-only HEAD --\" would report\nall files as changed if they have a non-root owner/group, since they\nappear as owned by root under fakeroot, leading to non-empty\noutput. Refreshing the index first makes the decision based on content\nchanges only.\n\n[*] http://fakeroot.alioth.debian.org/\n\nSigned-off-by: Christian Jaeger \n---\n GIT-VERSION-GEN | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex cb7cd4b..fb3e2d8 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -17,6 +17,12 @@ elif test -d .git -o -f .git &&\n \t*$LF*) (exit 1) ;;\n \tv[0-9]*)\n \t\ttest -z \"$(git diff-index --name-only HEAD --)\" ||\n+\t\t{\n+\t\t\t# some metadata of files has changed; what\n+\t\t\t# about the contents?\n+\t\t\tgit update-index --refresh\n+\t\t\ttest -z \"$(git diff-index --name-only HEAD --)\"\n+\t\t} ||\n \t\tVN=\"$VN-dirty\" ;;\n \tesac\n then\n-- \n1.6.0.rc2.1.g7e734\n"},{"id":"86433","messageId":"sjj6zt28jy9qy7y8@jaeger.mine.nu","threadId":"14877","inReplyTo":null,"subject":"GIT-VERSION-GEN gives \"-dirty\" when file metadata changed","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-07T19:35:17Z","receivedAt":"2008-08-07T19:35:17Z","isPatch":false,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"Hello,\n\nToday 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 \n 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.\n\nI'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).\n\nPerhaps 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).\n\nChristian.\n"},{"id":"86441","messageId":"7vd4kkijjd.fsf@gitster.siamese.dyndns.org","threadId":"14877","inReplyTo":"sjj6zt28jy9qy7y8@jaeger.mine.nu","subject":"Re: GIT-VERSION-GEN gives \"-dirty\" when file metadata changed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-07T21:48:06Z","receivedAt":"2008-08-07T21:48:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Jaeger writes:\n\n> 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),\n\nI personally think that _you_ are responsible for doing the refresh\nyourself after becoming root, if you checkout as yourself and then build\nas root (or use fakeroot to build as if it is built as root).\n\nBy the way \"man fakeroot\" says...\n\n -u, --unknown-is-real\n Use the real ownership of files previously unknown to fakeroot\n instead of pretending they are owned by root:root.\n\n\nwhich sounds like a sensible thing to do (I would even imagine that would\nbe a sensible default for fakeroot in general), and I would imagine that\nwould help.\n\nNot that an extra update-index --refresh would be a huge performance hit,\nbut I hesitate to take a patch that adds something that should\nconceptually be unnecessary.\n"},{"id":"86471","messageId":"489C0A16.4040403@jaeger.mine.nu","threadId":"14877","inReplyTo":"7vd4kkijjd.fsf@gitster.siamese.dyndns.org","subject":"Re: GIT-VERSION-GEN gives \"-dirty\" when file metadata changed","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-08T08:55:50Z","receivedAt":"2008-08-08T08:55:50Z","isPatch":false,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"Junio C Hamano wrote:\n> Christian Jaeger writes:\n>\n> \n>> 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),\n>> \n>\n> I personally think that _you_ are responsible for doing the refresh\n> yourself after becoming root, if you checkout as yourself and then build\n> as root (or use fakeroot to build as if it is built as root).\n>\n> By the way \"man fakeroot\" says...\n>\n> -u, --unknown-is-real\n> Use the real ownership of files previously unknown to fakeroot\n> instead of pretending they are owned by root:root.\n>\n>\n> which sounds like a sensible thing to do (I would even imagine that would\n> be a sensible default for fakeroot in general), and I would imagine that\n> would help.\n> \n\nThat's true, running \"dpkg-buildpackage -uc -us -b -r'fakeroot -u'\" \nmakes the dirty bit go away.\n\nAlthough my guess is that most users who haven't read this thread will \nrun into the same issue until they understand the reason after some half \nor full hour of debugging or so.\n\nAlso I don't see why I should keep in mind to run the refresh \nexplicitely if any changes happened (are there any users who are using \nGit to report metadata changes to them (occasionally) which aren't \nchanges that would be stored in Git when running commit?).\n\n> Not that an extra update-index --refresh would be a huge performance hit,\n> but I hesitate to take a patch that adds something that should\n> conceptually be unnecessary.\n> \n\nIsn't conceptually of interest whether the *contents* of the files have \nchanged (or a metadata piece that matters to Git)? As mentioned, even \njust moving the sources to another partition using \"mv\" after checkout \nbut before running \"make\" will give a binary that is \"dirty\", and the \nuser might be confused and led into wrong conclusions or needless \ninvestigations.\n\nI realize that also some git porcellain does not fall back to checking \nthe contents, for example the current gitk will report the working dir \nas having \"local uncommitted changes\" (which in fact did confuse me when \nit happened to me, IIRC because of \"mv\"-ing a checkout, and left a \nfeeling of slight brokenness). Still at the more relevant places like \n\"git commit\" there will of course be the content check.\n\nI personally think it would be cleaner to always only report changes if \nreally changes which can be stored in Git have happened. Not only in \nGIT-VERSION-GEN but also in gitk and maybe some other places. Isn't the \nmetadata checking only used as a performance optimization? It would be \nsensible to report changes if metadata has changed that is actually \nbeing stored in Git, i.e. the exec bit, of course (and then no content \ncheck would be necessary).\n\nChristian.\n"}]}