Re: [PATCH v3 19/27] refs: add a concept of a reference transaction
Michael Haggerty mhag...@alum.mit.edu writes: In retrospect, you might have been objecting more to the misleading docstring than to the behavior as implemented at the time. Yeah, I was reacting to the comment that said create can delete ;-) The docstring implied that create could actually be used to delete a reference, but that was not true: it always checked that the reference didn't exist beforehand. So at worst it could leave a non-existent reference un-created. OK, all is clear then. (2) keep the assert(), reject such user input at the callers, and document that these are invalid inputs; The current status in v3 is that (2) is implemented. Good. Thanks. -- 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 v3 19/27] refs: add a concept of a reference transaction
On 04/07/2014 09:13 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
+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.
I forgot to confirm that the callers *do* verify that they don't pass
incorrect values to ref_transaction_create() and
ref_transaction_delete(). But if they wouldn't, then die(BUG:) would
not be appropriate either, would it? It would have to be die(you silly
user...).
Another reason I'm comfortable with only having assert()s in this case
is that even if the preconditions are not met, nothing really crazy
happens. If I were guarding against something nastier, like a buffer
overflow, then I would more likely have used die(BUG:) instead.
It is not material to this discussion, but I wonder how often production
builds use NDEBUG. I just checked that Debian does not; i.e.,
assertions are enabled for git there. Personally I wouldn't run code
built with NDEBUG unless building for a severely resource-constrained
device, and even then I'd be pretty nervous about it.
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 v3 19/27] refs: add a concept of a reference transaction
Michael Haggerty mhag...@alum.mit.edu writes:
I forgot to confirm that the callers *do* verify that they don't pass
incorrect values to ref_transaction_create() and
ref_transaction_delete(). But if they wouldn't, then die(BUG:) would
not be appropriate either, would it? It would have to be die(you silly
user...).
Having the assert() there gives a confused message to the readers
(at least it did to this reader).
assert() implies that callers that are not buggy should not give
input that triggers the condition, which would mean it is the
callers' responsibility to sanity check the user-input to reject
creating a ref at 0{40} or deleting a ref whose old value is 0{40},
which in turn means these input are errors that need to be diagnosed
and documented.
But as you said below...
... even if the preconditions are not met, nothing really crazy
happens.
I agree that it also is perfectly fine to treat such input as
not-an-input-error.
It is a signal that these checks are not 'if (...) die()' that the
code may take that stance.
I cannot tell which interpretation the code is trying to implement.
Any one of the following I can understand:
(1) drop the assert(), declaring that the user input is perfectly
fine;
(2) keep the assert(), reject such user input at the callers, and
document that these are invalid inputs;
(3) replace the assert() with 'if (...) die(you silly user...)',
and document that these are invalid inputs; or
(4) same as (3) but replace with warn(), declaring such input as
suspicious.
but the current assert() makes the code look cannot decide ;-).
I would consider the first two more sensible than the other two, and
am leaning slightly towards (1) over (2).
Thanks.
--
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 v3 19/27] refs: add a concept of a reference transaction
On 04/14/2014 11:25 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
I forgot to confirm that the callers *do* verify that they don't pass
incorrect values to ref_transaction_create() and
ref_transaction_delete(). But if they wouldn't, then die(BUG:) would
not be appropriate either, would it? It would have to be die(you silly
user...).
Having the assert() there gives a confused message to the readers
(at least it did to this reader).
assert() implies that callers that are not buggy should not give
input that triggers the condition, which would mean it is the
callers' responsibility to sanity check the user-input to reject
creating a ref at 0{40} or deleting a ref whose old value is 0{40},
which in turn means these input are errors that need to be diagnosed
and documented.
In v2 of this patch series, I didn't forbid calling create with new_sha
== 0{40}, because, even though it's not what the user would think of as
creating a reference, the code didn't care--it would just verify that
the reference didn't exist before and then leave it undefined.
Then in [1] you commented:
Sounds a bit crazy that you can ask create, which verifies the
absense of the thing, to delete a thing.
I reacted to your comment by changing the documentation to forbid
calling create with new_sha1 == 0{40}, and enforced the rule using an
assert(). At the same time I added an analogous restriction that if
delete is called with have_old set, then old_sha1 must not be 0{40}.
In retrospect, you might have been objecting more to the misleading
docstring than to the behavior as implemented at the time. The
docstring implied that create could actually be used to delete a
reference, but that was not true: it always checked that the reference
didn't exist beforehand. So at worst it could leave a non-existent
reference un-created. Sorry for the confusion.
But as you said below...
... even if the preconditions are not met, nothing really crazy
happens.
I agree that it also is perfectly fine to treat such input as
not-an-input-error.
That was the case in v2.
It is a signal that these checks are not 'if (...) die()' that the
code may take that stance.
I cannot tell which interpretation the code is trying to implement.
Any one of the following I can understand:
(1) drop the assert(), declaring that the user input is perfectly
fine;
(2) keep the assert(), reject such user input at the callers, and
document that these are invalid inputs;
(3) replace the assert() with 'if (...) die(you silly user...)',
and document that these are invalid inputs; or
(4) same as (3) but replace with warn(), declaring such input as
suspicious.
but the current assert() makes the code look cannot decide ;-).
I would consider the first two more sensible than the other two, and
am leaning slightly towards (1) over (2).
The current status in v3 is that (2) is implemented.
Obviously I don't feel strongly about it, given that I implemented (1)
in v2. But I think that this restriction makes code at the calling site
easier to understand: create now always means create; i.e., if the
transaction goes through, then the reference exists afterwards. And
delete always means delete; i.e., afterwards, there is one less
reference in the world.
Michael
[1] http://mid.gmane.org/xmqqtxaczvod@gitster.dls.corp.google.com
--
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 v3 19/27] refs: add a concept of a reference transaction
Michael Haggerty mhag...@alum.mit.edu writes:
+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).
--
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
