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

[Jeff King <peff@peff.net>]

On Mon, Nov 12, 2018 at 07:11:23PM +0900, Junio C Hamano wrote:

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

Hmm. I think for the purposes of this series we could jump straight to
converting %(worktree) to mean "the path of the worktree for which this
branch is HEAD, or the empty string otherwise".

Then the caller from git-branch (or anybody wanting to emulate it) could
still do:

  %(if)%(worktree)%(then)+ %(refname)%(end)

As a bonus, the decision to use "+" becomes a lot easier. It is no
longer a part of the format language that we must promise forever, but
simply a porcelain decision by git-branch.

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

Yes, we faced this a bit with Olga's cat-file conversion patches (where
we had a shared struct object_info). There probably should just be a
file-global data-structure storing the worktree info once (in an ideal
world, it would be part of a "struct ref_format" that uses no global
variables, but that is not how the code is structured today).

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

This came up over in another thread yesterday, too. So yeah, perhaps we
should move on that (I am OK punting on it for this series and
converting it later, though).

-Peff