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

[Shourya Shukla <shouryashukla.oo@gmail.com>]

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 706ac68751..65492752a7 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -67,7 +67,12 @@ static void delete_worktrees_dir_if_empty(void)
>  	rmdir(git_path("worktrees")); /* ignore failed removal */
>  }
>  
> -static int should_prune_worktree(const char *id, struct strbuf *reason)
> +/*
> + * 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
> + * cannot be determined. Caller is responsible for freeing returned path.
> + */
> +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
>  {
>  	struct stat st;
>  	char *path;
> @@ -75,6 +80,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
>  	size_t len;
>  	ssize_t read_result;
>  
> +	*wtpath = NULL;
>  	if (!is_directory(git_path("worktrees/%s", id))) {
>  		strbuf_addstr(reason, _("not a valid directory"));
>  		return 1;
> @@ -120,16 +126,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
>  	}
>  	path[len] = '\0';
>  	if (!file_exists(path)) {
> -		free(path);
>  		if (stat(git_path("worktrees/%s/index", id), &st) ||
>  		    st.st_mtime <= expire) {
>  			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
> +			free(path);
>  			return 1;
>  		} else {
> +			*wtpath = path;
>  			return 0;
>  		}
>  	}
> -	free(path);
> +	*wtpath = path;
>  	return 0;
>  }

What exactly is the role of 'wtpath' in here? Maybe this is explained in
the later patches. 

> @@ -141,22 +148,52 @@ static void prune_worktree(const char *id, const char *reason)
>  		delete_git_dir(id);
>  }
>  
> +static int prune_cmp(const void *a, const void *b)
> +{
> +	const struct string_list_item *x = a;
> +	const struct string_list_item *y = b;
> +	int c;
> +
> +	if ((c = fspathcmp(x->string, y->string)))
> +	    return c;
> +	/* paths same; sort by .git/worktrees/<id> */
> +	return strcmp(x->util, y->util);
> +}
> 

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'?

> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
> index b7d6d5d45a..fd3916fee0 100755
> --- a/t/t2401-worktree-prune.sh
> +++ b/t/t2401-worktree-prune.sh
> @@ -92,4 +92,16 @@ test_expect_success 'not prune proper checkouts' '
>  	test -d .git/worktrees/nop
>
>  
> +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?

Otherwise this looks good to me. But again, we need comments from others
too.

Nicely done! :)