Re: [PATCH v2 2/5] worktree: add `write_worktree_linking_files` function

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 29, 2024, 22:55 UTC
Message-ID: <ZyFn9xErajxzQo29@nand.local>
In-Reply-To: <4a316a5f-9360-4f24-843f-bcbb5b3378c4@gmail.com>

On Tue, Oct 29, 2024 at 02:52:36PM +0000, Phillip Wood wrote:

Show 13 quoted lines

> Hi Caleb
>
> On 28/10/2024 19:09, Caleb White wrote:
> > A new helper function, `write_worktree_linking_files()`, centralizes
> > the logic for computing and writing either relative or absolute
> > paths, based on the provided configuration. This function accepts
> > `strbuf` pointers to both the worktree’s `.git` link and the
> > repository’s `gitdir`, and then writes the appropriate path to each.
>
> That sounds like a useful change. I think it would be better to pass an
> extra parameter "use_relative_paths" rather than relying on a global
> varibale in worktree.c. Thank you for adding some documentaion for the new
> function.

Good suggestion. I definitely agree that this is a useful direction.

Show 7 quoted lines

> > This also teachs `git worktree repair` to fix the linking files if
> > there is an absolute/relative paths but the links are correct. E.g.,
> > `git worktree repair` can be used to convert a valid worktree between
> > absolute and relative paths.
>
> This might be better as a separate step so that reviewers can concentrate on
> the correctness of write_werktree_linking_files() when reviewing this patch.

Indeed. This patch (even though the diffstat isn't overly large) is somewhat noisy just because of the number of spots that needed to be adjusted here.

I wonder if another way to split this up (in addition to what you wrote above) might be to introduce the new function and convert one single caller in the first patch. Then subsequent patches can go one callsite at a time and convert them to use the new function.

That way, each patch is easy-ish to verify in isolation. I know that results in some more patches, but I think that the additional clarity I imagine we'll get is worth doing so.

Thanks, Taylor