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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 10, 2020, 15:21 UTC
Message-ID: <CAPig+cQjR78+wanyRXhB33qsVwHqjSAc_c1O+CG6ZkJi6W3mDA@mail.gmail.com>
In-Reply-To: <20200610115028.GA11750@konoha>

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.

Show 6 quoted lines

> > +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.