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

[Anders Kaseorg <andersk@mit.edu>]

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.

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