Re: [PATCH 15/33] refs: change the internal reference-iteration API

2013-04-16 Thread Michael Haggerty
On 04/15/2013 07:38 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 [...]  But more
 importantly, this change prevents peel_ref() from returning invalid
 results in the following scenario:

 When iterating via the external API, the iteration always includes
 both packed and loose references, and in particular never presents a
 packed ref if there is a loose ref with the same name.  The internal
 API, on the other hand, gives the option to iterate over only the
 packed references.  During such an iteration, there is no check
 whether the packed ref might be hidden by a loose ref of the same
 name.  But until now the packed ref was recorded in current_ref during
 the iteration.  So if peel_ref() were called with the reference name
 corresponding to current ref, it would return the peeled version of
 the packed ref even though there might be a loose ref that peels to a
 different value.  This scenario doesn't currently occur in the code,
 but fix it to prevent things from breaking in a very confusing way in
 the future.
 
 Hopefully that means in later patches in this series ;-)

I don't think that the rest of the series would have triggered this
problem either.  In fact, if I had written repack_without_ref()'s
peeling functionality using peel_ref(), then it would have *depended* on
this bug for its proper operation...otherwise it would have written the
peeled version of the loose ref to the packed-ref file.  Of course, it's
all pretty academic because the peeled version of a packed ref should
never be used when it is overridden by a loose ref, so the incorrect
peeled values in the packed-ref file shouldn't have any observable effects.

The real problem is that calling the old peel_ref() function on a packed
reference was illegitimate because the function only knew how to peel a
ref that was still active.  Plus it's kindof silly tucking away the
current reference in a global variable then looking it up again instead
of passing the ref_entry around.

Callers outside of refs.c could also not have triggered this bug because
they have no way to access overridden packed refs.

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 15/33] refs: change the internal reference-iteration API

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

 ...  This scenario doesn't currently occur in the code,
 but fix it to prevent things from breaking in a very confusing way in
 the future.
 
 Hopefully that means in later patches in this series ;-)

 I don't think that the rest of the series would have triggered this
 problem either.

Yeah, I misread the message when I said Hopefully. I somehow
thought it was saying we will fix it in the future; otherwise
things can break in a confusing way, which is not the case.
--
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 15/33] refs: change the internal reference-iteration API

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

 Establish an internal API for iterating over references, which gives
 the callback functions direct access to the ref_entry structure
 describing the reference.  (Do not change the iteration API that is
 exposed outside of the module.)

 Define a new internal callback signature

int each_ref_entry_fn(struct ref_entry *entry, void *cb_data)

 Change do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to
 accept each_ref_entry_fn callbacks, and rename them to
 do_for_each_entry_in_dir() and do_for_each_entry_in_dirs(),
 respectively.  Adapt their callers accordingly.

 Add a new function do_for_each_entry() analogous to do_for_each_ref()
 but using the new callback style.

Nicely done.


 Change do_one_ref() into an each_ref_entry_fn that does some
 bookkeeping and then calls a wrapped each_ref_fn.

 Reimplement do_for_each_ref() in terms of do_for_each_entry(), using
 do_one_ref() as an adapter.

 Please note that the responsibility for setting current_ref remains in
 do_one_ref(), which means that current_ref is *not* set when iterating
 over references via the new internal API.  This is not a disadvantage,
 because current_ref is not needed by callers of the internal API (they
 receive a pointer to the current ref_entry anyway).  But more
 importantly, this change prevents peel_ref() from returning invalid
 results in the following scenario:

 When iterating via the external API, the iteration always includes
 both packed and loose references, and in particular never presents a
 packed ref if there is a loose ref with the same name.  The internal
 API, on the other hand, gives the option to iterate over only the
 packed references.  During such an iteration, there is no check
 whether the packed ref might be hidden by a loose ref of the same
 name.  But until now the packed ref was recorded in current_ref during
 the iteration.  So if peel_ref() were called with the reference name
 corresponding to current ref, it would return the peeled version of
 the packed ref even though there might be a loose ref that peels to a
 different value.  This scenario doesn't currently occur in the code,
 but fix it to prevent things from breaking in a very confusing way in
 the future.

Hopefully that means in later patches in this series ;-)
--
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 15/33] refs: change the internal reference-iteration API

2013-04-14 Thread Michael Haggerty
Establish an internal API for iterating over references, which gives
the callback functions direct access to the ref_entry structure
describing the reference.  (Do not change the iteration API that is
exposed outside of the module.)

Define a new internal callback signature

   int each_ref_entry_fn(struct ref_entry *entry, void *cb_data)

Change do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to
accept each_ref_entry_fn callbacks, and rename them to
do_for_each_entry_in_dir() and do_for_each_entry_in_dirs(),
respectively.  Adapt their callers accordingly.

Add a new function do_for_each_entry() analogous to do_for_each_ref()
but using the new callback style.

Change do_one_ref() into an each_ref_entry_fn that does some
bookkeeping and then calls a wrapped each_ref_fn.

Reimplement do_for_each_ref() in terms of do_for_each_entry(), using
do_one_ref() as an adapter.

Please note that the responsibility for setting current_ref remains in
do_one_ref(), which means that current_ref is *not* set when iterating
over references via the new internal API.  This is not a disadvantage,
because current_ref is not needed by callers of the internal API (they
receive a pointer to the current ref_entry anyway).  But more
importantly, this change prevents peel_ref() from returning invalid
results in the following scenario:

When iterating via the external API, the iteration always includes
both packed and loose references, and in particular never presents a
packed ref if there is a loose ref with the same name.  The internal
API, on the other hand, gives the option to iterate over only the
packed references.  During such an iteration, there is no check
whether the packed ref might be hidden by a loose ref of the same
name.  But until now the packed ref was recorded in current_ref during
the iteration.  So if peel_ref() were called with the reference name
corresponding to current ref, it would return the peeled version of
the packed ref even though there might be a loose ref that peels to a
different value.  This scenario doesn't currently occur in the code,
but fix it to prevent things from breaking in a very confusing way in
the future.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 142 +
 1 file changed, 81 insertions(+), 61 deletions(-)

diff --git a/refs.c b/refs.c
index 8cc6109..51f68d3 100644
--- a/refs.c
+++ b/refs.c
@@ -556,22 +556,34 @@ static int ref_resolves_to_object(struct ref_entry *entry)
  */
 static struct ref_entry *current_ref;
 
+typedef int each_ref_entry_fn(struct ref_entry *entry, void *cb_data);
+
+struct ref_entry_cb {
+   const char *base;
+   int trim;
+   int flags;
+   each_ref_fn *fn;
+   void *cb_data;
+};
+
 /*
- * Handle one reference in a do_for_each_ref*()-style iteration.
+ * Handle one reference in a do_for_each_ref*()-style iteration,
+ * calling an each_ref_fn for each entry.
  */
-static int do_one_ref(const char *base, each_ref_fn fn, int trim,
- int flags, void *cb_data, struct ref_entry *entry)
+static int do_one_ref(struct ref_entry *entry, void *cb_data)
 {
+   struct ref_entry_cb *data = cb_data;
int retval;
-   if (prefixcmp(entry-name, base))
+   if (prefixcmp(entry-name, data-base))
return 0;
 
-   if (!((flags  DO_FOR_EACH_INCLUDE_BROKEN) ||
+   if (!((data-flags  DO_FOR_EACH_INCLUDE_BROKEN) ||
  ref_resolves_to_object(entry)))
return 0;
 
current_ref = entry;
-   retval = fn(entry-name + trim, entry-u.value.sha1, entry-flag, 
cb_data);
+   retval = data-fn(entry-name + data-trim, entry-u.value.sha1,
+ entry-flag, data-cb_data);
current_ref = NULL;
return retval;
 }
@@ -580,11 +592,11 @@ static int do_one_ref(const char *base, each_ref_fn fn, 
int trim,
  * Call fn for each reference in dir that has index in the range
  * offset = index  dir-nr.  Recurse into subdirectories that are in
  * that index range, sorting them before iterating.  This function
- * does not sort dir itself; it should be sorted beforehand.
+ * does not sort dir itself; it should be sorted beforehand.  fn is
+ * called for all references, including broken ones.
  */
-static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,
- const char *base,
- each_ref_fn fn, int trim, int flags, void 
*cb_data)
+static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,
+   each_ref_entry_fn fn, void *cb_data)
 {
int i;
assert(dir-sorted == dir-nr);
@@ -594,10 +606,9 @@ static int do_for_each_ref_in_dir(struct ref_dir *dir, int 
offset,
if (entry-flag  REF_DIR) {
struct ref_dir *subdir = get_ref_dir(entry);
sort_ref_dir(subdir);
-   retval =