Re: [PATCH v3] worktree: add 'list' command

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2015, 22:10 UTC
Message-ID: <xmqqr3nau74h.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1439239982-42826-2-git-send-email-rappazzo@gmail.com>

Michael Rappazzo <rappazzo@gmail.com> writes:

Show 7 quoted lines

> +static int list(int ac, const char **av, const char *prefix)
> +{
> +	int main_only = 0;
> +	struct option options[] = {
> +		OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")),
> +		OPT_END()
> +	};

Hmm, main-only is still there?

Show 7 quoted lines

> +
> +	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +	if (ac)
> +		usage_with_options(worktree_usage, options);
> +
> +	struct strbuf main_path = STRBUF_INIT;
> +	const char* common_dir = get_git_common_dir();

Asterisks stick to the variable names, not types.

> +	int is_bare = is_bare_repository();

Please do not introduce decl-after-stmt.

> +	if (is_bare) {
> +		strbuf_addstr(&main_path, absolute_path(common_dir));

Hmm, interesting.

Because .git/config is shared, core.bare read from that tells us if the "main" one is bare, even if you start this command from one of its linked worktrees. So in that sense, this test of is_bare correctly tells if "main" one is a bare repository.

But that by itself feels wrong. Doesn't the presense of a working tree mean that you should not get "is_bare==true" in such a case (i.e. your "main" one is bare, you are in a linked worktree of it that has the index and the working tree)?

Duy?  Eric?  What do you guys think?

There are many codepaths that change their behaviour (e.g. if we create reflogs by default) based on the return value of is_bare_repository(). If I am reading this correctly, I _think_ a new working area that was prepared with "git worktree add" out of a bare repository would not work well, as these operations behave as if we do not have a working tree. Perhaps is_bare_repository() in such a working area _should_ say "No", even if core.bare in the shared bare one is set to true.

> +		strbuf_strip_suffix(&main_path, "/.");

In any case, what is that stripping of "/." about? Who is adding that extra trailing string?

What I am getting at is (1) perhaps it shouldn't be adding that in the first place, and (2) if some other code is randomly adding "/." at the end, what guarantees you that you would need to strip it only once here---if the other code added that twice, don't you have to repeatedly remove "/." from the end?