Re: [PATCH v2 06/20] checkout: check_linked_checkout: improve "already checked out" aesthetic

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 17, 2015, 00:32 UTC
Message-ID: <CAPig+cRtCon=jaqbjZyHTvJ3cydiyAz+5OC=3x30VfAJniYYMQ@mail.gmail.com>
In-Reply-To: <xmqqy4ig3s3q.fsf@gitster.dls.corp.google.com>

On Thu, Jul 16, 2015 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> How does this work with manually configured GIT_DIR environment, by
> the way?  I think GIT_DIR=/collection/of/repos/foo.git would be OK,
> as strbuf_strip_suffix() would hopefully leave it intact, but I am
> more interested in the general working of linked checkout feature,
> not just this error message.

I may be misunderstanding your question, but my answer is that strbuf_strip_suffix() is not applied to $GIT_DIR, but rather to the content of file $GIT_COMMON_DIR/worktrees/<tag>/gitdir, which is the path to the .git file in the linked worktree. That is, given:

    git worktree add ../foo HEAD

then the content of 'gitdir' is the absolute path of "../foo/.git", and strbuf_strip_suffix() operates on that value.

Show 13 quoted lines

> In the new world order with GIT_DIR and GIT_COMMON_DIR, does
> "$GIT_DIR" always have to be the same as "$GIT_WORK_TREE/.git"?  Do
> we need some sanity check if that is the case?  Perhaps: if you have
> $GIT_DIR set to $somewhere/.git/worktrees/$name, then
>
>  - $GIT_COMMON_DIR must match $somewhere/.git,
>
>  - $somewhere/.git/worktrees/$name/commondir must point at
>    $GIT_COMMON_DIR,
>
>  - $GIT_WORK_TREE/.git must match $GIT_DIR
>
> or something like that?

Duy is probably better suited to answer this, as he would likely have taken these issues into consideration when implementing the feature. (I've been poking through documentation and code for quite a while trying to answer this email but don't yet have a sufficient grasp to do it justice. I'm not even sure where such a sanity check would be placed.)

Show 7 quoted lines

>>       } else
>>               strbuf_addstr(&gitdir, get_git_common_dir());
>>       skip_prefix(branch, "refs/heads/", &branch);
>> +     strbuf_strip_suffix(&gitdir, "/.git");
>
> Sick people have '/.git' and run "git add etc/passwd"; do we want to
> consider such a use case?

I originally implemented this as stripping only ".git" since it felt natural for the result to have a trailing slash, however, when I looked back at your report[1], I saw that you suggested stripping "/.git", so I changed it to strip the slash as well. Given the above "sick" use-case, we may indeed want to strip only ".git".

[1]: http://article.gmane.org/gmane.comp.version-control.git/274001

>>       die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
>>  done:
>>       strbuf_release(&path);