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
Brad King <brad.king@kitware.com>
Date
Mar 11, 2014, 21:41 UTC
Message-ID
<531F82FE.9030305@kitware.com>
In-Reply-To
<xmqqa9cwpkiw.fsf@gitster.dls.corp.google.com>
On Tue, Mar 11, 2014 at 4:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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 :(

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
Then after a few releases it can be switched to an error.

Thanks, -Brad

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 3cc5c66..1e9fe7c 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,6 +730,12 @@ test_expect_success 'stdin -z fails update with bad ref name' '
 	grep "fatal: invalid ref format: ~a" err
 '

+test_expect_success 'stdin -z fails update with empty new value' '
+	printf $F "update $a" "" >stdin &&
+	test_must_fail git update-ref -z --stdin <stdin 2>err &&
+	grep "fatal: update $a: missing <newvalue>" err
+'
+
 test_expect_success 'stdin -z fails update with no new value' '
 	printf $F "update $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&

-- 
1.8.5.2
Previous: Junio C HamanoNext: Michael Haggerty
Message 9 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.