Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jun 10, 2020 at 7:50 AM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> > +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
>
> What exactly is the role of 'wtpath' in here? Maybe this is explained in
> the later patches.

'wtpath' holds the location of the worktree. It's used in this patch
by prune_worktrees() to collect a list of paths which haven't been
marked for pruning. Once it has the full list, it passes it to
prune_dups() for pruning duplicate entries.

> > +static int prune_cmp(const void *a, const void *b)
>
> Can we rename the function arguments better? 'a' and 'b' sound very
> naive to me. Maybe change these to something more like: 'firstwt' and
> 'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or
> like 'wt' and 'wt_dup'?

As with any language, C has its idioms, as does Git itself. Sticking
to idioms makes it easier for others to understand the code
at-a-glance. Short variable names, such as "i" and "j" for indexes,
"n" for counters, "s" and "t" for strings, are very common among
experienced programmers. Likewise, "a" and "b" are well-understood as
the arguments of a "comparison" function. There are many such examples
in the Git source code itself. Here are just a few:

    cmp_uint32(const void *a_, const void *b_)
    maildir_filename_cmp(const char *a, const char *b)
    tipcmp(const void *a_, const void *b_)
    cmp_by_tag_and_age(const void *a_, const void *b_)
    cmp_remaining_objects(const void *a, const void *b)
    version_cmp(const char *a, const char *b)
    diffnamecmp(const void *a_, const void *b_)
    spanhash_cmp(const void *a_, const void *b_)
    void_hashcmp(const void *a, const void *b)
    pathspec_item_cmp(const void *a_, const void *b_)

> > +test_expect_success 'prune duplicate (linked/linked)' '
> > +   test_when_finished rm -fr .git/worktrees w1 w2 &&
>
> Nit: maybe make it 'rm -rf' as that's the popular way of doing it?

It's true that "-rf" has wider usage in Git tests than "-fr", though
the latter is heavily used, as well. Ordinarily, matching the style
already present in a particular script would be a good way to choose
between the two, however, the worktree-related test scripts which this
series touches already have a mix of the two. This is the first use of
combined "-r" and "-f" in this particular script, so I could go with
the more common "-rf", however, it's not worth re-rolling just for
that.

Thanks for the review.