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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2020, 17:34 UTC
Message-ID: <xmqqa71ag6zb.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cQjR78+wanyRXhB33qsVwHqjSAc_c1O+CG6ZkJi6W3mDA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 11 quoted lines

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

"wt" being a fairly common abbreviation of "work(ing) tree" in the codebase may escape new readers of the code. The comment before the function can explicitly mention the variable name in the description to help them, I would think. For example ...

> +/*
> + * Return true if worktree entry should be pruned, along with the reason for
> + * pruning. Otherwise, return false and the worktree's path, or NULL if it

... the first sentence makes it clear that the function returns two things (i.e. "true"---presumably is returned as its return value, and "the reason"---the readers are supposed to guess it is stuffed in the strbuf), and the second sentence also says the function returns two things (i.e. "false", and "the worktree's path"---it is not immediately obvious where NULL goes, though).

> + * cannot be determined. Caller is responsible for freeing returned path.
> + */

	Determine if the worktree entry specified by its "id" should
	be pruned.

	When returning 'true', the caller-supplied strbuf "reason"
	is filled with the reason why it should be pruned.  "wtpath"
	is left intact.

	When returning 'false', the string variable pointed by
	"wtpath" receives the absolute path of the worktree; or NULL
	if the location of the worktree cannot be determined.
	"reason" is left intact.

perhaps? I didn't check what you do to *wtpath when you return true, so "left intact" may not be what you are doing, and I am *not* suggesting to leave it intact in that case---I am only suggesting that we should describe what happens.

Show 25 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_)

While that is true, what happens in the funcion is a bit unusual.

> +static int prune_cmp(const void *a, const void *b)
> +{
> +	const struct string_list_item *x = a;
> +	const struct string_list_item *y = b;

Usually we do

	static int foo_cmp(const void *a_, const void *b_)
	{
		const true_type *a = a_;
		const true_type *b = b_;

		/* use a and b for comparison */

without involving 'x' and 'y'.

Show 7 quoted lines

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

I myself write "rm -rf" more often when I am casually programming out of inertia, but when consciously making a decision on how to spell the combination, I tend to go alphabetical, because there is no logical reason to write 'r' first, other than the fact that "-rf" visually looks prettier than "-fr".

If recursiveness of the removal is more important than forcedness, then favouring "-rf" over "-fr" would be justifiable, but I do not think that is the case here.

And as you said, it is not something that justifies a reroll (or updating existing uses of "-rf") alone.