Re: [PATCH v4 5/8] worktree: add relative cli/config options to `add` command

From: Caleb White <cdwhite3@pm.me>
Date: Nov 20, 2024, 05:01 UTC
Message-ID: <D5QQQCUL0DR7.31L4W6C4NT6UU@pm.me>
In-Reply-To: <20fdb5ba-075f-48b5-9e92-f85378500710@gmail.com>

On Tue Nov 19, 2024 at 9:07 AM CST, Phillip Wood wrote:

Show 8 quoted lines

> Hi Caleb
>
> This looks good, I've left a couple of comments below - there are a few
> places where the tests could be cleaned up (I've only commented on the
> first instance of each cleanup) but the coverage looks good and the
> implementation looks good to me too. I'll leave it here for today, sorry
> it has taken me so long to get round to looking at these. I should be
> able to look at the rest of the patches this week.

No worries sir, thank you for taking the time to review. I greatly appreciate it. I'll start implementing the changes on my end, but I'll wait until you finish reviewing before sending v5.

Show 15 quoted lines

>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> index 70437c815f13852bd2eb862176b8b933e6de0acf..88d2261012166a929b7f167d79720e4d965fd71b 100644
>> --- a/Documentation/git-worktree.txt
>> +++ b/Documentation/git-worktree.txt
>> @@ -216,6 +216,10 @@ To remove a locked worktree, specify `--force` twice.
>>   This can also be set up as the default behaviour by using the
>>   `worktree.guessRemote` config option.
>>
>> +--[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.

Show 11 quoted lines

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

Show 14 quoted lines

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

Show 15 quoted lines

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

Show 17 quoted lines

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

Show 12 quoted lines

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