Patrick Steinhardt <ps@xxxxxx> writes: >> > Now there is a very particular edge case in this situation: when >> > preparing an empty ref transacton, we end up returning whatever value >> > `read_ref_without_reload()` returned to the caller. Under normal >> > conditions this would be fine: "HEAD" should usually exist, and thus the >> > function would return `0`. But if "HEAD" doesn't exist, the function >> > returns a positive value which we end up returning to the caller. >> > >> > Fix this bug by resetting the return code to `0` and add a test. So this _will_ surface as a bug when the other change in the series is applied, but it nevertheless is worth fixing independently of the other one, because ... >> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, >> > &head_referent, &head_type); >> > if (ret < 0) >> > goto done; >> > + ret = 0; ... after "if the refs->err records an error already, skip everything we do and return to the caller", we should take the ownership of what we return (which will be in "ret") from now on. So the current code uses "ret" as an uninitialized variable, even not technically so because it is "initialized" to "refs->err" upfront, and this is like a fix of uninitialized variable use. >> So this is not really a problem in this function, it's more of that >> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero. > > Well, yes. I'd claim that it is a problem in this function because it > returns positive even though the transaction was prepared successfully. > >> Nit: would be nice to have a comment about why overriding this value is >> ok. > > True. Yup. It seems we will see a v2 for updating the test code as well, so I'll assume that you'd explain this as an independent fix (as well as a required preliminary fix for the other one). Thanks, both.
