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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2015, 18:10 UTC
Message-ID: <xmqqio72495x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442583027-47653-4-git-send-email-rappazzo@gmail.com>

Michael Rappazzo <rappazzo@gmail.com> writes:

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

static struct worktree *get_main_worktree(void)

Show 11 quoted lines

> +{
> +	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?

Show 10 quoted lines

> +/**
> + * 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).

Show 10 quoted lines

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

Show 17 quoted lines

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