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
