Re: [PATCH v3] worktree: add 'list' command

From: Mike Rappazzo <rappazzo@gmail.com>
Date: Aug 11, 2015, 11:41 UTC
Message-ID: <CANoM8SWeqxD2vWLQmEfxxxn8Dz4yPfjGOoOH=Azn1A3So+wz2Q@mail.gmail.com>
In-Reply-To: <xmqqr3nau74h.fsf@gitster.dls.corp.google.com>

On Mon, Aug 10, 2015 at 6:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>> +static int list(int ac, const char **av, const char *prefix)
>> +{
>> +     int main_only = 0;
>> +     struct option options[] = {
>> +             OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
>> +             OPT_END()
>> +     };
>
> Hmm, main-only is still there?

Sorry, I missed that.

>
>> +     int is_bare = is_bare_repository();
>
> Please do not introduce decl-after-stmt.

Since I reused this value below, I thought it would be acceptable.

Show 14 quoted lines

>> +     if (is_bare) {
>> +             strbuf_addstr(&main_path, absolute_path(common_dir));
>
> Hmm, interesting.
>
> Because .git/config is shared, core.bare read from that tells us if
> the "main" one is bare, even if you start this command from one of
> its linked worktrees.  So in that sense, this test of is_bare
> correctly tells if "main" one is a bare repository.
>
> But that by itself feels wrong.  Doesn't the presense of a working
> tree mean that you should not get "is_bare==true" in such a case
> (i.e. your "main" one is bare, you are in a linked worktree of it
> that has the index and the working tree)?

Is is even correct for a bare repo to be included in the list? I think that is part of what you are asking here.

Show 11 quoted lines

>
>> +             strbuf_strip_suffix(&main_path, "/.");
>
> In any case, what is that stripping of "/." about?  Who is adding
> that extra trailing string?
>
> What I am getting at is (1) perhaps it shouldn't be adding that in
> the first place, and (2) if some other code is randomly adding "/."
> at the end, what guarantees you that you would need to strip it only
> once here---if the other code added that twice, don't you have to
> repeatedly remove "/." from the end?

In the case of a common dir, the value returned is just '.'. I wanted to resolve that to the full path, so I called `absolute_path(common_dir)`. Hence the trailing "/.". Similarly, in the main repo, the common dir is just ".git", and I handled that by using `get_git_work_tree()`.