Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree

From: Anders Kaseorg <andersk@mit.edu>
Date: Nov 9, 2021, 01:10 UTC
Message-ID: <a25d105a-875b-fa6a-771a-37936779f067@mit.edu>
In-Reply-To: <xmqqzgqe448a.fsf@gitster.g>

On 11/8/21 15:28, Junio C Hamano wrote:

> My reading hiccupped after "at"; perhaps enclose the double-dot
> inside a pair of double quotes would make it easier to follow.

Will update.

Show 7 quoted lines

>> @@ -1456,11 +1456,11 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
>>   		work_tree = worktree->path;
>>   	else if (git_work_tree_cfg)
>>   		work_tree = git_work_tree_cfg;
> 
> Not a fault of this patch at all, but I am not sure if this existing
> bit of code is correct.

Perhaps this code is unreachable?

The worktree argument of update_worktree() should never be NULL, because we’d only have set do_update_worktree = 1 if find_shared_symref() returned non-NULL. And it looks to me like worktree->path should always be initialized to non-NULL, in either get_main_worktree() or get_linked_worktree()? I haven’t read enough of the code to be totally confident in this though.

> The callsite of the other function this patch modifies is in this
> update() function much later, and I think it should be updated to
> use the variable "worktree" instead of calling find_shared_symref()
> again with the same parameters.

Will update.

> We are not checking if we correctly update the working tree; we are
> only seeing "git push" succeeds.  Which might want to be tightened
> up.

Reasonable.  This is also the case with the existing test.

> It is a bit sad that these two tests are so inter-dependent.
> Depending on an earlier failure of other tests, this may fail in an
> unexpected way.

Yeah, I guess I wasn’t sure how much interdependence was allowed or expected. For example, the existing test already fails when run by itself (./t5516-fetch-push.sh --run=103) because the repository starts out empty. I’ll see what I can do, perhaps making use of the test_when_finished helper.

Anders