Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
On 03/11/2014 10:41 PM, Brad King wrote: On Tue, Mar 11, 2014 at 4:06 PM, Junio C Hamano gits...@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). 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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
On 03/10/2014 05:38 PM, Michael Haggerty wrote: 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. I agree. 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'm not using empty==zero with -z in any deployment. Since the feature is quite new, the behavior change is not silent, and it is easy to construct input that works with both versions, I do not think we need to worry about compatibility. or rewrite the documentation to describe my rules. I prefer this approach. Thanks, -Brad -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
Michael Haggerty mhag...@alum.mit.edu writes: 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? 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. Since nobody else were raising the issue back then, however, we are stuck with the interface. I am not against deprecating and removing the support for it in the longer term, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
On Tue, Mar 11, 2014 at 4:06 PM, Junio C Hamano gits...@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 :( 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 2err + 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 2err -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/26] t1400: Pass a legitimate newvalue to update command
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. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- t/t1400-update-ref.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 29391c6..e2f1dfa 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -927,7 +927,7 @@ test_expect_success 'stdin -z update refs fails with wrong old value' ' test_expect_success 'stdin -z delete refs works with packed and loose refs' ' git pack-refs --all git update-ref $c $m~1 - printf $F delete $a $m update $b $Z $m update $c $m~1 stdin + printf $F delete $a $m update $b $Z $m update $c $Z $m~1 stdin git update-ref -z --stdin stdin test_must_fail git rev-parse --verify -q $a test_must_fail git rev-parse --verify -q $b -- 1.9.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
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. -Brad -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
Brad,
Thanks for your feedback.
On 03/10/2014 06:03 PM, Brad King wrote:
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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
