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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 10, 2014, 21:38 UTC
Message-ID: <531E30D7.40208@alum.mit.edu>
In-Reply-To: <531DF079.9050909@kitware.com>

Brad,

Thanks for your feedback.

On 03/10/2014 06:03 PM, Brad King wrote:

Show 25 quoted lines

> On 03/10/2014 08:46 AM, Michael Haggerty wrote:
>> This test is trying to test a few ways to delete references using "git
>> update-ref -z --stdin".  The third line passed in is
>>
>>     update SP /refs/heads/c NUL NUL <sha1> NUL
>>
>> , which is not a correct way to delete a reference according to the
>> documentation (the new value should be zeros, not empty).  Pass zeros
>> instead as the new value to test the code correctly.
> 
> In my original work on this feature, an empty <newvalue> is allowed.
> Since newvalue is not optional an empty value can be treated as zero.
> The relevant documentation is:
> 
>  update::
>          Set <ref> to <newvalue> after verifying <oldvalue>, if given.
>          Specify a zero <newvalue> to ensure the ref does not exist
> 
>  ...
> 
>  Use 40 "0" or the empty string to specify a zero value, except that
>  with `-z` an empty <oldvalue> is considered missing.
> 
> The two together say that <newvalue> can be the empty string instead
> of a literal zero.

OK, with your explanation and after reading the docs a couple more times, I can see your reading. Your rules as I now understand them:

* Without -z
  * 0{40} or the empty string represents zeros
  * No preceding SP delimiter indicates that the value is missing
    (as are any following values)

* With -z
  * For <newvalue>
    * 0{40} or the empty string represents zeros (the value is
      not allowed to be missing)
  * For <oldvalue>
    * 0{40} represents zeros
    * The empty string indicates that the value is missing

I implemented the slightly simpler rules

* Without -z
  * 0{40} or the empty string represents zeros
  * No preceding delimiter indicates that the value is missing (as
    are any following values)

* With -z
  * 0{40} represents zeros
  * The empty string indicates that the value is missing

It seems to me that "-z" input will nearly always be machine-generated, so there is not much reason to accept the empty string as shorthand for zeros. So I think that my version of the rules, being simpler to explain, is a slight improvement. But your version is already out in the wild, so backwards-compatibility is also a consideration, even though it is rather a fine point in a rather unlikely usage (why use update rather than delete to delete a reference?).

I don't know. I'm willing to rewrite the code to go back to your rules, or rewrite the documentation to describe my rules.

Neutral bystanders *cough*Junio*cough*, what do you prefer?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/