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

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

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

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 <sky@genki.is>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 scripts/setlocalversion | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 79f7dd57d571..71f39410691b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -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

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Genki Sky]

Hi Guenter,

On Tue,  6 Nov 2018 10:10:38 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> 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.

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.

Thoughts? It'd be nice to find a fix that works for everyone.

Genki

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Brian Norris]

Hi Genki,

On Tue, Nov 06, 2018 at 11:23:05AM -0800, Genki Sky wrote:
> On Tue,  6 Nov 2018 10:10:38 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > 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.

I've seen the same problem, by way of working with the same kernel build
system ;)

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

I agree with Guenter, that if you're specifying a different build
directory, the source tree should not be written to at all.

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

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

In any case, I'd be very happy with a Revert for now (for 4.20, and even
-stable), and a follow-up replacement, so:

Reviewed-by: Brian Norris <briannorris@chromium.org>

for the $subject patch.

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Christian Kujau]

On Tue, 6 Nov 2018, Brian Norris wrote:
> > 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

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

-- 
BOFH excuse #192:

runaway cat on system.

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Guenter Roeck]

On 11/6/18 6:58 PM, Christian Kujau wrote:
> On Tue, 6 Nov 2018, Brian Norris wrote:
>>> 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
> 
>>> 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.
> 
For whatever reason, I did not get any of the interim e-mails, only this one.
My apologies to not responding earlier. But then Brian has said it all:
Any write attempt onto the read-only file system will cause our build
system to bail out.

Also, again: I think it is reasonable to expect that the source tree
is not touched if an output directory is specified. Any write attempt
violates this assumption. This most definitely includes any "touch"
commands on the source tree.

Thanks,
Guenter

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Brian Norris]

On Tue, Nov 6, 2018 at 6:58 PM Christian Kujau <lists@nerdbynature.de> wrote:
> 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.

What do you mean, "depending on the user's shell"? AFAICT, it's not
really a shell-specific question, since POSIX defines '-w':

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

(I highly doubt we care about a non-POSIX /bin/sh.)

But the dash man-page you point at is correctly noting that 'test -w'
isn't sufficient for noticing a read-only mount (i.e., you have
permissions, but the mount isn't writeable). Contrary to what Guenter
said in his reply, our build isn't actually off a read-only mount --
it's just running without these write permissions, so 'test -w' will
do the right thing.

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

I suppose that could work, if you do that only after checking 'test
-w'. It's important to not even try to write to the source tree when
not permitted.

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.

Brian

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

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Brian Norris]

On Tue, Nov 06, 2018 at 08:00:36PM -0800, Brian Norris wrote:
> 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.

I think I can add a little to this: that option definitely avoids
writing to the git index (thus, avoiding the problem that Guenter and I
are trying to resolve) and the documentation suggests it should still
get up-to-date index information (thus, avoiding the problem that Genki
was trying to fix). So something like this would work (on top of the
revert):

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..7dfe45badcca 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -74,7 +74,7 @@ scm_version()
 		fi
 
 		# Check for uncommitted changes
-		if git diff-index --name-only HEAD | grep -qv "^scripts/package"; then
+		if git --no-optional-locks status -uno --porcelain | grep -qv '^.. scripts/package'; then
 			printf '%s' -dirty
 		fi
 
Unfortunately, --no-optional-locks is new as of git 2.14. Dunno how new
of a git we expect people to use.

Brian

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Genki Sky]

On Wed, 7 Nov 2018 10:44:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> On Tue, Nov 06, 2018 at 08:00:36PM -0800, Brian Norris wrote:
> > 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.

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

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Guenter Roeck]

On Wed, Nov 07, 2018 at 12:43:58PM -0800, Genki Sky wrote:
> On Wed, 7 Nov 2018 10:44:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> > On Tue, Nov 06, 2018 at 08:00:36PM -0800, Brian Norris wrote:
> > > 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.
> 
> > 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.

Guenter

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Genki Sky]

On Wed, 7 Nov 2018 12:55:14 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> 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.

> Ubuntu 16.04 ships with git version 2.7.4.

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

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Doug Anderson]

Hi,

On Wed, Nov 7, 2018 at 1:07 PM Genki Sky <sky@genki.is> wrote:
>
> On Wed, 7 Nov 2018 12:55:14 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > 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.

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


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

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.

-Doug

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Genki Sky]

On Wed, 7 Nov 2018 13:18:19 -0800, Doug Anderson <dianders@chromium.org> wrote:
> 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

I see, thanks.

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

Okay, it's trading one regression for another, but I guess it's clear
that yours is much more painful. Sorry to have held up progress here.
I'll back out of the discussion until this is reverted.

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Brian Norris]

On Wed, Nov 7, 2018 at 1:18 PM Doug Anderson <dianders@chromium.org> wrote:
> On Wed, Nov 7, 2018 at 1:07 PM Genki Sky <sky@genki.is> wrote:
> > On Wed, 7 Nov 2018 12:55:14 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > > 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.

+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?

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Masahiro Yamada]

On Thu, Nov 8, 2018 at 5:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Nov 07, 2018 at 12:43:58PM -0800, Genki Sky wrote:
> > On Wed, 7 Nov 2018 10:44:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> > > On Tue, Nov 06, 2018 at 08:00:36PM -0800, Brian Norris wrote:
> > > > 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.
> >
> > > 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.


I agree.
We should avoid any write attempt to the source tree for any reason.



> Ubuntu 16.04 ships with git version 2.7.4.
>
> Guenter



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Masahiro Yamada]

On Thu, Nov 8, 2018 at 12:20 PM Brian Norris <briannorris@chromium.org> wrote:
>
> On Wed, Nov 7, 2018 at 1:18 PM Doug Anderson <dianders@chromium.org> wrote:
> > On Wed, Nov 7, 2018 at 1:07 PM Genki Sky <sky@genki.is> wrote:
> > > On Wed, 7 Nov 2018 12:55:14 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > > > 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.
>
> +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?


Thanks!


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Masahiro Yamada]

On Wed, Nov 7, 2018 at 12:05 PM Christian Kujau <lists@nerdbynature.de> wrote:
>
> On Tue, 6 Nov 2018, Brian Norris wrote:
> > > 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.


Hmm, interesting.

The result of "test -w ." depends on the implementation of "test" command.


Bash's build-in 'test' did a good job for me;
"test -w ." returns 1 for read-only mounted NFS.


For Busybox's 'test',
"test -w ." returns 0 for read-only
(or writable, but without no-root-squash) NFS.





> 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
>
> > > 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.
>
> --
> BOFH excuse #192:
>
> runaway cat on system.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Masahiro Yamada]

On Wed, Nov 7, 2018 at 3:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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 <sky@genki.is>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

Applied to linux-kbuild/fixes.


Thanks!


>  scripts/setlocalversion | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 79f7dd57d571..71f39410691b 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -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
>


-- 
Best Regards
Masahiro Yamada

[PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Brian Norris]

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 <sky@genki.is>
Cc: Christian Kujau <lists@nerdbynature.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
On Fri, Nov 09, 2018 at 11:55:26AM +0900, Masahiro Yamada wrote:
> > On Wed, Nov 7, 2018 at 1:18 PM Doug Anderson <dianders@chromium.org> wrote:
> > > On Wed, Nov 7, 2018 at 1:07 PM Genki Sky <sky@genki.is> wrote:
> > > > On Wed, 7 Nov 2018 12:55:14 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > > > > 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.
> >
> > +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?

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

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..eab1f90de50d 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -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
 
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Guenter Roeck]

On Fri, Nov 09, 2018 at 10:34:37AM -0800, Brian Norris wrote:
> 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 <sky@genki.is>
> Cc: Christian Kujau <lists@nerdbynature.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Working for me with git v2.7.4.

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
> On Fri, Nov 09, 2018 at 11:55:26AM +0900, Masahiro Yamada wrote:
> > > On Wed, Nov 7, 2018 at 1:18 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > On Wed, Nov 7, 2018 at 1:07 PM Genki Sky <sky@genki.is> wrote:
> > > > > On Wed, 7 Nov 2018 12:55:14 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > > > > > 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.
> > >
> > > +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?
> 
> 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(-)
> 
> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 71f39410691b..eab1f90de50d 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -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
>  
> -- 
> 2.19.1.930.g4563a0d9d0-goog

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Genki Sky]

Hi, thanks very much for doing this. But, this patch always prints
-dirty for me, even with no untracked changes in git. I think this is
because:

On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 71f39410691b..eab1f90de50d 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -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

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.

With echo:
	$ x=$(printf ''); if echo "$x" | grep -qv 'ignore'; then echo dirty; fi
	dirty
	$ x=$(printf '\n'); if echo "$x" | grep -qv 'ignore'; then echo dirty; fi
	dirty
	$ x=$(printf 'ignore\n'); if echo "$x" | grep -qv 'ignore'; then echo dirty; fi
	$ x=$(printf 'untracked\n'); if echo "$x" | grep -qv 'ignore'; then echo dirty; fi
	dirty

With printf:
	$ x=$(printf ''); if printf '%s' "$x" | grep -qv 'ignore'; then echo dirty; fi
	$ x=$(printf '\n'); if printf '%s' "$x" | grep -qv 'ignore'; then echo dirty; fi
	$ x=$(printf 'ignore\n'); if printf '%s' "$x" | grep -qv 'ignore'; then echo dirty; fi
	$ x=$(printf 'untracked\n'); if printf '%s' "$x" | grep -qv 'ignore'; then echo dirty; fi
	dirty

(Hopefully I'm not missing something.)

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Andreas Schwab]

On Nov 10 2018, Genki Sky <sky@genki.is> wrote:

> On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
>> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
>> index 71f39410691b..eab1f90de50d 100755
>> --- a/scripts/setlocalversion
>> +++ b/scripts/setlocalversion
>> @@ -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
>
> 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.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Genki Sky]

Hi Andreas,

On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Nov 10 2018, Genki Sky <sky@genki.is> wrote:
> > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> >> +		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.
>
> 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.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sat, Nov 10, 2018 at 10:12 PM Genki Sky <sky@genki.is> wrote:
>
> Hi Andreas,
>
> On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab <schwab@linux-m68k.org> wrote:
> > On Nov 10 2018, Genki Sky <sky@genki.is> wrote:
> > > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> > >> +          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.
> >
> > 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.

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
%

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 4:48 PM Alexander Kapshuk
<alexander.kapshuk@gmail.com> wrote:
>
> On Sat, Nov 10, 2018 at 10:12 PM Genki Sky <sky@genki.is> wrote:
> >
> > Hi Andreas,
> >
> > On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab <schwab@linux-m68k.org> wrote:
> > > On Nov 10 2018, Genki Sky <sky@genki.is> wrote:
> > > > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris <briannorris@chromium.org> wrote:
> > > >> +          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.
> > >
> > > 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.
>
> 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
> %

To improve the readability, the git command may be stored in a variable
and substituted into a command within the if block like so:

% cmd='git
--no-optional-locks
status
-uno
--porcelain'

% if $cmd 2>/dev/null | grep -qv '^.. scripts/package'; then
        printf '%s' -dirty
fi
%

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Genki Sky]

Hi Alexander,

On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk <alexander.kapshuk@gmail.com> wrote:
> 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

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.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 7:41 PM Genki Sky <sky@genki.is> wrote:
>
> Hi Alexander,
>
> On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk <alexander.kapshuk@gmail.com> wrote:
> > 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
>
> 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

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 9:59 PM Alexander Kapshuk
<alexander.kapshuk@gmail.com> wrote:
>
> On Sun, Nov 11, 2018 at 7:41 PM Genki Sky <sky@genki.is> wrote:
> >
> > Hi Alexander,
> >
> > On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk <alexander.kapshuk@gmail.com> wrote:
> > > 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
> >
> > 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

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Brian Norris]

On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> 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

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

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Tue, Nov 13, 2018 at 2:09 AM Brian Norris <briannorris@chromium.org> wrote:
>
> On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > 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
>
> 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

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.

Thanks.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Brian Norris]

Hi Alexander,

On Tue, Nov 13, 2018 at 12:36 AM Alexander Kapshuk
<alexander.kapshuk@gmail.com> wrote:
>
> On Tue, Nov 13, 2018 at 2:09 AM Brian Norris <briannorris@chromium.org> wrote:
> >
> > On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > > 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
> >
> > 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
>
> 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.

> {
>         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.

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

[PATCH v2] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Brian Norris]

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 <sky@genki.is>
Cc: Christian Kujau <lists@nerdbynature.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Suggested-by: Alexander Kapshuk <alexander.kapshuk@gmail.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

On Tue, Nov 13, 2018 at 10:32:16AM -0800, Brian Norris wrote:
> I can submit it. I expect Masahiro-san would prefer a proper v2 patch
> for review, given how much would change from my v1.

Done

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

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

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..2190de4b57ad 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -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 -qv '^\(.. \)\?scripts/package'; then
 			printf '%s' -dirty
 		fi
 
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Tue, Nov 13, 2018 at 8:32 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Alexander,
>
> On Tue, Nov 13, 2018 at 12:36 AM Alexander Kapshuk
> <alexander.kapshuk@gmail.com> wrote:
> >
> > On Tue, Nov 13, 2018 at 2:09 AM Brian Norris <briannorris@chromium.org> wrote:
> > >
> > > On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > > > 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
> > >
> > > 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
> >
> > 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.
>
> > {
> >         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.
>
> 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

Hi Brian,

Thanks for taking my proposed patch in and submitting it.

I went with the ERE because, in my opinion, the pattern looks
'cleaner' without having the metacharacters like '()?' escaped.
EREs are POSIX-compatible, so should be supported by any POSIX-compliant shell.

Your non-ERE version does generate the same output as my ERE one. So
it just boils down to one's personal preference.

Thanks.

Re: [PATCH v2] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Genki Sky]

Thanks all,

On Tue, 13 Nov 2018 11:03:36 -0800, Brian Norris <briannorris@chromium.org> wrote:
> [ ... ]
>
> Cc: Genki Sky <sky@genki.is>
> Cc: Christian Kujau <lists@nerdbynature.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Alexander Kapshuk <alexander.kapshuk@gmail.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Tested-by: Genki Sky <sky@genki.is>

Re: [PATCH v2] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Andreas Schwab]

On Nov 13 2018, Brian Norris <briannorris@chromium.org> wrote:

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

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

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Brian Norris]

On Tue, Nov 13, 2018 at 2:03 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Nov 13 2018, Brian Norris <briannorris@chromium.org> wrote:
>
> > +             } | grep -qv '^\(.. \)\?scripts/package'; then
>
> \? is a GNU extension, so if you want to stay portable you need to
> switch to ERE.

Ack. That's what I get for reading the GNU man pages... I'll send a v3.

Thanks,
Brian

[PATCH v3] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Brian Norris]

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 <sky@genki.is>
Cc: Christian Kujau <lists@nerdbynature.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Suggested-by: Alexander Kapshuk <alexander.kapshuk@gmail.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
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(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..365b3c2b8f43 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -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

Re: [PATCH v3] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Masahiro Yamada]

On Thu, Nov 15, 2018 at 11:12 AM Brian Norris <briannorris@chromium.org> wrote:
>
> 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 <sky@genki.is>
> Cc: Christian Kujau <lists@nerdbynature.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Alexander Kapshuk <alexander.kapshuk@gmail.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>


Worked for me, and clean implementation. Nice!

I will wait a few days before I pick it up
in case some people may give comments, Reviewed-by, Tested-by, etc.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Masahiro Yamada]

On Thu, Nov 15, 2018 at 11:12 AM Brian Norris <briannorris@chromium.org> wrote:
>
> 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 <sky@genki.is>
> Cc: Christian Kujau <lists@nerdbynature.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Alexander Kapshuk <alexander.kapshuk@gmail.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 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
>    (\?)
> ---


Applied to linux-kbuild.
Thanks!


>  scripts/setlocalversion | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 71f39410691b..365b3c2b8f43 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -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



-- 
Best Regards
Masahiro Yamada