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

> On 01/11/2024 04:38, Caleb White wrote:
>> +test_expect_success 'repair relative worktree to use absolute paths' '
>> +	test_when_finished "rm -rf main side sidemoved" &&
>> +	test_create_repo main &&
>> +	test_commit -C main init &&
>> +	git -C main worktree add --relative-paths --detach ../side &&
>> +	echo "$(pwd)/sidemoved/.git" >expect-gitdir &&
>> +	echo "gitdir: $(pwd)/main/.git/worktrees/side" >expect-gitfile &&
>> +	mv side sidemoved &&
>> +	git -C main worktree repair ../sidemoved &&
>> +	test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
>> +	test_cmp expect-gitfile sidemoved/.git
>> +'
>
> These tests looks sensibile, we should probably check that "git worktree
> repair" repects worktree.userelativepaths. I wonder if we have any
> coverage of repair_worktrees() as I think in these tests the problem is
> fixed by repair_worktree_at_path() before we call repair_worktrees().

>>   test_done
>> diff --git a/worktree.c b/worktree.c
>> index 6b640cd9549ecb060236f7eddf1390caa181f1a0..2cb994ac462debf966ac51b5a4f33c30cfebd4ef 100644
>> --- a/worktree.c
>> +++ b/worktree.c
>> @@ -574,12 +574,14 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
>>    * pointing at <repo>/worktrees/<id>.
>>    */
>>   static void repair_gitfile(struct worktree *wt,
>> -			   worktree_repair_fn fn, void *cb_data)
>> +			   worktree_repair_fn fn,
>> +			   void *cb_data,
>
> Style wise leaving "fn" and "cb_data" on the same line would be fine.
> That applies to all the functions.

>> +	else if (use_relative_paths == is_absolute_path(dotgit_contents))
>> +		repair = _(".git file absolute/relative path mismatch");
>
> Comparing ints as booleans makes me nervous in case we have a non-zero
> value that isn't 1 but is_absolute_path() returns 0 or 1 and we know
> use_relative_paths is 0 or 1.

>>   	if (repair) {
>>   		fn(0, wt->path, repair, cb_data);
>> -		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
>> +		write_worktree_linking_files(dotgit, gitdir, use_relative_paths);
>
> We used to update only the ".git", now we'll update both. In the case
> where we're changing to/from absolute/relative paths that's good because
> we'll update the "gitdir" file as well. In the other cases it looks like
> we've we've found this worktree via the "gitdir" file so it should be
> safe to write the same value back to that file.

>> [...]
>>   void repair_worktree_at_path(const char *path,
>> -			     worktree_repair_fn fn, void *cb_data)
>> +			     worktree_repair_fn fn,
>> +			     void *cb_data,
>> +			     int use_relative_paths)
>>   {
>>   	struct strbuf dotgit = STRBUF_INIT;
>> -	struct strbuf realdotgit = STRBUF_INIT;
>>   	struct strbuf backlink = STRBUF_INIT;
>>   	struct strbuf inferred_backlink = STRBUF_INIT;
>>   	struct strbuf gitdir = STRBUF_INIT;
>>   	struct strbuf olddotgit = STRBUF_INIT;
>> -	struct strbuf realolddotgit = STRBUF_INIT;
>> -	struct strbuf tmp = STRBUF_INIT;
>>
>>   	char *dotgit_contents = NULL;
>>   	const char *repair = NULL;
>>   	int err;
>> @@ -779,25 +783,25 @@ void repair_worktree_at_path(const char *path,
>>   		goto done;
>>
>>   	strbuf_addf(&dotgit, "%s/.git", path);
>> -	if (!strbuf_realpath(&realdotgit, dotgit.buf, 0)) {
>> +	if (!strbuf_realpath(&dotgit, dotgit.buf, 0)) {
>
> This works because strbuf_realpath() copies dotgit.buf before it resets
> dotgit but that does not seem to be documented and looking at the output of
>
>      git grep strbuf_realpath | grep \\.buf
>
> I don't see any other callers relying on this outside of your earlier
> changes to this file. Given that I wonder if we should leave it as is
> which would also simplify this patch as the interesting changes are
> swamped by the strbuf tweaking.