Re: [PATCH] builtin/worktree: support relative paths for worktrees

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2024, 01:35 UTC
Message-ID: <xmqqikupbxh5.fsf@gitster.g>
In-Reply-To: <pull.1783.git.git.1726880551891.gitgitgadget@gmail.com>

"Francesco Guastella via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> From: Francesco Guastella <guastella.francesco@gmail.com>
>
> Add a new configuration option, `worktree.useRelativePaths`, which allows
> users to choose between storing relative or absolute paths in the files
> Git uses to maintain links between the main worktree and any additional
> ones.

I may be misremembering things, but wasn't the use of absolute paths a concious design decision?

When the source repository and an attached worktree know each other with relative location, moving merely one of them would make it impossible to locate the other.

On the other hand, if they know the other peer's absolute location, at least the one that was moved would still be able to locate the one that did not move, which means that it is possible to find from the one that moved the other one that did not move, and teach the latter where the new location of the moved one is.

The only case where it may be an improvement to have them refer to each other with relative locations is when both of them move in unison without breaking their relative location.

There may be problems other than the design choice I mentioned above in the resulting code after applying this huge patch, but as it stands, the patch does a bit too many things at once to be sensibly reviewable. I cannot comment much on the implementation (at least in this message) in its current form.

For example, the refactoring of t/t2400 into t/lib-worktree might be an excellent thing to do, but it looks like that the change to the tests alone deserves to be split into at least a few patches (one to refactor the test script without changing any functionality, and one or more patches to add or improve test helpers, and another that comes with code and behaviour change that add tests for the new behaviour when configured, or something like that).

I might be tempted to come back to it later, but style violations in the t/lib-worktree.sh were annoying enough to discourage me from reviewing it (if it followed Documentation/CodingGuidelines, it wouldn't have repelled my eyes).

Thanks.