Re: [PATCHv2 1/2] git-gui: handle non-standard worktree locations

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 20, 2009, 00:44 UTC
Message-ID: <alpine.DEB.1.00.0902200137390.10279@pacific.mpi-cbg.de>
In-Reply-To: <cb7bb73a0902191621x4e7a66bcj427095906e5a4078@mail.gmail.com>

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*

Show 26 quoted lines

> >> +# _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.

Show 21 quoted lines

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