From: phillip.wood123@gmail.com
Date: Sun, 24 Nov 2024 19:27:32 GMT
Subject: Re: [PATCH v4 7/8] worktree: add relative cli/config options to `repair` command
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:
> 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).

>>>    	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.

> 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
>