threads / discuss / 17881
Subject: [PATCHv2 0/2] git-gui: generic and robust worktree/gitdir support
Giuseppe BilottaVersion 2 of the patchest to make git-gui work with non-standard gitdir/worktree locations, by making it support the GIT_DIR and GIT_WORK_TREE environment variables. As a plus we also get improved detection and support for bare repositories.
Giuseppe Bilotta (2): git-gui: handle non-standard worktree locations git-gui: handle bare repos correctly
git-gui/git-gui.sh | 69 ++++++++++++++++++++++++++++++++++++++++----------- 1 files changed, 54 insertions(+), 15 deletions(-)
Giuseppe Bilotta[PATCHv2 1/2] git-gui: handle non-standard worktree locations
Don't rely on the git worktree being the updir of the gitdir, since it might not be. Instead, define (and use) a new _gitworktree global variable, setting it to $GIT_WORK_TREE if present, falling back to core.worktree if defined, and finally to whatever we guess the correct worktree is. Getting core.worktree requires the config from the alleged git dir _gitdir to be loaded early.
Supporting non-standard worktree locations also breaks the git-gui assumption (made when calling gitk) that the worktree was the dirname of $_gitdir and that, by consequence, the git dir could be set to the tail of $_gitdir once we changed to the worktree root directory. Therefore, we need to export a GIT_DIR environment variable set to the full, normalized path of $_gitdir instead. We also skip changing to the worktree directory if it's empty (i.e. if we're working on a bare repository). --- git-gui/git-gui.sh | 37 ++++++++++++++++++++++++++++--------- 1 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh index e018e07..12c7ff3 100755 --- a/git-gui/git-gui.sh +++ b/git-gui/git-gui.sh @@ -121,6 +121,7 @@ unset oguimsg set _appname {Git Gui} set _gitdir {} +set _gitworktree {} set _gitexec {} set _reponame {} set _iscygwin {} @@ -1062,13 +1063,25 @@ if {![file isdirectory $_gitdir]} { error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"] exit 1 } +# _gitdir exists, so try loading the config +load_config 0 +apply_config +# try to set work tree from environment, falling back to core.worktree +if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} { + set _gitworktree [get_config core.worktree] +} if {$_prefix ne {}} { - regsub -all {[^/]+/} $_prefix ../ cdup + if {$_gitworktree eq {}} { + regsub -all {[^/]+/} $_prefix ../ cdup + } else { + set cdup $_gitworktree + } if {[catch {cd $cdup} err]} { catch {wm withdraw .} error_popup [strcat [mc "Cannot move to top of working directory:"] "\n\n$err"] exit 1 } + set _gitworktree [pwd] unset cdup } elseif {![is_enabled bare]} { if {[lindex [file split $_gitdir] end] ne {.git}} { @@ -1076,11 +1089,15 @@ if {$_prefix ne {}} { error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"] exit 1 } - if {[catch {cd [file dirname $_gitdir]} err]} { + if {$_gitworktree eq {}} { + set _gitworktree [file dirname $_gitdir] + } + if {[catch {cd $_gitworktree} err]} { catch {wm withdraw .} - error_popup [strcat [mc "No working directory"] " [file dirname $_gitdir]:\n\n$err"] + error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"] exit 1 } + set _gitworktree [pwd] } set _reponame [file split [file normalize $_gitdir]] if {[lindex $_reponame end] eq {.git}} { @@ -1878,6 +1895,7 @@ proc incr_font_size {font {amt 1}} { set starting_gitk_msg [mc "Starting gitk... please wait..."] proc do_gitk {revs} { + global _gitworktree # -- Always start gitk through whatever we were loaded with. This # lets us bypass using shell process on Windows systems. # @@ -1895,8 +1913,10 @@ proc do_gitk {revs} { } set pwd [pwd] - cd [file dirname [gitdir]] - set env(GIT_DIR) [file tail [gitdir]] + if { $_gitworktree ne {} } { + cd $_gitworktree + } + set env(GIT_DIR) [file normalize [gitdir]] eval exec $cmd $revs & @@ -1915,6 +1935,7 @@ proc do_gitk {revs} { } proc do_explore {} { + global _gitworktree set explorer {} if {[is_Cygwin] || [is_Windows]} { set explorer "explorer.exe" @@ -1924,7 +1945,7 @@ proc do_explore {} { # freedesktop.org-conforming system is our best shot set explorer "xdg-open" } - eval exec $explorer [file dirname [gitdir]] & + eval exec $explorer $_gitworktree & } set is_quitting 0 @@ -2270,8 +2291,6 @@ proc show_less_context {} { ## ## ui construction -load_config 0 -apply_config set ui_comm {} # -- Menu Bar @@ -3302,7 +3321,7 @@ unset i set file_lists($ui_index) [list] set file_lists($ui_workdir) [list] -wm title . "[appname] ([reponame]) [file normalize [file dirname [gitdir]]]" +wm title . "[appname] ([reponame]) [file normalize $_gitworktree]" focus -force $ui_comm # -- Warn the user about environmental problems. Cygwin's Tcl
-- 1.6.2.rc1.179.g17e82
Johannes SchindelinRe: [PATCHv2 1/2] git-gui: handle non-standard worktree locations
Hi,
whenever I read "worktree" I get cautious.
_Very_ cautious.
I mean, _VERY_ cautious. And for a reason.
The whole idea of a worktree was bolted onto Git, so much so that almost invariably, every attempt at making it work properly, fails.
Honestly, I think it caused us more pain than any other part of Git, _including_ the nasty bugs in git_config_set().
On Thu, 19 Feb 2009, Giuseppe Bilotta wrote:
> Don't
Personally, I prefer a less colloquial (and while at it, a more concise) commit message.
> +# _gitdir exists, so try loading the config
> +load_config 0
> +apply_config
> +# try to set work tree from environment, falling back to core.worktree
> +if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
> + set _gitworktree [get_config core.worktree]
> +}
> if {$_prefix ne {}} {
> - regsub -all {[^/]+/} $_prefix ../ cdup
> + if {$_gitworktree eq {}} {
> + regsub -all {[^/]+/} $_prefix ../ cdup
> + } else {
> + set cdup $_gitworktree
> + }It appears as if you redo a the logic laid out in setup.c. Don't. Instead, teach rev-parse to output the path of the work tree.
Oh, wait, --show-cdup already shows that...
> @@ -1076,11 +1089,15 @@ if {$_prefix ne {}} {
> error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"]
> exit 1
> }
> - if {[catch {cd [file dirname $_gitdir]} err]} {
> + if {$_gitworktree eq {}} {
> + set _gitworktree [file dirname $_gitdir]
> + }This is certainly wrong in bare repositories.
(If that code is only exercized in non-bare repositories, you failed to indicate that. The commit message is the place for such comments.)
> + if {[catch {cd $_gitworktree} err]} {
> catch {wm withdraw .}
> - error_popup [strcat [mc "No working directory"] " [file dirname $_gitdir]:\n\n$err"]
> + error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
> exit 1
> }
> + set _gitworktree [pwd]And why do you have to cd to the worktree? You might not be able to come back to the GIT_DIR, after all.
> @@ -1895,8 +1913,10 @@ proc do_gitk {revs} {
> }
>
> set pwd [pwd]
> - cd [file dirname [gitdir]]
> - set env(GIT_DIR) [file tail [gitdir]]
> + if { $_gitworktree ne {} } {
> + cd $_gitworktree
> + }
> + set env(GIT_DIR) [file normalize [gitdir]]
>
> eval exec $cmd $revs &
> Just out of curiosity: what was your method of making sure that you catch all sites where the worktree is assumed to be $GIT_DIR/..?
That is a most crucial information for the commit message!!!
Besides, have you read about the fallout of making "git rev-parse --git-dir" showing absolute paths always?
If yes, why do you ignore it in your patch?
Ciao, Dscho
Giuseppe BilottaRe: [PATCHv2 1/2] git-gui: handle non-standard worktree locations
On Thu, Feb 19, 2009 at 3:22 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi, > > whenever I read "worktree" I get cautious. > > _Very_ cautious. > > > I mean, _VERY_ cautious. And for a reason. > > The whole idea of a worktree was bolted onto Git, so much so that almost > invariably, every attempt at making it work properly, fails. > > Honestly, I think it caused us more pain than any other part of Git, > _including_ the nasty bugs in git_config_set().
I will refer to this http://kerneltrap.org/mailarchive/git/2008/8/26/3077314 post from gitster, in particular, the last paragraph that reads:
""" I would strongly advocate for keeping the possibility of separating git-dir and work-tree, and possibly dropping the assumption that everything "foo.git" is a bare repo. There are config variables for this. The Tcl/Tk family I mentioned makes even stronger assumptions. I promise to have a look at these when I find time (oh yeah...). """
As it happens, gitk already works pretty fine with 'detatched' git trees, and git gui is the only tool that can't cope with them properly.
Also, as it happens, its inability with working with detatched git trees or other unusual setups is the ONLY thing preventing git gui from being usable in Zit, the git-based single-file content tracker I'm working on ( http://git.oblomov.eu/zit )
Since there is NO practical reason NOT to support these unusual configurations, and since the changes do NOT break standard usage, your personal dislike for abnormal worktree configurations is scarcely a meaning obstacle to patch inclusion.
(And please excuse the attitude, but yours is absolutely the worst I've ever seen on this mailing list, and yes it has been abundantly discussed.(
> On Thu, 19 Feb 2009, Giuseppe Bilotta wrote: > >> Don't > > Personally, I prefer a less colloquial (and while at it, a more concise) > commit message.
Whatever.
>> +# _gitdir exists, so try loading the config
>> +load_config 0
>> +apply_config
>> +# try to set work tree from environment, falling back to core.worktree
>> +if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
>> + set _gitworktree [get_config core.worktree]
>> +}
>> if {$_prefix ne {}} {
>> - regsub -all {[^/]+/} $_prefix ../ cdup
>> + if {$_gitworktree eq {}} {
>> + regsub -all {[^/]+/} $_prefix ../ cdup
>> + } else {
>> + set cdup $_gitworktree
>> + }
>
> It appears as if you redo a the logic laid out in setup.c. Don't.
> Instead, teach rev-parse to output the path of the work tree.
>
> Oh, wait, --show-cdup already shows that...As spearce itself remarked while reviewing the first round of this patchset, git-gui is currently backwards compatible as far as git 1.5.0. Introducing new features in future versions of git rev-parse is not going to help here anyway. (Also, I have no idea if this --show-cdup worked in 1.5.0 or not, I just took the existing code and adapted it to the possibility of gitworktree being already defined.)
>> @@ -1076,11 +1089,15 @@ if {$_prefix ne {}} {
>> error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"]
>> exit 1
>> }
>> - if {[catch {cd [file dirname $_gitdir]} err]} {
>> + if {$_gitworktree eq {}} {
>> + set _gitworktree [file dirname $_gitdir]
>> + }
>
> This is certainly wrong in bare repositories.It's also totally irrelevant and not less wrong than what the previous code did, since it used [file dirname $_gitdir] all across the code to do what I do with $_gitworktree now.
So the current code is correct in all the ways the old code was, plus in quite a few more ways where the previous code was wrong. And although there might be a couple of cases that the new approach doesn't fix, I'd rather prefer you pointed out which cases they where, how could they fail, and what possible ways you can suggest to work around them.
>> + if {[catch {cd $_gitworktree} err]} {
>> catch {wm withdraw .}
>> - error_popup [strcat [mc "No working directory"] " [file dirname $_gitdir]:\n\n$err"]
>> + error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
>> exit 1
>> }
>> + set _gitworktree [pwd]
>
> And why do you have to cd to the worktree?Because that's what the previous code did, and the intention of this patch is not to alter the logic of the code unless it's strictly necessary.
> You might not be able to come back to the GIT_DIR, after all.
How so?
>> @@ -1895,8 +1913,10 @@ proc do_gitk {revs} {
>> }
>>
>> set pwd [pwd]
>> - cd [file dirname [gitdir]]
>> - set env(GIT_DIR) [file tail [gitdir]]
>> + if { $_gitworktree ne {} } {
>> + cd $_gitworktree
>> + }
>> + set env(GIT_DIR) [file normalize [gitdir]]
>>
>> eval exec $cmd $revs &
>>
>
> Just out of curiosity: what was your method of making sure that you catch
> all sites where the worktree is assumed to be $GIT_DIR/..?I looked at the code, and tested as many features that interacted with the worktree as I could think of.
> Besides, have you read about the fallout of making "git rev-parse > --git-dir" showing absolute paths always? > > If yes, why do you ignore it in your patch?
Because I fail to see how this is even remotely relevant.
-- Giuseppe "Oblomov" Bilotta
Johannes SchindelinRe: [PATCHv2 1/2] git-gui: handle non-standard worktree locations
Hi,
On Fri, 20 Feb 2009, Giuseppe Bilotta wrote:
> Since there is NO practical reason NOT to support these unusual > configurations, and since the changes do NOT break standard usage, your > personal dislike for abnormal worktree configurations is scarcely a > meaning obstacle to patch inclusion.
It is not a personal dislike. It is based on experience.
> (And please excuse the attitude, but yours is absolutely the worst > I've ever seen on this mailing list, and yes it has been abundantly > discussed.(
And what exactly did you want to achieve with that comment?
Really... *sighs with a tired, crooked smile*
> >> +# _gitdir exists, so try loading the config
> >> +load_config 0
> >> +apply_config
> >> +# try to set work tree from environment, falling back to core.worktree
> >> +if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
> >> + set _gitworktree [get_config core.worktree]
> >> +}
> >> if {$_prefix ne {}} {
> >> - regsub -all {[^/]+/} $_prefix ../ cdup
> >> + if {$_gitworktree eq {}} {
> >> + regsub -all {[^/]+/} $_prefix ../ cdup
> >> + } else {
> >> + set cdup $_gitworktree
> >> + }
> >
> > It appears as if you redo a the logic laid out in setup.c. Don't.
> > Instead, teach rev-parse to output the path of the work tree.
> >
> > Oh, wait, --show-cdup already shows that...
>
> As spearce itself remarked while reviewing the first round of this
> patchset, git-gui is currently backwards compatible as far as git
> 1.5.0. Introducing new features in future versions of git rev-parse is
> not going to help here anyway. (Also, I have no idea if this
> --show-cdup worked in 1.5.0 or not, I just took the existing code and
> adapted it to the possibility of gitworktree being already defined.)Well, I actually looked. Not really far, just 1.4.0. It has --show-cdup. It does not have worktree.
> >> @@ -1076,11 +1089,15 @@ if {$_prefix ne {}} {
> >> error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"]
> >> exit 1
> >> }
> >> - if {[catch {cd [file dirname $_gitdir]} err]} {
> >> + if {$_gitworktree eq {}} {
> >> + set _gitworktree [file dirname $_gitdir]
> >> + }
> >
> > This is certainly wrong in bare repositories.
>
> It's also totally irrelevant and not less wrong than what the previous
> code did, since it used [file dirname $_gitdir] all across the code to
> do what I do with $_gitworktree now.
>
> So the current code is correct in all the ways the old code was, plus
> in quite a few more ways where the previous code was wrong. And
> although there might be a couple of cases that the new approach
> doesn't fix, I'd rather prefer you pointed out which cases they where,
> how could they fail, and what possible ways you can suggest to work
> around them.It would not take all that much effort to address my comment in terms of code.
Ciao, Dscho
Giuseppe BilottaRe: [PATCHv2 1/2] git-gui: handle non-standard worktree locations
On Fri, Feb 20, 2009 at 1:44 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi, > > On Fri, 20 Feb 2009, Giuseppe Bilotta wrote: > >> Since there is NO practical reason NOT to support these unusual >> configurations, and since the changes do NOT break standard usage, your >> personal dislike for abnormal worktree configurations is scarcely a >> meaning obstacle to patch inclusion. > > It is not a personal dislike. It is based on experience.
My experience is that tools that don't support nonstandard worktree configurations are either very old or plain broken, and in both cases they can and should be updated/fixed to cope with such configurations.
>> (And please excuse the attitude, but yours is absolutely the worst >> I've ever seen on this mailing list, and yes it has been abundantly >> discussed.( > > And what exactly did you want to achieve with that comment?
Nothing, but I still felt there was a need to clarify that this it not my typical attitude.
>> >> +# _gitdir exists, so try loading the config
>> >> +load_config 0
>> >> +apply_config
>> >> +# try to set work tree from environment, falling back to core.worktree
>> >> +if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
>> >> + set _gitworktree [get_config core.worktree]
>> >> +}
>> >> if {$_prefix ne {}} {
>> >> - regsub -all {[^/]+/} $_prefix ../ cdup
>> >> + if {$_gitworktree eq {}} {
>> >> + regsub -all {[^/]+/} $_prefix ../ cdup
>> >> + } else {
>> >> + set cdup $_gitworktree
>> >> + }
>> >
>> > It appears as if you redo a the logic laid out in setup.c. Don't.
>> > Instead, teach rev-parse to output the path of the work tree.
>> >
>> > Oh, wait, --show-cdup already shows that...
>>
>> As spearce itself remarked while reviewing the first round of this
>> patchset, git-gui is currently backwards compatible as far as git
>> 1.5.0. Introducing new features in future versions of git rev-parse is
>> not going to help here anyway. (Also, I have no idea if this
>> --show-cdup worked in 1.5.0 or not, I just took the existing code and
>> adapted it to the possibility of gitworktree being already defined.)
>
> Well, I actually looked. Not really far, just 1.4.0. It has --show-cdup.
> It does not have worktree.git blame puts it in 1.0.3, actually. Regardless, changes such as replacing the $prefix regexp with a show-cdup was not in the scope of this patch. More to the point, given the definition of $_prefix, the additional cost of the extra git-rev-parse call that would do exactly the same thing that we do now is totally not worth it.
>> >> @@ -1076,11 +1089,15 @@ if {$_prefix ne {}} {
>> >> error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"]
>> >> exit 1
>> >> }
>> >> - if {[catch {cd [file dirname $_gitdir]} err]} {
>> >> + if {$_gitworktree eq {}} {
>> >> + set _gitworktree [file dirname $_gitdir]
>> >> + }
>> >
>> > This is certainly wrong in bare repositories.
>>
>> It's also totally irrelevant and not less wrong than what the previous
>> code did, since it used [file dirname $_gitdir] all across the code to
>> do what I do with $_gitworktree now.
>>
>> So the current code is correct in all the ways the old code was, plus
>> in quite a few more ways where the previous code was wrong. And
>> although there might be a couple of cases that the new approach
>> doesn't fix, I'd rather prefer you pointed out which cases they where,
>> how could they fail, and what possible ways you can suggest to work
>> around them.
>
> It would not take all that much effort to address my comment in terms
> of code.Except for the fact that there was nothing to address.
-- Giuseppe "Oblomov" Bilotta
Giuseppe Bilotta[PATCHv2 2/2] git-gui: handle bare repos correctly
Refactor checking for a bare repository into its own proc, with a new logic such that the repository is considered bare if: * either the core.bare setting is true * or the worktree is not set and the directory name ends with .git The error message for the case of an unhandled bare repository is also updated to reflect the fact that the problem is not the funny name but the bareness.
The new refactored proc is also used to disable the menu entry to explore the working copy, and to skip changing to the worktree before the gitk invocation. ---
The only thing that I see no easy way to support is the "git --bare gui" syntax, which would probably require git setting up an appropriate environment variable.
git-gui/git-gui.sh | 34 +++++++++++++++++++++++++++------- 1 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh index 12c7ff3..a4c0d4b 100755 --- a/git-gui/git-gui.sh +++ b/git-gui/git-gui.sh @@ -122,6 +122,7 @@ unset oguimsg set _appname {Git Gui} set _gitdir {} set _gitworktree {} +set _isbare {} set _gitexec {} set _reponame {} set _iscygwin {} @@ -254,6 +255,23 @@ proc get_config {name} { } } +proc is_bare {} { + global _isbare + global _gitdir + global _gitworktree + + if {$_isbare eq {}} { + if {[is_config_true core.bare] + || ($_gitworktree eq {} + && [lindex [file split $_gitdir] end] ne {.git})} { + set _isbare 1 + } else { + set _isbare 0 + } + } + return $_isbare +} + ###################################################################### ## ## handy utils @@ -1084,9 +1102,9 @@ if {$_prefix ne {}} { set _gitworktree [pwd] unset cdup } elseif {![is_enabled bare]} { - if {[lindex [file split $_gitdir] end] ne {.git}} { + if {[is_bare]} { catch {wm withdraw .} - error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"] + error_popup [strcat [mc "Cannot use bare repository:"] "\n\n$_gitdir"] exit 1 } if {$_gitworktree eq {}} { @@ -1913,7 +1931,7 @@ proc do_gitk {revs} { } set pwd [pwd] - if { $_gitworktree ne {} } { + if { ![is_bare] } { cd $_gitworktree } set env(GIT_DIR) [file normalize [gitdir]] @@ -2317,10 +2335,12 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} { # menu .mbar.repository -.mbar.repository add command \ - -label [mc "Explore Working Copy"] \ - -command {do_explore} -.mbar.repository add separator +if {![is_bare]} { + .mbar.repository add command \ + -label [mc "Explore Working Copy"] \ + -command {do_explore} + .mbar.repository add separator +} .mbar.repository add command \ -label [mc "Browse Current Branch's Files"] \
-- 1.6.2.rc1.179.g17e82
Johannes SchindelinRe: [PATCHv2 2/2] git-gui: handle bare repos correctly
Hi,
On Thu, 19 Feb 2009, Giuseppe Bilotta wrote:
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 12c7ff3..a4c0d4b 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -122,6 +122,7 @@ unset oguimsg
> set _appname {Git Gui}
> set _gitdir {}
> set _gitworktree {}
> +set _isbare {}
> set _gitexec {}
> set _reponame {}
> set _iscygwin {}
> @@ -254,6 +255,23 @@ proc get_config {name} {
> }
> }
>
> +proc is_bare {} {
> + global _isbare
> + global _gitdir
> + global _gitworktree
> +
> + if {$_isbare eq {}} {
> + if {[is_config_true core.bare]
> + || ($_gitworktree eq {}
> + && [lindex [file split $_gitdir] end] ne {.git})} {
> + set _isbare 1
> + } else {
> + set _isbare 0
> + }
> + }
> + return $_isbare
> +}git rev-parse --is-bare-repository anyone?
> @@ -1913,7 +1931,7 @@ proc do_gitk {revs} {
> }
>
> set pwd [pwd]
> - if { $_gitworktree ne {} } {
> + if { ![is_bare] } {Why is this change needed at all?
> @@ -2317,10 +2335,12 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
> #
> menu .mbar.repository
>
> -.mbar.repository add command \
> - -label [mc "Explore Working Copy"] \
> - -command {do_explore}
> -.mbar.repository add separator
> +if {![is_bare]} {
> + .mbar.repository add command \
> + -label [mc "Explore Working Copy"] \
> + -command {do_explore}
> + .mbar.repository add separator
> +}How did you make sure that there are no more places?
(I, for one, would expect the standard mode of staging to fail in a bare repository.)
Ciao, Dscho
Giuseppe BilottaRe: [PATCHv2 2/2] git-gui: handle bare repos correctly
On Thu, Feb 19, 2009 at 3:25 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>>
>> +proc is_bare {} {
>> + global _isbare
>> + global _gitdir
>> + global _gitworktree
>> +
>> + if {$_isbare eq {}} {
>> + if {[is_config_true core.bare]
>> + || ($_gitworktree eq {}
>> + && [lindex [file split $_gitdir] end] ne {.git})} {
>> + set _isbare 1
>> + } else {
>> + set _isbare 0
>> + }
>> + }
>> + return $_isbare
>> +}
>
> git rev-parse --is-bare-repository anyone?As mentioned elsewhere, git-gui is supposed to be backwards compatible with git 1.5.0; --is-bare-repository was introduced in 1.5.2. What I can do is try that and fall back to this other implementation otherwise.
>> @@ -1913,7 +1931,7 @@ proc do_gitk {revs} {
>> }
>>
>> set pwd [pwd]
>> - if { $_gitworktree ne {} } {
>> + if { ![is_bare] } {
>
> Why is this change needed at all?Because the worktree being undefined in non-bare repositories is an error and should raise consequently.
>> @@ -2317,10 +2335,12 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
>> #
>> menu .mbar.repository
>>
>> -.mbar.repository add command \
>> - -label [mc "Explore Working Copy"] \
>> - -command {do_explore}
>> -.mbar.repository add separator
>> +if {![is_bare]} {
>> + .mbar.repository add command \
>> + -label [mc "Explore Working Copy"] \
>> + -command {do_explore}
>> + .mbar.repository add separator
>> +}
>
> How did you make sure that there are no more places?I looked at the code and tested it.
> (I, for one, would expect the standard mode of staging to fail in a bare > repository.)
That's the reason why the only thing you can do in bare repositories is git gui blame.
-- Giuseppe "Oblomov" Bilotta
Giuseppe Bilotta[PATCHv2bis 2/2] git-gui: handle bare repos correctly
Refactor checking for a bare repository into its own proc, that relies on git rev-parse --is-bare-repository if possible. If that fails (e.g. older versions of git) we fall back to a logic such that the repository is considered bare if: * either the core.bare setting is true * or the worktree is not set and the directory name ends with .git The error message for the case of an unhandled bare repository is also updated to reflect the fact that the problem is not the funny name but the bareness.
The new refactored proc is also used to disable the menu entry to explore the working copy, and to skip changing to the worktree before the gitk invocation. --- git-gui/git-gui.sh | 43 ++++++++++++++++++++++++++++++++++++------- 1 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh index 12c7ff3..e42d254 100755 --- a/git-gui/git-gui.sh +++ b/git-gui/git-gui.sh @@ -122,6 +122,7 @@ unset oguimsg set _appname {Git Gui} set _gitdir {} set _gitworktree {} +set _isbare {} set _gitexec {} set _reponame {} set _iscygwin {} @@ -254,6 +255,32 @@ proc get_config {name} { } } +proc is_bare {} { + global _isbare + global _gitdir + global _gitworktree + + if {$_isbare eq {}} { + if {[catch { + set _bare [git rev-parse --is-bare-repository] + switch -- $_bare { + true { set _isbare 1 } + false { set _isbare 0} + default { throw } + } + }]} { + if {[is_config_true core.bare] + || ($_gitworktree eq {} + && [lindex [file split $_gitdir] end] ne {.git})} { + set _isbare 1 + } else { + set _isbare 0 + } + } + } + return $_isbare +} + ###################################################################### ## ## handy utils @@ -1084,9 +1111,9 @@ if {$_prefix ne {}} { set _gitworktree [pwd] unset cdup } elseif {![is_enabled bare]} { - if {[lindex [file split $_gitdir] end] ne {.git}} { + if {[is_bare]} { catch {wm withdraw .} - error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"] + error_popup [strcat [mc "Cannot use bare repository:"] "\n\n$_gitdir"] exit 1 } if {$_gitworktree eq {}} { @@ -1913,7 +1940,7 @@ proc do_gitk {revs} { } set pwd [pwd] - if { $_gitworktree ne {} } { + if { ![is_bare] } { cd $_gitworktree } set env(GIT_DIR) [file normalize [gitdir]] @@ -2317,10 +2344,12 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} { # menu .mbar.repository -.mbar.repository add command \ - -label [mc "Explore Working Copy"] \ - -command {do_explore} -.mbar.repository add separator +if {![is_bare]} { + .mbar.repository add command \ + -label [mc "Explore Working Copy"] \ + -command {do_explore} + .mbar.repository add separator +} .mbar.repository add command \ -label [mc "Browse Current Branch's Files"] \
-- 1.6.2.rc1.179.g17e82