Re: [PATCH v4 1/3] ref-filter: add worktreepath atom

From: Jeff King <peff@peff.net>
Date: Jan 3, 2019, 05:40 UTC
Message-ID: <20190103054043.GG20047@sigill.intra.peff.net>
In-Reply-To: <20181224084756.49952-2-nbelakovski@gmail.com>

On Mon, Dec 24, 2018 at 12:47:54AM -0800, nbelakovski@gmail.com wrote:

> [...]

Thanks for keeping with this. I think we're getting quite close, though I did find a few small-ish issues.

Show 6 quoted lines

> @@ -34,6 +36,8 @@ static struct ref_msg {
>  	"ahead %d, behind %d"
>  };
>  
> +static struct worktree **worktrees;
> +

Maybe define this near "struct hashmap ref_to_worktree_map" so it's more obvious that the two are related?

Show 8 quoted lines

> @@ -75,6 +79,11 @@ static struct expand_data {
>  	struct object_info info;
>  } oi, oi_deref;
>  
> +struct ref_to_worktree_entry {
> +    struct hashmap_entry ent; /* must be the first member! */
> +    struct worktree *wt; /* key is wt->head_ref */
> +};

Indent with spaces?

> -static int used_atom_cnt, need_tagged, need_symref;
> +static int used_atom_cnt, need_tagged, need_symref, has_worktree;
> +static struct hashmap ref_to_worktree_map;

Makes sense. I thought at first has_worktree was a flag that we might care about between parsing and formatting, but it's really just a flag to say "we lazy-loaded the worktree list".

Show 6 quoted lines

> +static int worktree_hashmap_cmpfnc(const void *unused_lookupdata, const void *existing_hashmap_entry_to_test,
> +				   const void *unused_key, const void *keydata_aka_refname)
> +{
> +	const struct ref_to_worktree_entry *e = existing_hashmap_entry_to_test;
> +	return strcmp(e->wt->head_ref, keydata_aka_refname);
> +}

So from the discussion in the cover letter, this needs to be more like:

  static int worktree_hashmap_cmpfnc(const void *unused_lookupdata,
                                     const void *ve1, const void *ve2,
				     const void *keydata_aka_refname)
  {
	const struct ref_to_worktree_entry *e1 = ve1, *e2 = ve2;
	return strcmp(e1->wt->head_ref, keydata_aka_refname ?
		                        keydata_aka_refname :
					e2->wt->head_ref);
  }

Show 8 quoted lines

> +static int worktree_atom_parser(const struct ref_format *format,
> +				struct used_atom *atom,
> +				const char *arg,
> +				struct strbuf *unused_err)
> +{
> +	int i;
> +	if (has_worktree)
> +		return 0;

Minor style nit, but please put a space between the declarations and the start of the code (not strictly necessary for a short function which has no other linebreaks, like the cmpfunc above, but here I think it's confusing not to).

Show 14 quoted lines

> +	worktrees = get_worktrees(0);
> +
> +	hashmap_init(&ref_to_worktree_map, worktree_hashmap_cmpfnc, NULL, 0);
> +
> +	for (i = 0; worktrees[i]; i++) {
> +		if (worktrees[i]->head_ref) {
> +			struct ref_to_worktree_entry *entry;
> +			entry = xmalloc(sizeof(*entry));
> +			entry->wt = worktrees[i];
> +			hashmap_entry_init(entry, strhash(worktrees[i]->head_ref));
> +
> +			hashmap_add(&ref_to_worktree_map, entry);
> +		}
> +	}

Makes sense to load the map.

Show 13 quoted lines

> +static const char *get_worktree_path(const struct used_atom *atom, const struct ref_array_item *ref)
> +{
> +	struct strbuf val = STRBUF_INIT;
> +	struct hashmap_entry entry;
> +	struct ref_to_worktree_entry *lookup_result;
> +
> +	hashmap_entry_init(&entry, strhash(ref->refname));
> +	lookup_result = hashmap_get(&ref_to_worktree_map, &entry, ref->refname);
> +
> +	strbuf_addstr(&val, lookup_result ? lookup_result->wt->path : "");
> +
> +	return strbuf_detach(&val, NULL);
> +}

And that makes sense to look up an item in it. Good.

Adding an empty string to a strbuf is a noop, so that part might more clearly be written as just:

  if (lookup_result)
	strbuf_addstr(&val, lookup_result->wt->path);

We return a "const char *" here, but the result is always allocated. Do we leak the result? Or should this return a "char *"?

I think there are a lot of other atoms that leak currently, but that is being fixed in another topic that is currently in pu.

Show 9 quoted lines

> @@ -2020,6 +2085,11 @@ void ref_array_clear(struct ref_array *array)
>  		free_array_item(array->items[i]);
>  	FREE_AND_NULL(array->items);
>  	array->nr = array->alloc = 0;
> +	if (has_worktree)
> +	{
> +		hashmap_free(&ref_to_worktree_map, 1);
> +		free_worktrees(worktrees);
> +	}

Here we free everything, but we don't unset has_worktree. So anybody trying to format more refs afterward would see our freed worktree list.

We probably want:

  has_worktree = 0;

here. Or simpler still, I think get_worktrees() will always return a non-NULL list (even if it is empty). So you could just drop has_worktree entirely, and use:

  if (worktrees)
	return; /* already loaded */;

in the loading function, and:

  free_worktrees(worktrees);
  worktrees = NULL;

here.

Show 11 quoted lines

> +test_expect_success '"add" a worktree' '
> +	mkdir worktree_dir &&
> +	git worktree add -b master_worktree worktree_dir master
> +'
> +
> +test_expect_success 'validate worktree atom' '
> +	{
> +	echo master: $PWD &&
> +	echo master_worktree: $PWD/worktree_dir &&
> +	echo side: not checked out
> +	} > expect &&

Minor style nit: use "} >expect" without the extra space.

This checks the actual directories. Good. I can never remember the rules for when to use $PWD versus $(pwd) on Windows. We may run afoul of the distinction here.

-Peff