Re: [PATCH] refs: unify parse_worktree_ref() and ref_type()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2022, 15:43 UTC
Message-ID: <xmqqillrb7qs.fsf@gitster.g>
In-Reply-To: <xmqq35cwcpws.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 34 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Otherwise we fall through with worktree_ref that we have stripped
>> main-worktree/ prefix, which means the original input
>>
>> 	main-worktree/worktrees/foo/blah
>>
>> is now 
>>
>> 	worktrees/foo/blah
>>
>> and the next skip_prefix() will see that it begins with "worktrees/".
>> Of course, if the initial input were
>>
>> 	worktrees/foo/blah
>>
>> then we wouldn't have skipped main-worktree/ prefix from it, and go
>> to the next skip_prefix().  So from here on, we cannot tell which
>> case the original input was.
>>
>> But that is OK.  Asking "give me the ref 'blah' in the worktree 'foo'"
>> in the current worktree should yield the same answer to the question
>> "give me the ref 'blah' in the worktree 'foo', as if I asked you to
>> do so in the main worktree".
>
> This makes me wonder...
>
> I wonder if it makes the resulting code clearer to go fully
> recursive, unlike the posted code that says "if a recursive call
> says it is for current, that means it is for main worktree, and
> otherwise pretend as if the input did not have the prefix".
>
> That is, something like
> ...

The above may be a wrong suggestion, as it was solely guided by my reading of the posted code that looked as if it wanted to support something like "main-worktree/worktrees/foo/refs/heads/main". If that wasn't the intention, and we only want to support

  1. a ref spelled in the traditional way before multiple worktree feature,
  2. 1., with "main-worktree/" prefixed, or
  3. 1., with "worktrees/<worktreename>/" prefixed

then a better approach would be to have a small helper parse_local_worktree_ref() and make the primary one into something like

parse_worktree_ref()
{
        if (begins with "main-worktree/") {	
		strip main-worktree/ prefix;
		switch (parse_local_worktree_ref(input minus prefix)) {
		... the same switch to turn "ours" into "main's" ...
		... but we probably want to special case if we are ...
		... the main worktree ...
		}
	}
	else if (begins with "worktrees/") {
		strip worktrees/ prefix and learn worktree name;
		switch (parse_local_worktree_ref(input minus prefix)) {
		... the same switch to turn "ours" into "theirs" ...
		... but we probably want to special case if we are ...
		... the worktree in question ...
		}
	}
	return parse_local_worktree_ref(input);
}

and have parse_local_worktree_ref() handle psuedorefs, per-worktree-refs.