| Message ID | 20190228043503.68494-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [qemu] configure: Enable werror for git worktrees | expand |
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
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 >
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
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
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
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
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
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(-)