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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
