On Tue Nov 19, 2024 at 11:54, Junio C Hamano <gitster@xxxxxxxxx> wrote: > "Bence Ferdinandy" <bence@xxxxxxxxxxxxxx> writes: > >> No, it is not, but it's also a mistake. It should be `updateres == 1`. >> refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other >> error currently. Before I touched the code it was 1 for any error, so I left >> that as is. So we want to error out on set_head if we get a 1 and continue if >> we get 0 or -1 (and handle the difference in the report_set_head_auto). >> >> Thanks for noticing, I'll get that fixed in v14. > > It is good that somebody noticed it (and it may have happened to be > me), but if it is a "mistake" as you said, I wonder why none of your > tests caught it. Do we have a gap in test coverage? > > Thanks. I've been looking into how I can force an error on remote set-head to test the error branch. For the files backend it seems to be pretty easy touch .git/refs/remotes/origin/HEAD.lock git remote set-head [something] rm .git/refs/remotes/origin/HEAD.lock My problem is that in this case, since I want to force an error on a user command, I don't see a way that is possible with git commands and I'm not sure how I could manipulate the reftable file in a way that can be reversed and only errors out the particular thing I need. I've been looking through the functions called here and it seems other errors all depend passing wrong parameters but that is not possible (and should not be possible) to trigger with remote set-head, so I think something "manual" needs to be done, like the above. Since this particular test just wants to test what happens if `refs_update_symref_extended` returns with 1 and not testing correct behaviour of backends and such, would it be acceptable if this particular test case would check for the backend and if it is reftables it will just pass without actually checking and do the manually locking thing above for the files backend? Thanks, Bence
