From: Anders Kaseorg Date: Tue, 09 Nov 2021 01:10:43 GMT Subject: Re: [PATCH v3 2/2] receive-pack: Protect current branch for bare repository worktree Message-ID: In-Reply-To: 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