Re: [PATCH v4 7/8] worktree: add relative cli/config options to `repair` command
- From
- phillip.wood123@gmail.com <phillip.wood123@gmail.com>
- Date
- Nov 24, 2024, 19:27 UTC
- Message-ID
- <60325a17-82e3-4a4c-a6fd-d3b597f1c2bc@gmail.com>
- In-Reply-To
- <D5TBGJS8FWBE.3QYEZWZUS0C71@pm.me>
Hi Caleb
On 23/11/2024 05:41, Caleb White wrote:
Show 12 quoted lines
> On Fri Nov 22, 2024 at 9:55 AM CST, Phillip Wood wrote: >> On 01/11/2024 04:38, Caleb White wrote: >> 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).
That's fine - it might be worth explaining it in the commit message though (the same goes for the other patches where we start writing both files instead of one).
Show 15 quoted lines
>>> 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.Let's leave it as is and see what Junio thinks - my worry is that if strbuf_realpath() gets refactored in the future it could break this code.
Show 5 quoted lines
> 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.
Yes the end state is nice, I just found it a bit confusing getting there.
Best Wishes
Phillip
> Best, > > Caleb >