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

Re: [PATCH 03/26] t1400: Pass a legitimate <newvalue> to update command

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Mar 20, 2014, 17:01 UTC
Message-ID
<532B1ED1.3090208@alum.mit.edu>
In-Reply-To
<531F82FE.9030305@kitware.com>
On 03/11/2014 10:41 PM, Brad King wrote:
Show 21 quoted lines
> On Tue, Mar 11, 2014 at 4:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I may be misremembering things, but your first sentence quoted above
>> was exactly my reaction while reviewing the original change, and I
>> might have even raised that as an issue myself, saying something
>> like "consistency across values is more important than type-saving
>> in a machine format".
> 
> For reference, the original design discussion of the format was here:
> 
>  http://thread.gmane.org/gmane.comp.version-control.git/233842
> 
> I do not recall this issue being raised before, but now that it has
> been raised I fully agree:
> 
>  http://thread.gmane.org/gmane.comp.version-control.git/243754/focus=243862
> 
> In -z mode an empty <newvalue> should be treated as missing just as
> it is for <oldvalue>.  This is obvious now in hindsight and I wish I
> had realized this at the time.  Back then I went through a lot of
> iterations on the format and missed this simplification in the final
> version :(

It's not your fault; anybody could have reviewed your code at the time (I most of all, because I have been so active in this area of the code).

Show 23 quoted lines
> Moving forward:
> 
> The "create" command rejects a zero <newvalue> so the change in
> question for that command is merely the wording of the error message
> and there is no compatibility issue.
> 
> The "update" command supports a zero <newvalue> so that it can
> be used for all operations (create, update, delete, verify) with
> the proper combination of old and new values.  The change in question
> makes an empty <newvalue> an error where it was previously treated
> as zero.  (BTW, Michael, I do not see a test case for the new error
> in your series.  Something like the patch below should work.)
> 
>> I am not against deprecating and removing
>> the support for it in the longer term, though.
> 
> As I reported in my above-linked response, I'm not depending on
> the old behavior myself.  Also if one were to start seeing this
> error then generated input needs only trivial changes to avoid it.
> If we do want to preserve compatibility for others then perhaps an
> empty <newvalue> with -z should produce:
> 
>  warning: update $ref: missing <newvalue>, treating as zero

This last suggestion is what I am implementing for the re-roll (coming shortly). Thanks for the discussion.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Brad KingNext: Michael Haggerty
Message 10 of 38 in “Clean up update-refs --stdin and implement ref_transaction”
  1. 00/26 Clean up update-refs --stdin and implement ref_transactionMichael Haggerty, Mar 10, 2014
  2. 01/26 t1400: Fix name and expected result of one testMichael Haggerty, Mar 10, 2014
  3. 02/26 t1400: Provide sensible input to the commandMichael Haggerty, Mar 10, 2014
  4. 03/26 t1400: Pass a legitimate <newvalue> to update commandMichael Haggerty, Mar 10, 2014
  5. Brad KingMar 10, 2014
  6. Michael HaggertyMar 10, 2014
  7. Brad KingMar 11, 2014
  8. Junio C HamanoMar 11, 2014
  9. Brad KingMar 11, 2014
  10. Michael HaggertyMar 20, 2014
  11. 04/26 parse_arg(): Really test that argument is properly terminatedMichael Haggerty, Mar 10, 2014
  12. 05/26 t1400: Add some more tests involving quoted argumentsMichael Haggerty, Mar 10, 2014
  13. Johan HerlandMar 10, 2014
  14. 06/26 refs.h: Rename the action_on_err constantsMichael Haggerty, Mar 10, 2014
  15. 07/26 update_refs(): Fix constnessMichael Haggerty, Mar 10, 2014
  16. 08/26 update-ref --stdin: Read the whole input at onceMichael Haggerty, Mar 10, 2014
  17. 09/26 parse_cmd_verify(): Copy old_sha1 instead of evaluating <oldvalue> twiceMichael Haggerty, Mar 10, 2014
  18. 10/26 update-ref.c: Extract a new function, parse_refname()Michael Haggerty, Mar 10, 2014
  19. 11/26 update-ref --stdin: Improve error messages for invalid valuesMichael Haggerty, Mar 10, 2014
  20. 12/26 update-ref --stdin: Make error messages more consistentMichael Haggerty, Mar 10, 2014
  21. 13/26 update-ref --stdin: Simplify error messages for missing oldvaluesMichael Haggerty, Mar 10, 2014
  22. Brad KingMar 10, 2014
  23. Brad KingMar 10, 2014
  24. 14/26 update-ref.c: Extract a new function, parse_next_sha1()Michael Haggerty, Mar 10, 2014
  25. 15/26 update-ref --stdin: Improve the error message for unexpected EOFMichael Haggerty, Mar 10, 2014
  26. 16/26 update-ref --stdin: Harmonize error messagesMichael Haggerty, Mar 10, 2014
  27. 17/26 refs: Add a concept of a reference transactionMichael Haggerty, Mar 10, 2014
  28. 18/26 update-ref --stdin: Reimplement using reference transactionsMichael Haggerty, Mar 10, 2014
  29. 19/26 refs: Remove API function update_refs()Michael Haggerty, Mar 10, 2014
  30. 20/26 struct ref_update: Rename field "ref_name" to "refname"Michael Haggerty, Mar 10, 2014
  31. 21/26 struct ref_update: Store refname as a FLEX_ARRAY.Michael Haggerty, Mar 10, 2014
  32. 22/26 commit_ref_transaction(): Introduce temporary variablesMichael Haggerty, Mar 10, 2014
  33. 23/26 struct ref_update: Add a lock memberMichael Haggerty, Mar 10, 2014
  34. 24/26 struct ref_update: Add type fieldMichael Haggerty, Mar 10, 2014
  35. 25/26 commit_ref_transaction(): Also free the ref_transactionMichael Haggerty, Mar 10, 2014
  36. 26/26 commit_ref_transaction(): Work with transaction->updates in placeMichael Haggerty, Mar 10, 2014
  37. Brad KingMar 10, 2014
  38. Michael HaggertyMar 10, 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.