Re: [PATCH v8 3/4] worktree: add functions to get worktree details

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

Michael Rappazzo <rappazzo@gmail.com> writes:

> +/**
> + * get the main worktree
> + */
> +static struct worktree *get_main_worktree()

static struct worktree *get_main_worktree(void)

> +{
> +	struct worktree *worktree = NULL;
>  	struct strbuf path = STRBUF_INIT;
> +	struct strbuf worktree_path = STRBUF_INIT;
> +	struct strbuf git_dir = STRBUF_INIT;
> +	struct strbuf head_ref = STRBUF_INIT;
> +	int is_bare = 0;
> +	int is_detached = 0;
>  
> +	strbuf_addf(&git_dir, "%s", absolute_path(get_git_common_dir()));
> +	strbuf_addf(&worktree_path, "%s", absolute_path(get_git_common_dir()));

Why not strbuf_add(&git_dir, absolute_path(get_git_common_dir()))
followed by strbuf_addbuf(&git_dir, &worktree_path)?

> +	is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> +	if (is_bare)
> +		strbuf_strip_suffix(&worktree_path, "/.");

Hmm, it is not immediately obvious which part is meant to be a
faithful translations from the old find_main_symref() that grab the
same kind of information the old code used to discover (and record
in a new mechanism that is "struct worktree"), and which part is a
new code that is needed to grab new pieces of information.  The
effort in [PATCH 2/4] to make it easier to follow sort of lost by
this rewrite.

I presume that discovery of the git_dir is from the original and
everything else including is_bare bit, head_ref, is_detached are
new?

Splitting this patch further into two may help showing the changes
better.  First move to the "grab information into an element of the
worktree array" code structure without adding new information, and
then "now make the worktree structure richer" as a follow up,
perhaps?

> +/**
> + * get the estimated worktree count for use in sizing the worktree array
> + * Note that the minimum count should be 2 (main worktree + NULL).  Using the
> + * file count in $GIT_COMMON_DIR/worktrees includes '.' and '..' so the
> + * minimum is satisfied by counting those entries.
> + */
> +static int get_estimated_worktree_count()
> +{
> +...
>  }

This is new.  My gut-feeling is that we would probably not want to
do this (see below).

> +struct worktree **get_worktrees(void)
>  {
> +	struct worktree **list = NULL;
>  	struct strbuf path = STRBUF_INIT;
>  	DIR *dir;
>  	struct dirent *d;
> -	char *existing;
> +	int counter = 0;
> +
> +	list = xmalloc(get_estimated_worktree_count() * sizeof(struct worktree *));

Here you scanned the directory to see how many possible worktrees
there are "at this moment".

Time passes.

How do you know that nobody added more worktrees in the meantime?
You don't.

>  
> -	if ((existing = find_main_symref(symref, target)))
> -		return existing;
> +	if ((list[counter] = get_main_worktree()))
> +		counter++;
>  
>  	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
>  	dir = opendir(path.buf);
>  	strbuf_release(&path);
> +	if (dir) {
> +		while ((d = readdir(dir)) != NULL) {
> +			struct worktree *linked = NULL;
> +			if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +				continue;
> +
> +			if ((linked = get_linked_worktree(d->d_name))) {
> +				list[counter++] = linked;

Which means that nothing stops you from overrunning the list[] with
this iteration.

By the way, when the body of "if" and "else" both consists of a
single statement, we tend to drop braces.

> +			}

And in order to avoid overrunning, you would need to do the usual
ALLOC_GROW() dance on list[] _anyway_.  So there is no much point,
other than to optimize for a case where you have thousands of
worktrees and the usual ALLOC_GROW() approach would have to do
realloc(3) too many times, to have the initial "guestimate" scan.