On Fri Nov 22, 2024 at 9:55 AM CST, Phillip Wood wrote:
> 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().
I'll add a test case for this.
>> 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.
Ah, good to know---I'm used to every argument being on its own line but
I can revert.
>> + 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.
Yes, both are either 0 or 1, so this should be safe.
>> 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.
Yes, there is an edge case that a file is written with the same (correct)
contents, but I think this is acceptable given that it would be more
complicated to check if the contents are the same before writing (which
would involve reading the 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.
I'd like to keep this if that's okay. All of the strbuf tweaking was
so that way there is a consistency the variable names across the
implementations---all the calls to `write_worktree_linking_files()` use
`dotgit` and `gitdir` as the strubuf names so it should be easier to
follow now.
Best,
Caleb
