[Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Alexey Kardashevskiy]

The configure script checks multiple times whether it works in a git
repository and it does this by "test -e "${source_path}/.git" in 4 cases
but in one case where it tries to enable werror "-d" is used there which
fails on git worktrees as .git is a file then and not a directory.

This changes the test to "-e" as other occurrences.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 05d72f1..f481ff9 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ fi
 # Consult white-list to determine whether to enable werror
 # by default.  Only enable by default for git builds
 if test -z "$werror" ; then
-    if test -d "$source_path/.git" && \
+    if test -e "$source_path/.git" && \
         { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then
         werror="yes"
     else
-- 
2.17.1

Re: [Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Thu, Feb 28, 2019 at 03:35:03PM +1100, Alexey Kardashevskiy wrote:
> The configure script checks multiple times whether it works in a git
> repository and it does this by "test -e "${source_path}/.git" in 4 cases
> but in one case where it tries to enable werror "-d" is used there which
> fails on git worktrees as .git is a file then and not a directory.
> 
> This changes the test to "-e" as other occurrences.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

CCing a few likely candidates based on get_maintainer -f

> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 05d72f1..f481ff9 100755
> --- a/configure
> +++ b/configure
> @@ -1835,7 +1835,7 @@ fi
>  # Consult white-list to determine whether to enable werror
>  # by default.  Only enable by default for git builds
>  if test -z "$werror" ; then
> -    if test -d "$source_path/.git" && \
> +    if test -e "$source_path/.git" && \
>          { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then
>          werror="yes"
>      else

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On 28/02/2019 06.00, David Gibson wrote:
> On Thu, Feb 28, 2019 at 03:35:03PM +1100, Alexey Kardashevskiy wrote:
>> The configure script checks multiple times whether it works in a git
>> repository and it does this by "test -e "${source_path}/.git" in 4 cases
>> but in one case where it tries to enable werror "-d" is used there which
>> fails on git worktrees as .git is a file then and not a directory.

That confused me. What is a "git worktree" where .git is a file? So far
.git was always a directory here...?

 Thomas


>> This changes the test to "-e" as other occurrences.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> CCing a few likely candidates based on get_maintainer -f
> 
>> ---
>>  configure | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 05d72f1..f481ff9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1835,7 +1835,7 @@ fi
>>  # Consult white-list to determine whether to enable werror
>>  # by default.  Only enable by default for git builds
>>  if test -z "$werror" ; then
>> -    if test -d "$source_path/.git" && \
>> +    if test -e "$source_path/.git" && \
>>          { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then
>>          werror="yes"
>>      else
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Gerd Hoffmann]

On Thu, Feb 28, 2019 at 08:01:53AM +0100, Thomas Huth wrote:
> On 28/02/2019 06.00, David Gibson wrote:
> > On Thu, Feb 28, 2019 at 03:35:03PM +1100, Alexey Kardashevskiy wrote:
> >> The configure script checks multiple times whether it works in a git
> >> repository and it does this by "test -e "${source_path}/.git" in 4 cases
> >> but in one case where it tries to enable werror "-d" is used there which
> >> fails on git worktrees as .git is a file then and not a directory.
> 
> That confused me. What is a "git worktree" where .git is a file? So far
> .git was always a directory here...?

git worktree --help

A worktree is basically a clone, but instead of copying over the git
repo it is simply referenced.

That is not the only possible case where .git is a file not a directory.
In newer versions "git submodule" stores the repo in
.git/modules/$submodule and $submodule/.git is just a file with a
reference:

kraxel@sirius ~/projects/qemu# cat roms/seabios/.git
gitdir: ../../.git/modules/roms/seabios

So, yes, the patch makes sense.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Thomas Huth]

On 28/02/2019 08.14, Gerd Hoffmann wrote:
> On Thu, Feb 28, 2019 at 08:01:53AM +0100, Thomas Huth wrote:
>> On 28/02/2019 06.00, David Gibson wrote:
>>> On Thu, Feb 28, 2019 at 03:35:03PM +1100, Alexey Kardashevskiy wrote:
>>>> The configure script checks multiple times whether it works in a git
>>>> repository and it does this by "test -e "${source_path}/.git" in 4 cases
>>>> but in one case where it tries to enable werror "-d" is used there which
>>>> fails on git worktrees as .git is a file then and not a directory.
>>
>> That confused me. What is a "git worktree" where .git is a file? So far
>> .git was always a directory here...?
> 
> git worktree --help

Thanks a lot for the explanation, now it makes sense, indeed.

Anyway, my local git from RHEL7 is obviously too old for that:

$ git worktree --help
No manual entry for gitworktree
$ git worktree
git: 'worktree' is not a git command. See 'git --help'.
$ git --version
git version 1.8.3.1

 Thomas

Re: [Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Paolo Bonzini]

On 28/02/19 08:14, Gerd Hoffmann wrote:
> On Thu, Feb 28, 2019 at 08:01:53AM +0100, Thomas Huth wrote:
>> On 28/02/2019 06.00, David Gibson wrote:
>>> On Thu, Feb 28, 2019 at 03:35:03PM +1100, Alexey Kardashevskiy wrote:
>>>> The configure script checks multiple times whether it works in a git
>>>> repository and it does this by "test -e "${source_path}/.git" in 4 cases
>>>> but in one case where it tries to enable werror "-d" is used there which
>>>> fails on git worktrees as .git is a file then and not a directory.
>>
>> That confused me. What is a "git worktree" where .git is a file? So far
>> .git was always a directory here...?
> 
> git worktree --help
> 
> A worktree is basically a clone, but instead of copying over the git
> repo it is simply referenced.
> 
> That is not the only possible case where .git is a file not a directory.
> In newer versions "git submodule" stores the repo in
> .git/modules/$submodule and $submodule/.git is just a file with a
> reference:
> 
> kraxel@sirius ~/projects/qemu# cat roms/seabios/.git
> gitdir: ../../.git/modules/roms/seabios
> 
> So, yes, the patch makes sense.
> 
> cheers,
>   Gerd
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH qemu] configure: Enable werror for git worktrees

[Peter Maydell]

On Thu, 28 Feb 2019 at 04:36, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> The configure script checks multiple times whether it works in a git
> repository and it does this by "test -e "${source_path}/.git" in 4 cases
> but in one case where it tries to enable werror "-d" is used there which
> fails on git worktrees as .git is a file then and not a directory.
>
> This changes the test to "-e" as other occurrences.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Given that there is this non-obvious pitfall in trying to
hand-code the "are we running from git?" check and that
we do it in five places in configure, that suggests that
we should abstract this out:

sources_in_git() {
    # Note that -d will give the wrong answer for git worktrees
    test -e "$source_path/.git"
}

and then use wherever we're currently checking by hand:
    if sources_in_git && ...

(warning: code not tested at all)

Defining a sources_in_git variable which we set at
the start of the script would work too; no strong preference.

thanks
-- PMM