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

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

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