Re: [PATCH 1/2 v4] worktree: add 'for_each_worktree' function

From: Mike Rappazzo <rappazzo@gmail.com>
Date: Aug 13, 2015, 23:32 UTC
Message-ID: <CANoM8SUGHzH-YjB9yK-juoX0S9zyUk=QhCK5JUqiQiqwOwwqJg@mail.gmail.com>
In-Reply-To: <1439493795.8855.16.camel@twopensource.com>

I will reroll this series with adjustments as you suggested, and I will add the extra tests for bare repos.

On Thu, Aug 13, 2015 at 3:23 PM, David Turner <dturner@twopensource.com> wrote:

> The scope of d can be smaller; move it down to the place I've marked
> below

I have adjusted the scoping. I misunderstood the meaning of some comments from the v3 patch, but your statements have helped me understand.

>
> strbuf_strip_suffix returns 1 if the suffix was stripped and 0
> otherwise, so there is no need for this strcmp.

Done.

Show 5 quoted lines

>
> I'm a little worried about this path manipulation; it looks like the
> config setting core.bare is the actual thing to check?  (Along with
> maybe the GIT_WORK_TREE environment var; not sure how that interacts
> with worktrees).

As Junio pointed out in a previous version of this patch, the core.bare will always be 'true' since they share the config. He also suggested that this could be the cause for concern. Therefore, I can use core.bare to check if the main is a bare repo. I guess that with the inclusion of tests using a bare repo, that will catch it if things change in the future.

Show 22 quoted lines

>
>> +     }
>> +
>> +     if (!main_is_bare) {
>> +             strbuf_addstr(&worktree_path, main_path.buf);
>> +
>> +             strbuf_addstr(&main_path, "/.git");
>> +             strbuf_addstr(&worktree_git, main_path.buf);
>> +
>> +             ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
>> +             if (ret)
>> +                     goto done;
>> +     }
>> +     strbuf_addstr(&main_path, "/worktrees");
>
> Earlier, you said:
>                 strbuf_addstr(&main_path, common_dir);
>                 strbuf_strip_suffix(&main_path, "/.git");
>
> Now you are adding /worktrees.  Doesn't this mean, in the non-bare case,
> that you'll be looking in $common_dir/worktrees instead of
> $common_dir/.git/worktrees ?

I read the gitdir file from the common dir.

Show 10 quoted lines

>> +                     while ((d = readdir(dir)) != NULL) {
>> +                             if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>> +                                     continue;
>> +
>> +                             strbuf_reset(&worktree_git);
>> +                             strbuf_addf(&worktree_git, "%s/%s/gitdir", main_path.buf, d->d_name);
>
> tioli: main_path never changes, so no need to keep chopping it off and
> adding it back on; just truncate worktree_git to main_path.len + 1 here
> and then add d->name.

I will try to clean it up a bit. I am not very experienced with C, but I will do my best.