2017-09-26 19:46:40

by Marc Herbert

[permalink] [raw]
Subject: Re: BUG in git diff-index

On 31/03/16 13:39, Junio C Hamano wrote:
> Andy Lowry <[email protected]> writes:
>
>> So I think now that the script should do "update-index --refresh"
>> followed by "diff-index --quiet HEAD". Sound correct?
>
> Yes. That has always been one of the kosher ways for any script to
> make sure that the files in the working tree that are tracked have
> not been modified relative to HEAD (assuming that the index matches
> HEAD).

Too bad kernel/scripts/setlocalversion didn't get the memo:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cdf2bc632ebc9ef51
> scripts/setlocalversion on write-protected source tree (2013)
> I don't see how removing "git update-index" could do any harm.

This causes a spurious "-dirty" suffix when building from a directory copy
(as Mike learned the hard way)

----

PS: I used NNTP and http://dir.gmane.org/gmane.comp.version-control.git
to quickly find this old thread (what could we do without NNTP?). Then
I googled for a web archive of this thread and Google could only find
this one: http://git.661346.n2.nabble.com/BUG-in-git-diff-index-tt7652105.html#none
Is there a robots.txt to block indexing on
https://public-inbox.org/git/[email protected] ?


2017-09-26 20:19:23

by Eric Wong

[permalink] [raw]
Subject: Re: BUG in git diff-index

Marc Herbert <[email protected]> wrote:
> PS: I used NNTP and http://dir.gmane.org/gmane.comp.version-control.git
> to quickly find this old thread (what could we do without NNTP?). Then
> I googled for a web archive of this thread and Google could only find
> this one: http://git.661346.n2.nabble.com/BUG-in-git-diff-index-tt7652105.html#none
> Is there a robots.txt to block indexing on
> https://public-inbox.org/git/[email protected] ?

There's no blocks on public-inbox.org and I'm completely against
any sort of blocking/throttling. Maybe there's too many pages
to index? Or the Message-IDs in URLs are too ugly/scary? Not
sure what to do about that...

Anyways, I just put up a robots.txt with Crawl-Delay: 1, since I
seem to recall crawlers use a more conservative delay by default:

==> https://public-inbox.org/robots.txt <==
User-Agent: *
Crawl-Delay: 1


I don't know much about SEO other than keeping a site up and
responsive; so perhaps there's more to be done about getting
things indexed...

2017-09-27 21:06:30

by Marc Herbert

[permalink] [raw]
Subject: Wrong -dirty suffix set by setlocalversion (was: BUG in git diff-index)

+ linux-kbuild list which is not in the output of:
./scripts/get_maintainer.pl -f scripts/setlocalversion
... but seems relevant anyway.

On 31/03/16 13:39, Junio C Hamano wrote:
> Andy Lowry <[email protected]> writes:
>
>> So I think now that the script should do "update-index --refresh"
>> followed by "diff-index --quiet HEAD". Sound correct?
>
> Yes. That has always been one of the kosher ways for any script to
> make sure that the files in the working tree that are tracked have
> not been modified relative to HEAD (assuming that the index matches
> HEAD).

Too bad kernel/scripts/setlocalversion didn't get the memo:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cdf2bc632ebc9ef51
> scripts/setlocalversion on write-protected source tree (2013)
> I don't see how removing "git update-index" could do any harm.

This causes a spurious "-dirty" suffix when building from a directory copy
(as Mike learned the hard way)

[...]

https://public-inbox.org/git/[email protected]

2018-05-25 02:47:30

by Mike Mason

[permalink] [raw]
Subject: Re: Wrong -dirty suffix set by setlocalversion (was: BUG in git diff-index)

How about something like this? It ignores attributes that should have no
bearing on whether the kernel is considered dirty. Copied trees with no other
changes would no longer be marked with -dirty. Plus it works on read-only
media since no index updating is required.

Would this also be considered kosher, at least for the purposes of
setlocalversion?

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..9da4c5e83285 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -73,8 +73,10 @@ scm_version()
printf -- '-svn%s' "`git svn find-rev $head`"
fi

- # Check for uncommitted changes
- if git diff-index --name-only HEAD | grep -qv "^scripts/package"; then
+ # Check for uncommitted changes. Only check mtime and size.
+ # Ignore insequential ctime, uid, gid and inode differences.
+ if git -c "core.checkstat=minimal" diff-index --name-only HEAD | \
+ grep -qv "^scripts/package"; then
printf '%s' -dirty
fi


2018-05-25 03:55:37

by Marc Herbert

[permalink] [raw]
Subject: Re: Wrong -dirty suffix set by setlocalversion (was: BUG in git diff-index)

On 24/05/2018 16:03, Mike Mason wrote:

> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 71f39410691b..9da4c5e83285 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -73,8 +73,10 @@ scm_version()
> printf -- '-svn%s' "`git svn find-rev $head`"
> fi
>
> - # Check for uncommitted changes
> - if git diff-index --name-only HEAD | grep -qv "^scripts/package"; then
> + # Check for uncommitted changes. Only check mtime and size.
> + # Ignore insequential ctime, uid, gid and inode differences.
> + if git -c "core.checkstat=minimal" diff-index --name-only HEAD | \
> + grep -qv "^scripts/package"; then
> printf '%s' -dirty
> fi

FWIW:

Reported-by: [email protected]
Reviewed-by: [email protected] (assuming a future and decent commit message)
Tested-by: [email protected]


So the real use case is making a copy of a whole tree before building.
Typical in automated builds, old example:
https://groups.google.com/a/chromium.org/d/msg/chromium-os-dev/zxOa0OLWFkw/N_Sb7EZOBwAJ

Here's a more complex but faster and more transparent way to test Mike's fix
than copying an entire tree:

# Make sure you start from a clean state
git describe --dirty # must not -dirty

make prepare

# Simulate a copy of the tree but with just one file
rsync --perms --times README README.mtime_backup
rm README
rsync --perms --times README.mtime_backup README
stat README README.mtime_backup

# Demo the BUG fixed by Mike
./scripts/setlocalversion # -dirty BUG! because spurious inode ctime difference
git diff-index HEAD
git describe --dirty # not -dirty
./scripts/setlocalversion # not -dirty any more cause describe refreshed index

# Make sure mtime still causes -dirty with AND without Mike's fix
touch README
./scripts/setlocalversion # -dirty