Re: [PATCH v2 1/2] ref-filter: add worktree atom

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

nbelakovski@gmail.com writes:

>  
> +static int worktree_head_atom_parser(const struct ref_format *format,
> +									 struct used_atom *atom,
> +									 const char *arg,
> +									 struct strbuf *unused_err)

This and ...

> +{
> +	struct worktree **worktrees = get_worktrees(0);
> +	int i;
> +
> +	string_list_init(&atom->u.worktree_heads, 1);
> +
> +	for (i = 0; worktrees[i]; i++) {
> +		if (worktrees[i]->head_ref)
> +			string_list_append(&atom->u.worktree_heads,
> +							   worktrees[i]->head_ref);

... this makes me suspect that you are using tabstop != 8 and that
is causing you to indent these lines overly deeply.

Please don't, while working on this codebase.


> +	}
> +
> +	string_list_sort(&atom->u.worktree_heads);
> +
> +	free_worktrees(worktrees);
> +	return 0;
> +}

So..., this function collects any and all branches that are checked
out in some worktree, and sort them _without_ dedup.  The user of
the resulting information (i.e. atom->u.worktree_heads) cannot tell
where each of the listed branches is checked out.

I wonder if "The worktree at /local/src/wt1 has this branch checked
out" is something the user of %(worktree) atom, or a variant thereof
e.g. "%(worktree:detailed)", may want to learn, but because that
information is lost when this function returns, such an enhancement
cannot be done without fixing this funciton.

Also, I am not sure if this "list of some info on worktrees" really
belongs to an individual atom.  For one thing, if a format includes
more than one instance of %(worktree) atoms, you'd iterate over the
worktrees as many times as the number of these atoms you have.  Is
there another existing atom that "caches" expensive piece of
information per used_atom[] element like this one?  Essentially I am
trying to convince myself that the approach taken by the patch is a
sane one by finding a precedent.

> +		} else if (!strcmp(name, "worktree")) {
> +			if (string_list_has_string(&atom->u.worktree_heads, ref->refname))

I thought we were moving towards killing the use of string_list as a
look-up table, as we do not want to see thoughtless copy&paste such
a code from parts of the code that are not performance critical to a
part.  Not very satisfying.

	I think we can let this pass, and later add a wrapper around
	hashmap that is meant to only be used to replace string-list
	used for this exact purpose, i.e. key is a string, and there
	is no need to iterate over the existing elements in any
	sorted order.  Optionally, we can limit the look up to only
	checking for existence, if it makes the code for the wrapper
	simpler.

> +				v->s = xstrdup("+");
> +			else
> +				v->s = xstrdup(" ");
> +			continue;
>  		} else if (starts_with(name, "align")) {
>  			v->handler = align_atom_handler;
>  			v->s = xstrdup("");
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fc067ed672..5e6d249d4c 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -441,4 +441,19 @@ test_expect_success '--merged is incompatible with --no-merged' '
>  	test_must_fail git for-each-ref --merged HEAD --no-merged HEAD
>  '
>  
> +test_expect_success '"add" a worktree' '
> +	mkdir worktree_dir &&
> +	git worktree add -b master_worktree worktree_dir master
> +'
> +
> +test_expect_success 'validate worktree atom' '
> +	cat >expect <<-\EOF &&
> +	master: checked out in a worktree
> +	master_worktree: checked out in a worktree
> +	side: not checked out in a worktree

As you started the here-doc with <<-, the next line EOF does not
have to be flushed to the left.  Indent it just the same way with a
tab.

> +EOF

The following line begins with a broken indentation, it seems.

> +    git for-each-ref --format="%(refname:short): %(if)%(worktree)%(then)checked out in a worktree%(else)not checked out in a worktree%(end)" refs/heads/ >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done