From: phillip.wood123@gmail.com
Date: Fri, 22 Nov 2024 16:44:35 GMT
Subject: Re: [PATCH v4 5/8] worktree: add relative cli/config options to `add` command
Message-ID: <7ddddf98-840d-4d36-b3bb-6d5265a1362f@gmail.com>
In-Reply-To: <D5QQQCUL0DR7.31L4W6C4NT6UU@pm.me>

Hi Caleb

On 20/11/2024 05:01, Caleb White wrote:
> On Tue Nov 19, 2024 at 9:07 AM CST, Phillip Wood wrote:
> 
>>> +--[no-]relative-paths::
>>> +	Overrides the `worktree.useRelativePaths` config option, see
>>> +	linkgit:git-config[1].
>>
>> I think it would be helpful to describe what this option does directly,
>> rather than in terms of a config setting which is documented elsewhere
>> and that may not be set
> 
> I originally had a description here, but it was requested by another
> reviewer that I remove the duplicate information and simply reference
> the config option. I don't mind which direction we go, but we need
> some consensus on this.

Oh right, from a user's point of view describing an option in terms of a 
config option that may not be set seems a bit confusing. Looking at the 
man page we describe --[no-]guess-remote and then mention the config 
option rather than just referring the reader to the config man page.

Best Wishes

Phillip

>>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>>> index dae63dedf4cac2621f51f95a39aa456b33acd894..e3b4a71ee0bc13d5e817cf7dcc398e9e2bd975de 100644
>>> --- a/builtin/worktree.c
>>> +++ b/builtin/worktree.c
>>> @@ -120,12 +120,14 @@ struct add_opts {
>>>    	int quiet;
>>>    	int checkout;
>>>    	int orphan;
>>> +	int relative_paths;
>>
>> Excellent - thanks for getting rind of the global in /worktree.c
> 
> Thanks! I'm happy with how this turned out.
> 
>>> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
>>> index cfc4aeb1798c6d023909cec771e5b74e983af5ea..d36d8a4db0e924877787697579544f10f92dc0cf 100755
>>> --- a/t/t2400-worktree-add.sh
>>> +++ b/t/t2400-worktree-add.sh
>>> @@ -1207,4 +1207,58 @@ test_expect_success '"add" with initialized submodule, with submodule.recurse se
>>>    	git -C project-clone -c submodule.recurse worktree add ../project-5
>>>    '
>>>
>>> +test_expect_success 'can create worktrees with relative paths' '
>>> +	test_when_finished "git worktree remove relative" &&
>>> +	git config worktree.useRelativePaths false &&
>>
>> Using test_config instead of "git config" ensures the setting is cleared
>> at the end of the test
> 
> Ah, good to know! I'll update.
> 
>>> +	git worktree add --relative-paths ./relative &&
>>> +	cat relative/.git >actual &&
>>
>> There's no need to copy the file just so we can say "actual" below
> 
> Good point. I saw this pattern in other tests and thought it was
> standard practice. I'll update.
> 
>>> +test_expect_success 'can create worktrees with absolute paths' '
>>> +	git config worktree.useRelativePaths true &&
>>> +	git worktree add ./relative &&
>>> +	cat relative/.git >actual &&
>>> +	echo "gitdir: ../.git/worktrees/relative" >expect &&
>>> +	test_cmp expect actual &&
>>> +	git worktree add --no-relative-paths ./absolute &&
>>> +	cat absolute/.git >actual &&
>>> +	echo "gitdir: $(pwd)/.git/worktrees/absolute" >expect &&
>>> +	test_cmp expect actual
>>> +'
>>
>> We're only checking ".git" here and not the "gitdir" file as well, I
>> guess that's ok as the implemntation that writes the files is tested
>> above and here we're interesting in testing the config setting.
> 
> I can just add a test for the other file to be complete.
> 
>>> +test_expect_success 'move repo without breaking relative internal links' '
>>> +	test_when_finished rm -rf repo moved &&
>>> +	git init repo &&
>>> +	(
>>> +		cd repo &&
>>> +		git config worktree.useRelativePaths true &&
>>> +		test_commit initial &&
>>> +		git worktree add wt1 &&
>>> +		cd .. &&
>>> +		mv repo moved &&
>>> +		cd moved/wt1 &&
>>> +		git status >out 2>err &&
>>> +		test_must_be_empty err
>>
>> This relies on "git status" failing if the worktree links are broken. A
>> more direct test might be to check the output of "git worktree list" or
>> "git rev-parse --git-dir".
> 
> All three commands would have the same error message, but I suppose I
> could switch to `git worktree list` for a more direct test.
> 
>>> +test_expect_success 'relative worktree sets extension config' '
>>> +	test_when_finished "rm -rf repo" &&
>>> +	git init repo &&
>>> +	git -C repo commit --allow-empty -m base &&
>>> +	git -C repo worktree add --relative-paths ./foo &&
>>> +	git -C repo config get core.repositoryformatversion >actual &&
>>> +	echo 1 >expected &&
>>> +	test_cmp expected actual &&
>>
>> This can be done with
>>
>> 	test_cmp_config -C repo 1 core.repositoryformatversion
> 
> Sweet! I like it.
> 
> 
> Best,
> 
> Caleb
> 
>