Re: [PATCH 13/33] peel_ref(): fix return value for non-peelable, not-current reference

2013-04-16 Thread Michael Haggerty 
On 04/15/2013 07:38 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 The old version was inconsistent: when a reference was
 REF_KNOWS_PEELED but with a null peeled value, it returned non-zero
 for the current reference but zero for other references.  Change the
 behavior for non-current references to match that of current_ref,
 which is what callers expect.  Document the behavior.

 Current callers did not trigger the previously-buggy behavior.
 
 Is that because we were lucky by codeflow, or is it just that we
 didn't have a testcase to trigger the behaviour?

Existing callers only called peel_ref() from within a for_each_ref-style
iteration and only for the current ref.  Therefore the buggy code path
was impossible to reach.

I will note that in the commit message.

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 13/33] peel_ref(): fix return value for non-peelable, not-current reference

2013-04-15 Thread Junio C Hamano 
Michael Haggerty mhag...@alum.mit.edu writes:

 The old version was inconsistent: when a reference was
 REF_KNOWS_PEELED but with a null peeled value, it returned non-zero
 for the current reference but zero for other references.  Change the
 behavior for non-current references to match that of current_ref,
 which is what callers expect.  Document the behavior.

 Current callers did not trigger the previously-buggy behavior.

Is that because we were lucky by codeflow, or is it just that we
didn't have a testcase to trigger the behaviour?


 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
  refs.c | 5 -
  refs.h | 8 
  2 files changed, 12 insertions(+), 1 deletion(-)

 diff --git a/refs.c b/refs.c
 index 84c2497..a0d8e32 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -120,7 +120,8 @@ struct ref_value {
   /*
* If REF_KNOWS_PEELED, then this field holds the peeled value
* of this reference, or null if the reference is known not to
 -  * be peelable.
 +  * be peelable.  See the documentation for peel_ref() for an
 +  * exact definition of peelable.
*/
   unsigned char peeled[20];
  };
 @@ -1339,6 +1340,8 @@ int peel_ref(const char *refname, unsigned char *sha1)
   struct ref_entry *r = get_packed_ref(refname);
  
   if (r  (r-flag  REF_KNOWS_PEELED)) {
 + if (is_null_sha1(r-u.value.peeled))
 + return -1;
   hashcpy(sha1, r-u.value.peeled);
   return 0;
   }
 diff --git a/refs.h b/refs.h
 index 17bc1c1..ac0ff92 100644
 --- a/refs.h
 +++ b/refs.h
 @@ -74,6 +74,14 @@ extern void add_packed_ref(const char *refname, const 
 unsigned char *sha1);
  
  extern int ref_exists(const char *);
  
 +/*
 + * If refname is a non-symbolic reference that refers to a tag object,
 + * and the tag can be (recursively) dereferenced to a non-tag object,
 + * store the SHA1 of the referred-to object to sha1 and return 0.  If
 + * any of these conditions are not met, return a non-zero value.
 + * Symbolic references are considered unpeelable, even if they
 + * ultimately resolve to a peelable tag.
 + */
  extern int peel_ref(const char *refname, unsigned char *sha1);
  
  /** Locks a refs/ ref returning the lock on success and NULL on failure. 
 **/
--
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 13/33] peel_ref(): fix return value for non-peelable, not-current reference

2013-04-14 Thread Michael Haggerty 
The old version was inconsistent: when a reference was
REF_KNOWS_PEELED but with a null peeled value, it returned non-zero
for the current reference but zero for other references.  Change the
behavior for non-current references to match that of current_ref,
which is what callers expect.  Document the behavior.

Current callers did not trigger the previously-buggy behavior.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 5 -
 refs.h | 8 
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 84c2497..a0d8e32 100644
--- a/refs.c
+++ b/refs.c
@@ -120,7 +120,8 @@ struct ref_value {
/*
 * If REF_KNOWS_PEELED, then this field holds the peeled value
 * of this reference, or null if the reference is known not to
-* be peelable.
+* be peelable.  See the documentation for peel_ref() for an
+* exact definition of peelable.
 */
unsigned char peeled[20];
 };
@@ -1339,6 +1340,8 @@ int peel_ref(const char *refname, unsigned char *sha1)
struct ref_entry *r = get_packed_ref(refname);
 
if (r  (r-flag  REF_KNOWS_PEELED)) {
+   if (is_null_sha1(r-u.value.peeled))
+   return -1;
hashcpy(sha1, r-u.value.peeled);
return 0;
}
diff --git a/refs.h b/refs.h
index 17bc1c1..ac0ff92 100644
--- a/refs.h
+++ b/refs.h
@@ -74,6 +74,14 @@ extern void add_packed_ref(const char *refname, const 
unsigned char *sha1);
 
 extern int ref_exists(const char *);
 
+/*
+ * If refname is a non-symbolic reference that refers to a tag object,
+ * and the tag can be (recursively) dereferenced to a non-tag object,
+ * store the SHA1 of the referred-to object to sha1 and return 0.  If
+ * any of these conditions are not met, return a non-zero value.
+ * Symbolic references are considered unpeelable, even if they
+ * ultimately resolve to a peelable tag.
+ */
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /** Locks a refs/ ref returning the lock on success and NULL on failure. **/
-- 
1.8.2.1

--
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