[git-gui] bug report: "Open existing repository" dialog fails on submodules

[git-gui] bug report: "Open existing repository" dialog fails on submodules

[Rémi Rampin]

Hi,

This bug report concerns git-gui. Apologies if this is not the right
mailing-list.

By submodule I mean a repository for which .git is not a regular Git
directory, but rather a "gitdir: ..." file.
While running "git gui" from such a directory will work fine, trying
to open it from the choose_repository window will fail with "Not a Git
repository". This is because of the simplistic implementation of proc
_is_git in lib/choose_repository.tcl.

I suggest fixing that function, or using Git directly to perform that
check, for instance checking "git rev-parse --show-toplevel". I'd
attempt a patch but my tcl-fu is weak.

Best
-- 
Rémi Rampin

Re: [git-gui] bug report: "Open existing repository" dialog fails on submodules

[Chris Packham]

Hi,

On Sat, Jan 31, 2015 at 10:46 AM, Rémi Rampin <remirampin@gmail.com> wrote:
> Hi,
>
> This bug report concerns git-gui. Apologies if this is not the right
> mailing-list.
>
> By submodule I mean a repository for which .git is not a regular Git
> directory, but rather a "gitdir: ..." file.
> While running "git gui" from such a directory will work fine, trying
> to open it from the choose_repository window will fail with "Not a Git
> repository". This is because of the simplistic implementation of proc
> _is_git in lib/choose_repository.tcl.
>
> I suggest fixing that function, or using Git directly to perform that
> check, for instance checking "git rev-parse --show-toplevel". I'd
> attempt a patch but my tcl-fu is weak.
>

I would have thought the following would work

--- 8< ---
Subject: [PATCH] git-gui: use git rev-parse to validate paths

The current _is_git function to validate a path as a git repository does
not handle a gitfiles which have been used for submodules for some time.
Instead of using a custom function let's just ask git rev-parse.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 lib/choose_repository.tcl | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..944ab50 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,19 +339,12 @@ method _git_init {} {
 }

 proc _is_git {path} {
-       if {[file exists [file join $path HEAD]]
-        && [file exists [file join $path objects]]
-        && [file exists [file join $path config]]} {
+       puts $path
+       if {[catch {exec git rev-parse --resolve-git-dir $path}]} {
+               return 0
+       } else {
                return 1
        }
-       if {[is_Cygwin]} {
-               if {[file exists [file join $path HEAD]]
-                && [file exists [file join $path objects.lnk]]
-                && [file exists [file join $path config.lnk]]} {
-                       return 1
-               }
-       }
-       return 0
 }

 proc _objdir {path} {
-- 
2.3.0.rc2
--- >8 ---

But it actually looks like git rev-parse --resolve-git-dir $path needs
to be run inside a git repository _any_ git repository, which seems a
bit backwards to me.

  $ cd
  $ git rev-parse --resolve-git-dir ~/src/git/.git
  fatal: Not a git repository (or any parent up to mount point /home)
  Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

  $ cd ~/src/git
  $ git rev-parse --resolve-git-dir ~/src/git-gui/.git
  /home/chrisp/src/git-gui/.git

So one potential fix git a gui-gui bug, one new(?) bug in git rev-parse.

Re: [git-gui] bug report: "Open existing repository" dialog fails on submodules

[Chris Packham]

On Mon, Feb 2, 2015 at 9:41 PM, Chris Packham <judge.packham@gmail.com> wrote:
> Hi,
>
> On Sat, Jan 31, 2015 at 10:46 AM, Rémi Rampin <remirampin@gmail.com> wrote:
>> Hi,
>>
>> This bug report concerns git-gui. Apologies if this is not the right
>> mailing-list.
>>
>> By submodule I mean a repository for which .git is not a regular Git
>> directory, but rather a "gitdir: ..." file.
>> While running "git gui" from such a directory will work fine, trying
>> to open it from the choose_repository window will fail with "Not a Git
>> repository". This is because of the simplistic implementation of proc
>> _is_git in lib/choose_repository.tcl.
>>
>> I suggest fixing that function, or using Git directly to perform that
>> check, for instance checking "git rev-parse --show-toplevel". I'd
>> attempt a patch but my tcl-fu is weak.
>>
>
> I would have thought the following would work
>
> --- 8< ---
> Subject: [PATCH] git-gui: use git rev-parse to validate paths
>
> The current _is_git function to validate a path as a git repository does
> not handle a gitfiles which have been used for submodules for some time.
> Instead of using a custom function let's just ask git rev-parse.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  lib/choose_repository.tcl | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
> index 92d6022..944ab50 100644
> --- a/lib/choose_repository.tcl
> +++ b/lib/choose_repository.tcl
> @@ -339,19 +339,12 @@ method _git_init {} {
>  }
>
>  proc _is_git {path} {
> -       if {[file exists [file join $path HEAD]]
> -        && [file exists [file join $path objects]]
> -        && [file exists [file join $path config]]} {
> +       puts $path
> +       if {[catch {exec git rev-parse --resolve-git-dir $path}]} {
> +               return 0
> +       } else {
>                 return 1
>         }
> -       if {[is_Cygwin]} {
> -               if {[file exists [file join $path HEAD]]
> -                && [file exists [file join $path objects.lnk]]
> -                && [file exists [file join $path config.lnk]]} {
> -                       return 1
> -               }
> -       }
> -       return 0
>  }
>
>  proc _objdir {path} {
> --
> 2.3.0.rc2
> --- >8 ---
>
> But it actually looks like git rev-parse --resolve-git-dir $path needs
> to be run inside a git repository _any_ git repository, which seems a
> bit backwards to me.
>
>   $ cd
>   $ git rev-parse --resolve-git-dir ~/src/git/.git
>   fatal: Not a git repository (or any parent up to mount point /home)
>   Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
>
>   $ cd ~/src/git
>   $ git rev-parse --resolve-git-dir ~/src/git-gui/.git
>   /home/chrisp/src/git-gui/.git
>
> So one potential fix git a gui-gui bug, one new(?) bug in git rev-parse.

Not a new one. Happens in 1.9.1. Still a bit counter-intuitive IMO.

Re: [git-gui] bug report: "Open existing repository" dialog fails on submodules

[Rémi Rampin]

2015-02-02 3:41 UTC-05:00, Chris Packham <judge.packham@gmail.com>:
> [...]
> But it actually looks like git rev-parse --resolve-git-dir $path needs
> to be run inside a git repository _any_ git repository, which seems a
> bit backwards to me.
> [...]

Indeed, looking at git-rev-parse(1), the correct option might be
--show-toplevel, which will print the cwd if it is the top-level of a
non-bare repository:

    cd $candidate && test $(git rev-parse --show-toplevel) = $candidate

or

    test $(git --git-dir=$candidate rev-parse --show-toplevel) = $candidate

Of course Git will resolve symlinks at this point, so $candidate has
to be resolved first for the equality to make sense.

Other solution is to parse the "gitdir: ..." format and recurse, which
is not exactly hard (provided you speak Tcl).

[PATCH 1/2] Fixes _is_git

[Remi Rampin]

Function _git_dir would previously fail to accept a "gitdir: ..." file
as a valid Git repository.
---
 lib/choose_repository.tcl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..49ff641 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,6 +339,16 @@ method _git_init {} {
 }
 
 proc _is_git {path} {
+	if {[file isfile $path]} {
+		set fp [open $path r]
+		gets $fp line
+		close $fp
+		if {[regexp "^gitdir: (.+)$" $line line link_target]} {
+			return [_is_git [file join [file dirname $path] $link_target]]
+		}
+		return 0
+	}
+
 	if {[file exists [file join $path HEAD]]
 	 && [file exists [file join $path objects]]
 	 && [file exists [file join $path config]]} {
-- 
1.9.5.msysgit.0

[PATCH 2/2] Makes _do_open2 set _gitdir to actual path

[Remi Rampin]

If _is_git had to follow "gitdir: ..." files to reach the actual Git
directory, we set _gitdir to that final path.
---
 lib/choose_repository.tcl | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 49ff641..641068d 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -338,13 +338,17 @@ method _git_init {} {
 	return 1
 }
 
-proc _is_git {path} {
+proc _is_git {path {outdir_var ""}} {
+	if {$outdir_var ne ""} {
+		upvar 1 $outdir_var outdir
+	}
 	if {[file isfile $path]} {
 		set fp [open $path r]
 		gets $fp line
 		close $fp
 		if {[regexp "^gitdir: (.+)$" $line line link_target]} {
-			return [_is_git [file join [file dirname $path] $link_target]]
+			set link_target_abs [file join [file dirname $path] $link_target]
+			return [_is_git $link_target_abs outdir]
 		}
 		return 0
 	}
@@ -352,12 +356,14 @@ proc _is_git {path} {
 	if {[file exists [file join $path HEAD]]
 	 && [file exists [file join $path objects]]
 	 && [file exists [file join $path config]]} {
+		set outdir $path
 		return 1
 	}
 	if {[is_Cygwin]} {
 		if {[file exists [file join $path HEAD]]
 		 && [file exists [file join $path objects.lnk]]
 		 && [file exists [file join $path config.lnk]]} {
+			set outdir $path
 			return 1
 		}
 	}
@@ -1103,7 +1109,7 @@ method _open_local_path {} {
 }
 
 method _do_open2 {} {
-	if {![_is_git [file join $local_path .git]]} {
+	if {![_is_git [file join $local_path .git] actualgit]} {
 		error_popup [mc "Not a Git repository: %s" [file tail $local_path]]
 		return
 	}
@@ -1116,7 +1122,7 @@ method _do_open2 {} {
 	}
 
 	_append_recentrepos [pwd]
-	set ::_gitdir .git
+	set ::_gitdir $actualgit
 	set ::_prefix {}
 	set done 1
 }
-- 
1.9.5.msysgit.0

Re: [PATCH 1/2] Fixes _is_git

[Chris Packham]

Hi Remi,

Added Pat Thoyts the git-gui maintainer.

(Disclaimer, it's been years since I did anything with Tcl).

On Tue, Feb 3, 2015 at 6:24 AM, Remi Rampin <remirampin@gmail.com> wrote:
> Function _git_dir would previously fail to accept a "gitdir: ..." file
> as a valid Git repository.
> ---
>  lib/choose_repository.tcl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
> index 92d6022..49ff641 100644
> --- a/lib/choose_repository.tcl
> +++ b/lib/choose_repository.tcl
> @@ -339,6 +339,16 @@ method _git_init {} {
>  }
>
>  proc _is_git {path} {
> +       if {[file isfile $path]} {
> +               set fp [open $path r]
> +               gets $fp line
> +               close $fp
> +               if {[regexp "^gitdir: (.+)$" $line line link_target]} {

It might be simpler to use one of the 'string' commands e.g. string
wordend "gitdir: " I also suspect the string functions would be faster
than regexp but that probably doesn't matter.

> +                       return [_is_git [file join [file dirname $path] $link_target]]

Do we want to avoid pathological cases of infinite recursion? Someone
would have to maliciously create such a situation.

> +               }
> +               return 0
> +       }
> +
>         if {[file exists [file join $path HEAD]]
>          && [file exists [file join $path objects]]
>          && [file exists [file join $path config]]} {
> --
> 1.9.5.msysgit.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Makes _do_open2 set _gitdir to actual path

[Chris Packham]

On Tue, Feb 3, 2015 at 6:24 AM, Remi Rampin <remirampin@gmail.com> wrote:
> If _is_git had to follow "gitdir: ..." files to reach the actual Git
> directory, we set _gitdir to that final path.
> ---
>  lib/choose_repository.tcl | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
> index 49ff641..641068d 100644
> --- a/lib/choose_repository.tcl
> +++ b/lib/choose_repository.tcl
> @@ -338,13 +338,17 @@ method _git_init {} {
>         return 1
>  }
>
> -proc _is_git {path} {
> +proc _is_git {path {outdir_var ""}} {
> +       if {$outdir_var ne ""} {
> +               upvar 1 $outdir_var outdir
> +       }
>         if {[file isfile $path]} {
>                 set fp [open $path r]
>                 gets $fp line
>                 close $fp
>                 if {[regexp "^gitdir: (.+)$" $line line link_target]} {
> -                       return [_is_git [file join [file dirname $path] $link_target]]
> +                       set link_target_abs [file join [file dirname $path] $link_target]

At this point link_target_abs is something like
sub/../.git/modules/sub. It might be nice to normalize this with 'git
rev-parse --git-dir' or even just (cd $link_target_abs && pwd). I'm
not sure if tcl has anything built in that could do this kind of
normalization.

> +                       return [_is_git $link_target_abs outdir]
>                 }
>                 return 0
>         }
> @@ -352,12 +356,14 @@ proc _is_git {path} {
>         if {[file exists [file join $path HEAD]]
>          && [file exists [file join $path objects]]
>          && [file exists [file join $path config]]} {
> +               set outdir $path
>                 return 1
>         }
>         if {[is_Cygwin]} {
>                 if {[file exists [file join $path HEAD]]
>                  && [file exists [file join $path objects.lnk]]
>                  && [file exists [file join $path config.lnk]]} {
> +                       set outdir $path
>                         return 1
>                 }
>         }
> @@ -1103,7 +1109,7 @@ method _open_local_path {} {
>  }
>
>  method _do_open2 {} {
> -       if {![_is_git [file join $local_path .git]]} {
> +       if {![_is_git [file join $local_path .git] actualgit]} {
>                 error_popup [mc "Not a Git repository: %s" [file tail $local_path]]
>                 return
>         }
> @@ -1116,7 +1122,7 @@ method _do_open2 {} {
>         }
>
>         _append_recentrepos [pwd]
> -       set ::_gitdir .git
> +       set ::_gitdir $actualgit
>         set ::_prefix {}
>         set done 1
>  }
> --
> 1.9.5.msysgit.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Fixes _is_git

[Rémi Rampin]

2015-02-02 12:24 UTC-05:00, Remi Rampin <remirampin@gmail.com>:
>>  proc _is_git {path} {
>> +       if {[file isfile $path]} {
>> +               set fp [open $path r]
>> +               gets $fp line
>> +               close $fp
>> +               if {[regexp "^gitdir: (.+)$" $line line link_target]} {

2015-02-03 3:44 UTC-05:00, Chris Packham <judge.packham@gmail.com>:
> It might be simpler to use one of the 'string' commands e.g. string
> wordend "gitdir: " I also suspect the string functions would be faster
> than regexp but that probably doesn't matter.

I want to check that the file actually begins with "gitdir: " and then
extract the path, so I'm not sure if using string functions is that
simple/fast.

>> +                       return [_is_git [file join [file dirname $path] $link_target]]

> Do we want to avoid pathological cases of infinite recursion? Someone
> would have to maliciously create such a situation.

Limiting the recursion is very simple, but I'm not sure people are
supposed to stumble on that. More importantly this probably calls for a
different error message, thus a new error result that I am not ready to
implement. But it could be another patch.
But I suppose I can add a simple "return 0" limit to the recursion if
needed, let me know.

Re: [PATCH 2/2] Makes _do_open2 set _gitdir to actual path

[Rémi Rampin]

2015-02-02 12:24 UTC-05:00, Remi Rampin <remirampin@gmail.com>:
>> -                       return [_is_git [file join [file dirname $path] $link_target]]
>> +                       set link_target_abs [file join [file dirname $path] $link_target]

2015-02-03 3:51 UTC-05:00, Chris Packham <judge.packham@gmail.com>:
> At this point link_target_abs is something like
> sub/../.git/modules/sub. It might be nice to normalize this with 'git
> rev-parse --git-dir' or even just (cd $link_target_abs && pwd). I'm
> not sure if tcl has anything built in that could do this kind of
> normalization.

There is 'file normalize' according to the docs. I can update the patch
if needed.

Re: [PATCH 1/2] Fixes _is_git

[Chris Packham]

On Wed, Feb 4, 2015 at 4:52 AM, Rémi Rampin <remirampin@gmail.com> wrote:
> 2015-02-02 12:24 UTC-05:00, Remi Rampin <remirampin@gmail.com>:
>>>  proc _is_git {path} {
>>> +       if {[file isfile $path]} {
>>> +               set fp [open $path r]
>>> +               gets $fp line
>>> +               close $fp
>>> +               if {[regexp "^gitdir: (.+)$" $line line link_target]} {
>
> 2015-02-03 3:44 UTC-05:00, Chris Packham <judge.packham@gmail.com>:
>> It might be simpler to use one of the 'string' commands e.g. string
>> wordend "gitdir: " I also suspect the string functions would be faster
>> than regexp but that probably doesn't matter.
>
> I want to check that the file actually begins with "gitdir: " and then
> extract the path, so I'm not sure if using string functions is that
> simple/fast.

Makes sense.

>
>>> +                       return [_is_git [file join [file dirname $path] $link_target]]
>
>> Do we want to avoid pathological cases of infinite recursion? Someone
>> would have to maliciously create such a situation.
>
> Limiting the recursion is very simple, but I'm not sure people are
> supposed to stumble on that. More importantly this probably calls for a
> different error message, thus a new error result that I am not ready to
> implement. But it could be another patch.
> But I suppose I can add a simple "return 0" limit to the recursion if
> needed, let me know.

It'd have to be fairly intentional to cause any real problems. The one
thing I was thinking was to factor out the part that checks for HEAD
info objects etc into a __is_git that _is_git could call thus
eliminating recursion but I don't see it really being anything more
than a theoretical issue.

[PATCH 0/2] gitfile support git git-gui

[Remi Rampin]

New patch series. I hadn't realized Git doesn't recurse on "gitdir: ..."
links itself, it only follows one.

Also normalizes the path to the Git repository as requested.

Remi Rampin (2):
  Fixes chooser not accepting gitfiles
  Makes chooser set 'gitdir' to the resolved path

 lib/choose_repository.tcl | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

-- 
1.9.5.msysgit.0

[PATCH 1/2] Fixes chooser not accepting gitfiles

[Remi Rampin]

Makes _is_git handle the case where the path is a "gitdir: ..." file.
---
 lib/choose_repository.tcl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..abc6b1d 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,6 +339,16 @@ method _git_init {} {
 }
 
 proc _is_git {path} {
+	if {[file isfile $path]} {
+		set fp [open $path r]
+		gets $fp line
+		close $fp
+		if {[regexp "^gitdir: (.+)$" $line line link_target]} {
+			set path [file join [file dirname $path] $link_target]
+			set path [file normalize $path]
+		}
+	}
+
 	if {[file exists [file join $path HEAD]]
 	 && [file exists [file join $path objects]]
 	 && [file exists [file join $path config]]} {
-- 
1.9.5.msysgit.0

[PATCH 2/2] Makes chooser set 'gitdir' to the resolved path

[Remi Rampin]

If _is_git follows a "gitdir: ..." file link to get to the actual
repository, we want _gitdir to be set to that final path.
---
 lib/choose_repository.tcl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index abc6b1d..75d1da8 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -338,7 +338,10 @@ method _git_init {} {
 	return 1
 }
 
-proc _is_git {path} {
+proc _is_git {path {outdir_var ""}} {
+	if {$outdir_var ne ""} {
+		upvar 1 $outdir_var outdir
+	}
 	if {[file isfile $path]} {
 		set fp [open $path r]
 		gets $fp line
@@ -352,12 +355,14 @@ proc _is_git {path} {
 	if {[file exists [file join $path HEAD]]
 	 && [file exists [file join $path objects]]
 	 && [file exists [file join $path config]]} {
+		set outdir $path
 		return 1
 	}
 	if {[is_Cygwin]} {
 		if {[file exists [file join $path HEAD]]
 		 && [file exists [file join $path objects.lnk]]
 		 && [file exists [file join $path config.lnk]]} {
+			set outdir $path
 			return 1
 		}
 	}
@@ -1103,7 +1108,7 @@ method _open_local_path {} {
 }
 
 method _do_open2 {} {
-	if {![_is_git [file join $local_path .git]]} {
+	if {![_is_git [file join $local_path .git] actualgit]} {
 		error_popup [mc "Not a Git repository: %s" [file tail $local_path]]
 		return
 	}
@@ -1116,7 +1121,7 @@ method _do_open2 {} {
 	}
 
 	_append_recentrepos [pwd]
-	set ::_gitdir .git
+	set ::_gitdir $actualgit
 	set ::_prefix {}
 	set done 1
 }
-- 
1.9.5.msysgit.0

[PATCH 1/2] Fixes chooser not accepting gitfiles

[Remi Rampin]

Makes _is_git handle the case where the path is a "gitdir: ..." file.

Signed-off-by: Remi Rampin <remirampin@gmail.com>
---
 lib/choose_repository.tcl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..abc6b1d 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,6 +339,16 @@ method _git_init {} {
 }
 
 proc _is_git {path} {
+	if {[file isfile $path]} {
+		set fp [open $path r]
+		gets $fp line
+		close $fp
+		if {[regexp "^gitdir: (.+)$" $line line link_target]} {
+			set path [file join [file dirname $path] $link_target]
+			set path [file normalize $path]
+		}
+	}
+
 	if {[file exists [file join $path HEAD]]
 	 && [file exists [file join $path objects]]
 	 && [file exists [file join $path config]]} {
-- 
1.9.5.msysgit.0