On Tue Nov 19, 2024 at 9:07 AM CST, Phillip Wood wrote:
> 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.
>> 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.
>> 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
