Revert "scripts/setlocalversion: git: Make -dirty check more robust"

This reverts commit 6147b1cf19651c7de297e69108b141fb30aa2349.

The reverted patch results in attempted write access to the source repository, even if that repository is mounted read-only.

Output from "strace git status -uno --porcelain":

getcwd("/tmp/linux-test", 129) = 16
open("/tmp/linux-test/.git/index.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) =
-1 EROFS (Read-only file system)

While git appears to be able to handle this situation, a monitored build environment (such as the one used for Chrome OS kernel builds) may detect it and bail out with an access violation error. On top of that, the attempted write access suggests that git _will_ write to the file even if a build output directory is specified. Users may have the reasonable expectation that the source repository remains untouched in that situation.

Quoted 11 lines · Guenter Roeck

This reverts commit 6147b1cf19651c7de297e69108b141fb30aa2349.

The reverted patch results in attempted write access to the source repository, even if that repository is mounted read-only.

Output from "strace git status -uno --porcelain":

getcwd("/tmp/linux-test", 129) = 16
open("/tmp/linux-test/.git/index.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) =
-1 EROFS (Read-only file system)

Quoted 6 lines

While git appears to be able to handle this situation, a monitored build environment (such as the one used for Chrome OS kernel builds) may detect it and bail out with an access violation error. On top of that, the attempted write access suggests that git _will_ write to the file even if a build output directory is specified. Users may have the reasonable expectation that the source repository remains untouched in that situation.

Hmm, so in summary: According to 6147b1cf1965 ("scripts/setlocalversion: git: Make -dirty check more robust", 2018-08-28), one scenario requires the index to be refreshed to get a correct "dirty" or "not dirty" status. But according to your commit here, another scenario requires the kernel build system to not even attempt to update the git index, and doesn't care / aren't impacted by the cases where the index needs to be refreshed.

Perhaps both scenarios could be satisfied by having scripts/setlocalversion first check if .git has write permissions, and acting accordingly. Looking into history, this actually used to be done, but cdf2bc632ebc ("scripts/setlocalversion on write-protected source tree", 2013-06-14) removed the updating of the index.

However, I admit I don't understand the justification in that commit from 2013. I'm no NFS expert, but perhaps the real problem there is an incorrectly configured NFS setup (uid/gid mismatch between NFS client/server, or permissions mismatch between mount options and NFS server?). Christian Kujau: can you speak to that?

Well, we could also make our check $(touch .git/some-file-here 2>/dev/null && ...) instead of $(test -w .git) to handle misconfigured NFS setups. But not sure if that has its own problems.

Quoted 5 lines

Perhaps both scenarios could be satisfied by having scripts/setlocalversion first check if .git has write permissions, and acting accordingly. Looking into history, this actually used to be done, but cdf2bc632ebc ("scripts/setlocalversion on write-protected source tree", 2013-06-14) removed the updating of the index.

A "writeable" check (e.g., [ -w . ]) would be sufficient for our case. But I'm not so sure about that older NFS report, and I'm also not sure that we should be writing to the source tree at all in this case. Maybe we can also check whether there's a build output directory specified?

Quoted 9 lines

However, I admit I don't understand the justification in that commit from 2013. I'm no NFS expert, but perhaps the real problem there is an incorrectly configured NFS setup (uid/gid mismatch between NFS client/server, or permissions mismatch between mount options and NFS server?). Christian Kujau: can you speak to that?

Well, we could also make our check $(touch .git/some-file-here 2>/dev/null && ...) instead of $(test -w .git) to handle misconfigured NFS setups. But not sure if that has its own problems.

Trying to 'touch' the source tree will also break us. No matter whether you redirect stderr, our sandbox will still notice the build is doing something fishy and complain.

Quoted 10 lines · Brian Norris

Quoted 5 lines

Perhaps both scenarios could be satisfied by having scripts/setlocalversion first check if .git has write permissions, and acting accordingly. Looking into history, this actually used to be done, but cdf2bc632ebc ("scripts/setlocalversion on write-protected source tree", 2013-06-14) removed the updating of the index.

A "writeable" check (e.g., [ -w . ]) would be sufficient for our case. But I'm not so sure about that older NFS report, and I'm also not sure that we should be writing to the source tree at all in this case. Maybe we can also check whether there's a build output directory specified?

FWIW, the issue I reported back in 2013[0] was not an ill-configured NFS export, but a read-only NFS export (and then a read-write exported NFS export, but the user compiling the kernel did not have write permission) and so "test -w .git" did not help in determining if the source tree can actually written to. And depending on the user's shell[1], this may or may not still be the case.

So I'm all for the $(touch .git/some-file-here) test to decide if the kernel has to be modified during build.

Christian.

[0] https://lkml.org/lkml/2013/6/14/574
[1] https://manpages.debian.org/unstable/dash/dash.1.en.html

Quoted 13 lines

Quoted 9 lines

However, I admit I don't understand the justification in that commit from 2013. I'm no NFS expert, but perhaps the real problem there is an incorrectly configured NFS setup (uid/gid mismatch between NFS client/server, or permissions mismatch between mount options and NFS server?). Christian Kujau: can you speak to that?

Well, we could also make our check $(touch .git/some-file-here 2>/dev/null && ...) instead of $(test -w .git) to handle misconfigured NFS setups. But not sure if that has its own problems.

Trying to 'touch' the source tree will also break us. No matter whether you redirect stderr, our sandbox will still notice the build is doing something fishy and complain.

FWIW, the issue I reported back in 2013[0] was not an ill-configured NFS export, but a read-only NFS export (and then a read-write exported NFS export, but the user compiling the kernel did not have write permission) and so "test -w .git" did not help in determining if the source tree can actually written to. And depending on the user's shell[1], this may or may not still be the case.

So I'm all for the $(touch .git/some-file-here) test to decide if the kernel has to be modified during build.

Christian.

[0] https://lkml.org/lkml/2013/6/14/574
[1] https://manpages.debian.org/unstable/dash/dash.1.en.html

On a different tangent: how about the --no-optional-locks (see git(1))? Will this get you your "up-to-date" result without writing to the .git directory? I've only read the documentation, but not tested it.

Quoted 4 lines · Brian Norris

On a different tangent: how about the --no-optional-locks (see git(1))? Will this get you your "up-to-date" result without writing to the .git directory? I've only read the documentation, but not tested it.

Unfortunately, --no-optional-locks is new as of git 2.14. Dunno how new of a git we expect people to use.

Quoted 5 lines · Brian Norris

Quoted 4 lines · Brian Norris

On a different tangent: how about the --no-optional-locks (see git(1))? Will this get you your "up-to-date" result without writing to the .git directory? I've only read the documentation, but not tested it.

This option definitely seems to be what we want, good find.

Quoted 2 lines

Unfortunately, --no-optional-locks is new as of git 2.14. Dunno how new of a git we expect people to use.

Hmm, I'm not sure who can speak to this.

Though if it's too recent, then based on earlier discussion, it sounds like something like this (hack) might work best:

[ -w .git ] &&

          touch .git/some-file-here 2>/dev/null &&
          git update-index --refresh --unmerged >/dev/null

if git diff-index --name-only HEAD | ...

I do not think it is a good idea to create a random file in the .git directory under any circumstance, and much less so if an output directory was specified, no matter if the path is read-only or not. I also still think that it is a bad idea to touch the source tree if an output directory was specified. It defeats the purpose of specifying an output directory.

Ubuntu 16.04 ships with git version 2.7.4.

Quoted 5 lines · Guenter Roeck

I do not think it is a good idea to create a random file in the .git directory under any circumstance, and much less so if an output directory was specified, no matter if the path is read-only or not. I also still think that it is a bad idea to touch the source tree if an output directory was specified. It defeats the purpose of specifying an output directory.

I was thinking of touching a pre-existing file like .git/config or .git/description, which I was hoping would be harmless. But sounds like that's still not desired?

Okay, I guess one approach is to only refresh the index if $objtree == $srctree, by passing some flag to scripts/setlocalversion from scripts/package/Makefile. Is that what you're thinking? Feels a little strange, but it seems it'd satisfy everyone.

Quoted 1 line

Ubuntu 16.04 ships with git version 2.7.4.

Okay. I guess --no-optional-locks is a no-go then.

From reading the thread it sounds like Guenter was not even super happy with that based on the principal that you wouldn't expect a kernel build to be doing write operations in your .git directory even if $objtree == $srctree

In theory you could wrap it. If passing git with "--no-optional-locks" doesn't work you could fall back to the old code? That would mean only people with newer git would get your new feature and everyone else would stick with the pre-existing behavior.

It does seem like any things like this should be done atop Guenter's revert. AKA: revert first to get things working the way that they were and then start talking about how to make it better.

Quoted 4 lines · Genki Sky

Quoted 1 line · Guenter Roeck

Ubuntu 16.04 ships with git version 2.7.4.

Okay. I guess --no-optional-locks is a no-go then.

In theory you could wrap it. If passing git with "--no-optional-locks" doesn't work you could fall back to the old code? That would mean only people with newer git would get your new feature and everyone else would stick with the pre-existing behavior.

Quoted 6 lines · Doug Anderson

Quoted 4 lines · Genki Sky

Quoted 1 line · Guenter Roeck

Ubuntu 16.04 ships with git version 2.7.4.

Okay. I guess --no-optional-locks is a no-go then.

Quoted 4 lines

In theory you could wrap it. If passing git with "--no-optional-locks" doesn't work you could fall back to the old code? That would mean only people with newer git would get your new feature and everyone else would stick with the pre-existing behavior.

+1, that's what I was going to suggest. Presumably older git would give non-zero exit status for unknown flags, and we take that as signal to try to the old way?

Quoted 7 lines · Doug Anderson

Quoted 6 lines · Genki Sky

Quoted 4 lines · Guenter Roeck

Quoted 1 line

Ubuntu 16.04 ships with git version 2.7.4.

Okay. I guess --no-optional-locks is a no-go then.

Quoted 8 lines

Quoted 4 lines

In theory you could wrap it. If passing git with "--no-optional-locks" doesn't work you could fall back to the old code? That would mean only people with newer git would get your new feature and everyone else would stick with the pre-existing behavior.

+1, that's what I was going to suggest. Presumably older git would give non-zero exit status for unknown flags, and we take that as signal to try to the old way?

I also like this idea!

I will pick-up this revert patch soon.

Brian,
Could you please send a patch on top of that?

git-diff-index does not refresh the index for you, so using it for a "-dirty" check can give misleading results. Commit 6147b1cf19651 ("scripts/setlocalversion: git: Make -dirty check more robust") tried to fix this by switching to git-status, but it overlooked the fact that git-status also writes to the .git directory of the source tree, which is definitely not kosher for an out-of-tree (O=) build. That is getting reverted.

Fortunately, git-status now supports avoiding writing to the index via the --no-optional-locks flag, as of git 2.14. It still calculates an up-to-date index, but it avoids writing it out to the .git directory.

So, let's retry the solution from commit 6147b1cf19651 using this new flag first, and if it fails, we assume this is an older version of git and just use the old git-diff-index method.

Cc Genki Sky <[email protected]>

Cc Christian Kujau <[email protected]>

Cc Guenter Roeck <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---

Quoted 16 lines · Masahiro Yamada

Quoted 7 lines · Doug Anderson

Quoted 6 lines · Genki Sky

Quoted 4 lines · Guenter Roeck

Quoted 1 line

Ubuntu 16.04 ships with git version 2.7.4.

Okay. I guess --no-optional-locks is a no-go then.

Quoted 8 lines

Quoted 4 lines

In theory you could wrap it. If passing git with "--no-optional-locks" doesn't work you could fall back to the old code? That would mean only people with newer git would get your new feature and everyone else would stick with the pre-existing behavior.

+1, that's what I was going to suggest. Presumably older git would give non-zero exit status for unknown flags, and we take that as signal to try to the old way?

Quoted 7 lines

I also like this idea!

I will pick-up this revert patch soon.

Brian,
Could you please send a patch on top of that?

Done.

It's not supremely beautiful, but I believe it works. I tested with new git, and with a faked git wrapper that rejects --no-optional-locks, dumps an error to stderr, and returns a non-zero exit code. I don't happen to have an older copy of git lying around at the moment...

 scripts/setlocalversion | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

scripts/setlocalversion +13 -2

index 71f39410691b..eab1f90de50d 100755
@@ -73,8 +73,19 @@ 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.
+		# First, with git-status, but --no-optional-locks is only
+		# supported in git >= 2.14, so fall back to git-diff-index if
+		# it fails. Note that git-diff-index does not refresh the
+		# index, so it may give misleading results. See
+		# git-update-index(1), git-diff-index(1), and git-status(1).
+		local git_status
+		git_status="$(git --no-optional-locks status -uno --porcelain 2>/dev/null)"
+		if [ $? -eq 0 ]; then
+			if echo "$git_status" | grep -qv '^.. scripts/package'; then
+				printf '%s' -dirty
+			fi
+		elif git diff-index --name-only HEAD | grep -qv "^scripts/package"; then
 			printf '%s' -dirty
 		fi
 

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

scripts/setlocalversion +0 -2

index 71f39410691b..eab1f90de50d 100755
@@ -73,8 +73,19 @@ 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

Quoted 10 lines · Brian Norris

scripts/setlocalversion +0 -2

index 71f39410691b..eab1f90de50d 100755
@@ -73,8 +73,19 @@ 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

scripts/setlocalversion scm_version() · line 85

Quoted 10 lines

scripts/setlocalversion +10 -0

+		# Check for uncommitted changes.
+		# First, with git-status, but --no-optional-locks is only
+		# supported in git >= 2.14, so fall back to git-diff-index if
+		# it fails. Note that git-diff-index does not refresh the
+		# index, so it may give misleading results. See
+		# git-update-index(1), git-diff-index(1), and git-status(1).
+		local git_status
+		git_status="$(git --no-optional-locks status -uno --porcelain 2>/dev/null)"
+		if [ $? -eq 0 ]; then
+			if echo "$git_status" | grep -qv '^.. scripts/package'; then

Shouldn't this be:

if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then

I.e., use printf not echo? Because of echo introducing a newline.

Quoted 7 lines · Genki Sky

scripts/setlocalversion scm_version() · line 85

Quoted 3 lines · Brian Norris

scripts/setlocalversion +3 -0

+		git_status="$(git --no-optional-locks status -uno --porcelain 2>/dev/null)"
+		if [ $? -eq 0 ]; then
+			if echo "$git_status" | grep -qv '^.. scripts/package'; then

Shouldn't this be:

Quoted 3 lines

if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then

I.e., use printf not echo? Because of echo introducing a newline.

The input to grep should be a text file, thus should end with a newline.

Hi Andreas,

Quoted 6 lines · Andreas Schwab

Quoted 5 lines · Genki Sky

Quoted 3 lines · Brian Norris

scripts/setlocalversion +3 -0

+          git_status="$(git --no-optional-locks status -uno --porcelain 2>/dev/null)"
+          if [ $? -eq 0 ]; then
+                  if echo "$git_status" | grep -qv '^.. scripts/package'; then

Quoted 7 lines

Quoted 5 lines

Shouldn't this be:

if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then

I.e., use printf not echo? Because of echo introducing a newline.

The input to grep should be a text file, thus should end with a newline.

Ah okay, thanks. I guess GNU grep was being lenient. Well then, I think the line at least needs to be changed to:

        if [ -n "$git_status" ] && echo "$git_status" | grep -qv '^.. scripts/package'; then

I'm just trying to say that in the proposed patch, if git doesn't print anything, the echo adds a newline that wasn't there before. This causes the grep -qv to exit with status 0 (because there's at least one line that doesn't contain '^.. scripts/package'). Meaning it will print dirty.

Quoted 21 lines · Genki Sky

Hi Andreas,

Quoted 6 lines · Andreas Schwab

Quoted 5 lines · Genki Sky

Quoted 3 lines · Brian Norris

scripts/setlocalversion +3 -0

+          git_status="$(git --no-optional-locks status -uno --porcelain 2>/dev/null)"
+          if [ $? -eq 0 ]; then
+                  if echo "$git_status" | grep -qv '^.. scripts/package'; then

Quoted 7 lines

Quoted 5 lines

Shouldn't this be:

if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then

I.e., use printf not echo? Because of echo introducing a newline.

The input to grep should be a text file, thus should end with a newline.

Ah okay, thanks. I guess GNU grep was being lenient. Well then, I think the line at least needs to be changed to:

Quoted 7 lines

        if [ -n "$git_status" ] && echo "$git_status" | grep -qv '^.. scripts/package'; then

I'm just trying to say that in the proposed patch, if git doesn't print anything, the echo adds a newline that wasn't there before. This causes the grep -qv to exit with status 0 (because there's at least one line that doesn't contain '^.. scripts/package'). Meaning it will print dirty.

Piping the output of the git command to grep and using the return status of grep as the test condition within the if block, would be sufficient to determine whether or not '-dirty' should be printed.

Sample run:

% if git --no-optional-locks \
        status -uno --porcelain \
        2>/dev/null |
        grep -qv '^.. scripts/package'

then
printf '%s' -dirty
fi
%

Piping the output of the git command to grep and using the return status of grep as the test condition within the if block, would be sufficient to determine whether or not '-dirty' should be printed.

Sample run:

% if git --no-optional-locks \
        status -uno --porcelain \
        2>/dev/null |
        grep -qv '^.. scripts/package'

then
printf '%s' -dirty
fi

Hi Alexander,

Quoted 4 lines · Alexander Kapshuk

Piping the output of the git command to grep and using the return status of grep as the test condition within the if block, would be sufficient to determine whether or not '-dirty' should be printed.

Quoted 8 lines

Sample run:

% if git --no-optional-locks \
        status -uno --porcelain \
        2>/dev/null |
        grep -qv '^.. scripts/package'

then
printf '%s' -dirty
fi

I don't think this works well for us. We need to check whether --no-optional-locks is available before using the output to determine whether the tree is dirty or not. If it's not available, we have to fall back on diff-index. Let me know if I'm misreading you.

Quoted 17 lines · Genki Sky

Hi Alexander,

Quoted 4 lines · Alexander Kapshuk

Piping the output of the git command to grep and using the return status of grep as the test condition within the if block, would be sufficient to determine whether or not '-dirty' should be printed.

Quoted 8 lines

Sample run:

% if git --no-optional-locks \
        status -uno --porcelain \
        2>/dev/null |
        grep -qv '^.. scripts/package'

then
printf '%s' -dirty
fi

Quoted 4 lines

I don't think this works well for us. We need to check whether --no-optional-locks is available before using the output to determine whether the tree is dirty or not. If it's not available, we have to fall back on diff-index. Let me know if I'm misreading you.

It was I who failed to read the proposed patch in its entirety in the first place. I did not get the full picture as a result. My apologies.

Would something like this work for you?

local git_status
git_status=$(git --no-optional-locks status -uno --porcelain 2>/dev/null) test $? -eq 0 ||

        git_status=$(git diff-index --name-only HEAD)

if printf '%s' "$git_status" | grep -qv scripts/package; then
printf '%s' -dirty
fi

An even simpler approach would be:

{
        git --no-optional-locks status -uno --porcelain 2>/dev/null ||
        git diff-index --name-only HEAD
} | grep -qv scripts/package &&

printf '%s' -dirty

Sample run:
cmd
sh: cmd: command not found

{
        cmd 2>/dev/null ||
        date
} | grep -q 2018 &&

printf '%s' ok
ok

Quoted 12 lines · Alexander Kapshuk

An even simpler approach would be:

{
        git --no-optional-locks status -uno --porcelain 2>/dev/null ||
        git diff-index --name-only HEAD
} | grep -qv scripts/package &&

printf '%s' -dirty

Sample run:
cmd
sh: cmd: command not found

Quoted 6 lines

{
        cmd 2>/dev/null ||
        date
} | grep -q 2018 &&

printf '%s' ok
ok

You lose accuracy here, because now you're skipping any line that contains 'scripts/package', which would include, e.g., paths like

tools/some/other-scripts/package

Maybe if the grep expression were more like this?

grep -qv '^\(.. \)\?scripts/package'

I think it'd be safe enough to ignore paths that start with two characters and a space, like:

xy scripts/package
x/ scripts/package

Brian

Quoted 33 lines · Brian Norris

Quoted 12 lines · Alexander Kapshuk

An even simpler approach would be:

{
        git --no-optional-locks status -uno --porcelain 2>/dev/null ||
        git diff-index --name-only HEAD
} | grep -qv scripts/package &&

printf '%s' -dirty

Sample run:
cmd
sh: cmd: command not found

Quoted 6 lines

{
        cmd 2>/dev/null ||
        date
} | grep -q 2018 &&

printf '%s' ok
ok

You lose accuracy here, because now you're skipping any line that contains 'scripts/package', which would include, e.g., paths like

tools/some/other-scripts/package

Maybe if the grep expression were more like this?

grep -qv '^\(.. \)\?scripts/package'

I think it'd be safe enough to ignore paths that start with two characters and a space, like:

Quoted 4 lines

xy scripts/package
x/ scripts/package

Brian

Thanks for your input.
I've found the following grep command, that uses extended regular expressions, to work as required:

{
echo hello
echo scripts/package
echo '.. scripts/package'
echo tools/some/other-scripts/package
} | grep -Ev '^(.. )?scripts/package'

[Output]
hello
tools/some/other-scripts/package

If the participants of this email exchange consider the proposed implementation as fitting the bill,

{
        git --no-optional-locks status -uno --porcelain 2>/dev/null ||
        git diff-index --name-only HEAD
} | grep -Eqv '^(.. )?scripts/package' &&

printf '%s' -dirty

Was the original committer of the patch proposed here,
https://lkml.org/lkml/2018/11/10/55, going to take it in, and resend it as v2 of the patch, or did you want me to submit the patch instead? I would be happy with either way.

I can submit it. I expect Masahiro-san would prefer a proper v2 patch for review, given how much would change from my v1.

[ ... ]

Cc Genki Sky <[email protected]>

Cc Christian Kujau <[email protected]>

Cc Guenter Roeck <[email protected]>

Suggested-by Alexander Kapshuk <[email protected]>

Signed-off-by Brian Norris <[email protected]>

Quoted 1 line · Brian Norris

+ } | grep -qv '^\(.. \)\?scripts/package'; then

\? is a GNU extension, so if you want to stay portable you need to switch to ERE.

git-diff-index does not refresh the index for you, so using it for a "-dirty" check can give misleading results. Commit 6147b1cf19651 ("scripts/setlocalversion: git: Make -dirty check more robust") tried to fix this by switching to git-status, but it overlooked the fact that git-status also writes to the .git directory of the source tree, which is definitely not kosher for an out-of-tree (O=) build. That is getting reverted.

Fortunately, git-status now supports avoiding writing to the index via the --no-optional-locks flag, as of git 2.14. It still calculates an up-to-date index, but it avoids writing it out to the .git directory.

So, let's retry the solution from commit 6147b1cf19651 using this new flag first, and if it fails, we assume this is an older version of git and just use the old git-diff-index method.

It's hairy to get the 'grep -vq' (inverted matching) correct by stashing the output of git-status (you have to be careful about the difference betwen "empty stdin" and "blank line on stdin"), so just pipe the output directly to grep and use a regex that's good enough for both the git-status and git-diff-index version.

Cc Genki Sky <[email protected]>

Cc Christian Kujau <[email protected]>

Cc Guenter Roeck <[email protected]>

Suggested-by Alexander Kapshuk <[email protected]>

Signed-off-by Brian Norris <[email protected]>

git-diff-index does not refresh the index for you, so using it for a "-dirty" check can give misleading results. Commit 6147b1cf19651 ("scripts/setlocalversion: git: Make -dirty check more robust") tried to fix this by switching to git-status, but it overlooked the fact that git-status also writes to the .git directory of the source tree, which is definitely not kosher for an out-of-tree (O=) build. That is getting reverted.

Fortunately, git-status now supports avoiding writing to the index via the --no-optional-locks flag, as of git 2.14. It still calculates an up-to-date index, but it avoids writing it out to the .git directory.

So, let's retry the solution from commit 6147b1cf19651 using this new flag first, and if it fails, we assume this is an older version of git and just use the old git-diff-index method.

It's hairy to get the 'grep -vq' (inverted matching) correct by stashing the output of git-status (you have to be careful about the difference betwen "empty stdin" and "blank line on stdin"), so just pipe the output directly to grep and use a regex that's good enough for both the git-status and git-diff-index version.

Cc Genki Sky <[email protected]>

Cc Christian Kujau <[email protected]>

Cc Guenter Roeck <[email protected]>

Suggested-by Alexander Kapshuk <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
v1 -> v2:
* handle empty (non-dirty) results properly, where

     echo "${empty_variable}" | grep -qv "${something_else}"

always has a 0 exit status (a blank line is an inverted match for any non-blank expression). Just pipe directly to grep instead, with a hopefully-not-too-permissive regex to handle both versions.
* actually tested with dirty and non-dirty trees this time

v2 -> v3:
* switch to extended regex (-E), instead of relying on GNU extension (\?)
---

 scripts/setlocalversion | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

scripts/setlocalversion +10 -3

index 71f39410691b..365b3c2b8f43 100755
@@ -73,8 +73,16 @@ 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.
+               # First, with git-status, but --no-optional-locks is only
+               # supported in git >= 2.14, so fall back to git-diff-index if
+               # it fails. Note that git-diff-index does not refresh the
+               # index, so it may give misleading results. See
+               # git-update-index(1), git-diff-index(1), and git-status(1).
+               if {
+                       git --no-optional-locks status -uno --porcelain 2>/dev/null ||
+                       git diff-index --name-only HEAD
+               } | grep -qvE '^(.. )?scripts/package'; then
                        printf '%s' -dirty
                fi
 
--

2.19.1.930.g4563a0d9d0-goog

Hi Alexander,

Quoted 35 lines · Alexander Kapshuk

Quoted 33 lines · Brian Norris

Quoted 12 lines · Alexander Kapshuk

An even simpler approach would be:

{
        git --no-optional-locks status -uno --porcelain 2>/dev/null ||
        git diff-index --name-only HEAD
} | grep -qv scripts/package &&

printf '%s' -dirty

Sample run:
cmd
sh: cmd: command not found

Quoted 6 lines

{
        cmd 2>/dev/null ||
        date
} | grep -q 2018 &&

printf '%s' ok
ok

You lose accuracy here, because now you're skipping any line that contains 'scripts/package', which would include, e.g., paths like

tools/some/other-scripts/package

Maybe if the grep expression were more like this?

grep -qv '^\(.. \)\?scripts/package'

I think it'd be safe enough to ignore paths that start with two characters and a space, like:

Quoted 8 lines

Quoted 4 lines

xy scripts/package
x/ scripts/package

Brian

Thanks for your input.
I've found the following grep command, that uses extended regular expressions, to work as required:

Is there any good reason you switched to extended? It looks like my (basic regex) grep was equivalent.

Quoted 18 lines

{
echo hello
echo scripts/package
echo '.. scripts/package'
echo tools/some/other-scripts/package
} | grep -Ev '^(.. )?scripts/package'

[Output]
hello
tools/some/other-scripts/package

If the participants of this email exchange consider the proposed implementation as fitting the bill,

{
        git --no-optional-locks status -uno --porcelain 2>/dev/null ||
        git diff-index --name-only HEAD
} | grep -Eqv '^(.. )?scripts/package' &&

Quoted 6 lines

printf '%s' -dirty

Was the original committer of the patch proposed here,
https://lkml.org/lkml/2018/11/10/55, going to take it in, and resend it as v2 of the patch, or did you want me to submit the patch instead? I would be happy with either way.

I can submit it. I expect Masahiro-san would prefer a proper v2 patch for review, given how much would change from my v1.

And this time, I'll actually test it with a non-dirty tree! (Of course, my tree is naturally dirty when developing the patch, but I missed testing post-commit...)

Brian

Quoted 14 lines · Genki Sky

Quoted 5 lines · Brian Norris

Quoted 4 lines · Brian Norris

On a different tangent: how about the --no-optional-locks (see git(1))? Will this get you your "up-to-date" result without writing to the .git directory? I've only read the documentation, but not tested it.

This option definitely seems to be what we want, good find.

Quoted 2 lines

Unfortunately, --no-optional-locks is new as of git 2.14. Dunno how new of a git we expect people to use.

Hmm, I'm not sure who can speak to this.

Quoted 7 lines

Though if it's too recent, then based on earlier discussion, it sounds like something like this (hack) might work best:

[ -w .git ] &&

          touch .git/some-file-here 2>/dev/null &&
          git update-index --refresh --unmerged >/dev/null

if git diff-index --name-only HEAD | ...

I do not think it is a good idea to create a random file in the .git directory under any circumstance, and much less so if an output directory was specified, no matter if the path is read-only or not. I also still think that it is a bad idea to touch the source tree if an output directory was specified. It defeats the purpose of specifying an output directory.

Ubuntu 16.04 ships with git version 2.7.4.

Guenter

Quoted 10 lines · Brian Norris

Quoted 5 lines

Perhaps both scenarios could be satisfied by having scripts/setlocalversion first check if .git has write permissions, and acting accordingly. Looking into history, this actually used to be done, but cdf2bc632ebc ("scripts/setlocalversion on write-protected source tree", 2013-06-14) removed the updating of the index.

A "writeable" check (e.g., [ -w . ]) would be sufficient for our case. But I'm not so sure about that older NFS report, and I'm also not sure that we should be writing to the source tree at all in this case. Maybe we can also check whether there's a build output directory specified?

FWIW, the issue I reported back in 2013[0] was not an ill-configured NFS export, but a read-only NFS export (and then a read-write exported NFS export, but the user compiling the kernel did not have write permission) and so "test -w .git" did not help in determining if the source tree can actually written to. And depending on the user's shell[1], this may or may not still be the case.

So I'm all for the $(touch .git/some-file-here) test to decide if the kernel has to be modified during build.

Christian.

[0] https://lkml.org/lkml/2013/6/14/574
[1] https://manpages.debian.org/unstable/dash/dash.1.en.html

Quoted 6 lines

Quoted 6 lines

However, I admit I don't understand the justification in that commit from 2013. I'm no NFS expert, but perhaps the real problem there is an incorrectly configured NFS setup (uid/gid mismatch between NFS client/server, or permissions mismatch between mount options and NFS server?). Christian Kujau: can you speak to that?

Quoted 7 lines

Quoted 3 lines

Well, we could also make our check $(touch .git/some-file-here 2>/dev/null && ...) instead of $(test -w .git) to handle misconfigured NFS setups. But not sure if that has its own problems.

Trying to 'touch' the source tree will also break us. No matter whether you redirect stderr, our sandbox will still notice the build is doing something fishy and complain.

--
BOFH excuse #192:

runaway cat on system.

This reverts commit 6147b1cf19651c7de297e69108b141fb30aa2349.

The reverted patch results in attempted write access to the source repository, even if that repository is mounted read-only.

Output from "strace git status -uno --porcelain":

getcwd("/tmp/linux-test", 129) = 16
open("/tmp/linux-test/.git/index.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) =
-1 EROFS (Read-only file system)

While git appears to be able to handle this situation, a monitored build environment (such as the one used for Chrome OS kernel builds) may detect it and bail out with an access violation error. On top of that, the attempted write access suggests that git _will_ write to the file even if a build output directory is specified. Users may have the reasonable expectation that the source repository remains untouched in that situation.

Fixes 6147b1cf19651 ("scripts/setlocalversion: git: Make -dirty check more robust"

Cc Genki Sky <[email protected]>

Cc Masahiro Yamada <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---

 scripts/setlocalversion | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

scripts/setlocalversion +1 -2

index 79f7dd57d571..71f39410691b 100755
@@ -74,7 +74,7 @@ scm_version()
                fi
 
                # Check for uncommitted changes
-               if git status -uno --porcelain | grep -qv '^.. scripts/package'; then
+               if git diff-index --name-only HEAD | grep -qv "^scripts/package"; then
                        printf '%s' -dirty
                fi
 
--

2.7.4