On Tue Oct 29, 2024 at 5:55 PM CDT, Taylor Blau wrote: > On Tue, Oct 29, 2024 at 02:52:36PM +0000, Phillip Wood wrote: >> 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. I didn't particularly care for the global variable either, but that was the easiest way to get up and running. I don't want to just pass this parameter around as that will result in changing about 6-7 function signatures. What are some other ways settings have been propagated into the internals? Right now, I'm thinking of just using a getter/setter in the worktree API, which would remove the global variable. >> 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. Sounds good, I'll split this up into multiple patches for each of the subcommands. I'll also add the tests for that subcommand in the same patch for context. Best, Caleb
