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

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

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