git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 19/27] refs: add a concept of a reference transaction

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 7, 2014, 19:13 UTC
Message-ID
<xmqq8urhlzr1.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1396878498-19887-20-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 28 quoted lines
> +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).

Previous: Michael HaggertyNext: Michael Haggerty
Message 21 of 34 in “Clean up update-refs --stdin and implement ref_transaction”
  1. 00/27 Clean up update-refs --stdin and implement ref_transactionMichael Haggerty, Apr 7, 2014
  2. 01/27 t1400: fix name and expected result of one testMichael Haggerty, Apr 7, 2014
  3. 02/27 t1400: provide more usual input to the commandMichael Haggerty, Apr 7, 2014
  4. 03/27 parse_arg(): really test that argument is properly terminatedMichael Haggerty, Apr 7, 2014
  5. 04/27 t1400: add some more tests involving quoted argumentsMichael Haggerty, Apr 7, 2014
  6. 05/27 refs.h: rename the action_on_err constantsMichael Haggerty, Apr 7, 2014
  7. 06/27 update_refs(): fix constnessMichael Haggerty, Apr 7, 2014
  8. 07/27 update-ref --stdin: read the whole input at onceMichael Haggerty, Apr 7, 2014
  9. 08/27 parse_cmd_verify(): copy old_sha1 instead of evaluating <oldvalue> twiceMichael Haggerty, Apr 7, 2014
  10. 09/27 update-ref.c: extract a new function, parse_refname()Michael Haggerty, Apr 7, 2014
  11. 10/27 update-ref --stdin: improve error messages for invalid valuesMichael Haggerty, Apr 7, 2014
  12. 11/27 update-ref --stdin: make error messages more consistentMichael Haggerty, Apr 7, 2014
  13. 12/27 update-ref --stdin: simplify error messages for missing oldvaluesMichael Haggerty, Apr 7, 2014
  14. 13/27 t1400: test that stdin -z update treats empty <newvalue> as zerosMichael Haggerty, Apr 7, 2014
  15. 14/27 update-ref.c: extract a new function, parse_next_sha1()Michael Haggerty, Apr 7, 2014
  16. 15/27 update-ref --stdin -z: deprecate interpreting the empty string as zerosMichael Haggerty, Apr 7, 2014
  17. 16/27 t1400: test one mistake at a timeMichael Haggerty, Apr 7, 2014
  18. 17/27 update-ref --stdin: improve the error message for unexpected EOFMichael Haggerty, Apr 7, 2014
  19. 18/27 update-ref --stdin: harmonize error messagesMichael Haggerty, Apr 7, 2014
  20. 19/27 refs: add a concept of a reference transactionMichael Haggerty, Apr 7, 2014
  21. Junio C HamanoApr 7, 2014
  22. Michael HaggertyApr 14, 2014
  23. Junio C HamanoApr 14, 2014
  24. Michael HaggertyApr 15, 2014
  25. Junio C HamanoApr 15, 2014
  26. 20/27 update-ref --stdin: reimplement using reference transactionsMichael Haggerty, Apr 7, 2014
  27. 21/27 refs: remove API function update_refs()Michael Haggerty, Apr 7, 2014
  28. 22/27 struct ref_update: rename field "ref_name" to "refname"Michael Haggerty, Apr 7, 2014
  29. 23/27 struct ref_update: store refname as a FLEX_ARRAYMichael Haggerty, Apr 7, 2014
  30. 24/27 ref_transaction_commit(): simplify code using temporary variablesMichael Haggerty, Apr 7, 2014
  31. 25/27 struct ref_update: add a lock fieldMichael Haggerty, Apr 7, 2014
  32. 26/27 struct ref_update: add a type fieldMichael Haggerty, Apr 7, 2014
  33. 27/27 ref_transaction_commit(): work with transaction->updates in placeMichael Haggerty, Apr 7, 2014
  34. Ronnie SahlbergApr 11, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.