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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2015, 18:05 UTC
Message-ID: <xmqqpp2c4l5u.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1440280294-50679-3-git-send-email-rappazzo@gmail.com>

Michael Rappazzo <rappazzo@gmail.com> writes:

Show 11 quoted lines

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

Show 10 quoted lines

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