Re: [PATCH v5 0/8] Allow relative worktree linking to be configured by the user

From: Caleb White <cdwhite3@pm.me>
Date: Nov 26, 2024, 17:02 UTC
Message-ID: <D5W9TF80TCN0.19T8NJTHY4VGW@pm.me>
In-Reply-To: <xmqqv7wazfex.fsf@gitster.g>

On Tue Nov 26, 2024 at 12:18 AM CST, Junio C Hamano wrote:

Show 9 quoted lines

> Caleb White <cdwhite3@pm.me> writes:
>> Changes in v5:
>> - Added docs to `--relative-paths` option.
>
> You already had doc on this, but the default was not described at
> all.
>
>  --[no-]relative-paths::
> +       Link worktrees using relative paths or absolute paths (default).

I added a bit more explanation instead of just directing the user to the config variable (I originally had docs, but it was requested that I remove the duplication and just point to the config, however, I think the changes describes it a bit better as well as gives the default).

Show 5 quoted lines

>> - Added test coverage for `repair_worktrees()` and relative paths.
>> - Move `strbuf_reset` call in `infer_backlink()`.
>
> This was more like "revert the change in v4 that moved it
> unnecessarily", no?

Yes, that is correct.

>> - Cleaned up tests.
>
> Yup, there truely a lot of test changes between v4 and v5.  Many
> tests now use existing test helpers, which is good.

This is the majority of the reroll. It seems like MacOS doesn't like the `test_config` helper inside of a subshell, so I had to stick with `git config` in those cases.

Show 12 quoted lines

>> - Slight stylistic changes.
>
> I saw many changes like these (the diff is between v4 and v5)
>
>  static void repair_gitfile(struct worktree *wt,
> -                          worktree_repair_fn fn,
> -                          void *cb_data,
> +                          worktree_repair_fn fn, void *cb_data,
>                            int use_relative_paths)
>
> which looked good (the original had fn and cb_data defined on the
> same line).

Yes, this was brought up in the previous review and I decided to make the change.

Show 19 quoted lines

>> - Tweaked commit messages.
>
> Updates to the proposed log message for `repair` step [7/8] did not
> really "clarify", other than helping readers to see how messy things
> are.  It said:
>
>     +    To simplify things, both linking files are written when one of the files
>     +    needs to be repaired. In some cases, this fixes the other file before it
>     +    is checked, in other cases this results in a correct file being written
>     +    with the same contents.
>
> which may describe what the code happens to do correctly, but does
> not quite help building the confidence in what it does is correct.
>
> Suppose that the directory X has a repository, and the repository
> thinks that the directory W is its worktree.  But the worktree at
> the directory W thinks that its repository is not X but Y, and there
> indeed is a repository at the directory Y.  That repository thinks W
> belongs to it.

That's a bit of a confusing scenario, but I think this is what you're trying to describe:

    Repository X ----> Worktree W <---> Repository Y (Case 0)

which is not a normal case (but I'll get to that later). Most of the time, a repair would be performed with one of the following cases:

    Repository X <---> Worktree W (Case 1)
    Repository X ----> Worktree W (Case 2)
    Repository X <---- Worktree W (Case 3)
    Repository X       Worktree W (Case 4)

that is, a repository and worktree have valid links, have valid links in one direction or the other, or they have no valid links at all.

Before I go on, I think it would be helpful to revisit how the repair
operation works. There are two loops in the repair operation:
1. the `repair()` function iterates over (via `repair_worktree_at_path()`)
   the given worktrees/paths (or `.` if no paths are given) to potentially
   repair the `<repo>/.git/worktrees/<worktree_id>/gitdir` files
2. the `repair_worktrees()` function iterates over all the worktrees
   defined at `<repo>/.git/worktrees/*` to potentially repair the
   `.../<worktree_id>/.git` files

In Loop 1, a repair is performed if:
- there's an absolute/relative path mismatch
- the worktree `.git` file points to the repository, but the repository
  `gitdir` file is unreadable or does not point back to the worktree
- the worktree `.git` file does NOT point to the repository, but an
  inferred backlink can be established (the worktree id in the `.git`
  file matches a worktree id in the repository's `worktrees` directory),
  and that inferred repository does not point to the worktree

In Loop 2, a repair is performed if:
- there's an absolute/relative path mismatch
- the worktree pointed to by the repository `gitdir` file does not point
  back to the repository or the file is corrupted

Now back to Cases 1--4:
- In Case 1, the repair would not update any links (already valid).
- Case 2 is most likely when using absolute paths and the repository is
  moved, but the worktree is not. The worktree `.git` will be updated
  during Loop 2, however, now the repository `gitdir` file will also be
  written with the same contents (a no-op) to keep the code simple.
- Case 3 is most likely when using absolute paths and the worktree is
  moved, but the repository is not. The repository `gitdir` will be
  updated during Loop 1, however, now the worktree `.git` file will also
  be written with the same contents (a no-op) to keep the code simple.
- Case 4 can occur when using absolute paths and both the repository and
  worktree are moved, but it can also occur when using relative paths and
  either the repository or worktree is moved. Both linking files need to
  be updated. In the past, the repository `gitdir` file would be updated
  during Loop 1 (from the inferred backlink), and the worktree `.git`
  would not be updated until Loop 2. However, now both linking files are
  updated during Loop 1 and the repair is complete by the time Loop 2
  is reached.

Show 5 quoted lines

> If we examine X first, would we end up updating W to point at X
> (because X thinks W is its worktree)?
>
> Or do we make W to point at Y (because Y thinks W is its, and W
> thinks it is Y's)"?

A repair is always performed in the context of single repository, therefore, if operating on repository X and the worktree W is found to be a valid worktree for X, then yes, the repair would update the link from W to X so that Case 0 would now look like:

    Repository X <---> Worktree W <---- Repository Y

but again, this is a very weird case---the most likely scenario that I can think of is that a user copied a repository (with or without the worktree). The `es/worktree-repair-copied` topic added support for repairing a worktree from such a copy scenario. However, I did note[1,2] that the topic added the ability for a repository to "take over" a worktree from another repository if the worktree_id matched a worktree inside the current repository. This can happen if two repositories use the same worktree name (I usually name my worktrees the same name as the branch to keep things simple so this can happen if two repositories create a worktree for `master` for instance).

I recommended that worktrees be created with a unique hash/identifier so that the worktree_id is unique across all repositories even if they have the same name. I was planning on creating a future topic to address this, for example creating a worktree `develop` would look like:

    foo/
    ├── .git/worktrees/develop-6b3d7b/
    └── develop/

The actual worktree directory name would still be `develop`, but the worktree_id would be unique and prevent the "take over" scenario.

Show 8 quoted lines

> Either way, I think the comment is trying to say that, if we decide
> to make X and W belong to each other, we'd overwrite links from X to
> W and also W to X, even though the link from X was already pointing
> at W and the minimum fix we needed to make was to update the link
> from W to point at X.  Overwriting a link from X to W with a new
> link from X to W is a no-op, so it does not seem to help greatly,
> since `repair` is not at all performance critical.  The correctness
> is a lot more important.

Yes, you understand this correctly. The repair operation is not performance critical, so I decided to keep the code simple and just always update both linking files. The same `write_worktree_linking_files() is used for all operations (add, move, repair), some which require both files to be updated, and others which only require one file to be updated.

>> - Updated base to 090d24e9af.
>
> This made it harder than necessary to compare the two iterations, by
> the way.

My apologies for that. I wasn't sure what the procedure was when a dependent topic was merged to master. I figured it would be best to rebase onto the latest master.

Best,

Caleb

[1]: https://lore.kernel.org/git/20241008153035.71178-1-cdwhite3@pm.me/ [2]: https://lore.kernel.org/git/r4zmcET41Skr_FMop47AKd7cms9E8bKPSvHuAUpnYavzKEY6JybJta0_7GfuYB0q-gD-XNcvh5VDTfiT3qthGKjqhS1sbT4M2lUABynOz2Q=@pm.me/