Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 6, 2017, 11:19 UTC
Message-ID: <CACsJy8BiRM5HE5vkt6pa+dsNmoPjEbRM+UTA3323kPe9rwu9VQ@mail.gmail.com>
In-Reply-To: <CAGZ79kY8cW+6+60bgSAhS69nBWZbn1LA7gT3t4Ddii+MB2V_cA@mail.gmail.com>

On Thu, Aug 24, 2017 at 2:54 AM, Stefan Beller <sbeller@google.com> wrote:

Show 17 quoted lines

>> +int other_head_refs(each_ref_fn fn, void *cb_data)
>> +{
>> +       struct worktree **worktrees, **p;
>> +       int ret = 0;
>> +
>> +       worktrees = get_worktrees(0);
>> +       for (p = worktrees; *p; p++) {
>> +               struct worktree *wt = *p;
>> +               struct ref_store *refs;
>> +
>> +               if (wt->is_current)
>> +                       continue;
>
> As said in an earlier patch (and now this pattern
> coming up twice in this patch series alone), the lines
> of this function up to here, could become
> part of a worktree iterator function?

There are a couple "loop through all worktrees" code so far, but the filter condition is not always this.

While I like the idea of iterator function (especially if it does "struct worktree" memory management internally), I think it's a bit overkill to go for_each_worktree() with a callback function when the equivalent for(;;) is so short. We would need to declare struct to pass callback data, and the reader may have to got read for_each_worktree() code then come back here.

So, probably no worktree iterator (yet).

Show 15 quoted lines

>> +               refs = get_worktree_ref_store(wt);
>> +               ret = refs_head_ref(refs, fn, cb_data);
>> +               if (ret)
>> +                       break;
>
> with these 4 lines in the callback function.
>
>> +/*
>> + * Similar to head_ref() for all HEADs _except_ one from the current
>> + * worktree, which is covered by head_ref().
>> + */
>> +int other_head_refs(each_ref_fn fn, void *cb_data);
>
> This is already such an iterator function, just at another
> abstraction level.

yeah.. but we can't mix and match (or combine) ref/worktree iterator callbacks

-- 
Duy