Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
> +void ref_transaction_create(struct ref_transaction *transaction,
> + const char *refname,
> + unsigned char *new_sha1,
> + int flags)
> +{
> + struct ref_update *update = add_update(transaction, refname);
> +
> + assert(!is_null_sha1(new_sha1));
> + hashcpy(update->new_sha1, new_sha1);
> + hashclr(update->old_sha1);
> + update->flags = flags;
> + update->have_old = 1;
> +}
> +
> +void ref_transaction_delete(struct ref_transaction *transaction,
> + const char *refname,
> + unsigned char *old_sha1,
> + int flags, int have_old)
> +{
> + struct ref_update *update = add_update(transaction, refname);
> +
> + update->flags = flags;
> + update->have_old = have_old;
> + if (have_old) {
> + assert(!is_null_sha1(old_sha1));
> + hashcpy(update->old_sha1, old_sha1);
> + }
> +}
These assert()s will often turn into no-op in production builds. If
it is a bug in the code (i.e. the callers are responsible for
catching these conditions and issuing errors, and there are actually
such checks implemented in the callers), it is fine to have these as
assert()s, but otherwise these should be 'if (...) die("BUG:")', I
think.
Other than that, I did not spot anything questionable in this round.
Thanks; will replace the series (but on the same base as I needed to
apply the series there to compare what got changed with the old
version of corresponding change for each patches).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html