Re: [PATCH v5 2/2] worktree: add 'list' command

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

Michael Rappazzo <rappazzo@gmail.com> writes:

> +static int print_worktree_details(const char *path, const char *git_dir, void *cb_data)
> +{
> +	struct strbuf head_file = STRBUF_INIT;
> +	struct strbuf head_ref = STRBUF_INIT;
> +	struct stat st;
> +	struct list_opts *opts = cb_data;
> +	const char *ref_prefix = "ref: refs/heads/";
> +
> +	strbuf_addf(&head_file, "%s/HEAD", git_dir);
> +	if (!opts->path_only && !stat(head_file.buf, &st)) {
> +		strbuf_read_file(&head_ref, head_file.buf, st.st_size);

This does not work for traditional "symlinked HEAD", no?

I'd prefer to see the callback functions of for_each_worktree() not
to duplicate the logic we already have elsewhere in the system.
Such an incomplete attempt to duplication (as we see here) will lead
to unnecessary bugs (as we see here).

Conceptually, for-each-worktree should give us the worktree root
(i.e. equivalent to $GIT_WORK_TREE in the pre-multi-worktree world)
and the git directory (i.e. equivalent to $GIT_DIR in the
pre-multi-worktree world), and the callbacks should be able to do an
equivalent of

    system("git --work-tree=$GIT_WORK_TREE --git-dir=$GIT_DIR cmd")

where in this particular case "cmd" may be "symbolic-ref HEAD" or
something, no?

> +		strbuf_strip_suffix(&head_ref, "\n");
> +
> +		if (starts_with(head_ref.buf, ref_prefix)) {
> +			/* branch checked out */
> +			strbuf_remove(&head_ref, 0, strlen(ref_prefix));
> +		/* } else {
> +		 *  headless -- no-op
> +		 */
> +		}
> +		printf("%s  (%s)\n", path, head_ref.buf);

Is this new command meant to be a Porcelain?  This would not work as
a plumbing that produces a machine-parseable stable output.

I am not saying that it _should_; I do not know if we even need a
'list' command that is driven from an end-user script that gives
anything more than "where the work trees are".

My inclination is to suggest dropping the "which branch" code
altogether and only give "path_only" behaviour.

Thanks.