Re: [PATCH v11 31/41] refs.c: make prune_ref use a transaction to delete the ref

2014-05-28 Thread Jonathan Nieder
Ronnie Sahlberg wrote:

 Change prune_ref to delete the ref using a ref transaction. To do this we also
 need to add a new flag REF_ISPRUNING that will tell the transaction that we
 do not want to delete this ref from the packed refs.

s/from the packed refs/from packed-refs, nor delete the ref's reflog/

[...]
 --- a/refs.h
 +++ b/refs.h
 @@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct 
 strbuf *err);
   * The following functions add a reference check or update to a
   * ref_transaction.  In all of them, refname is the name of the
   * reference to be affected.  The functions make internal copies of
   * refname, so the caller retains ownership of the parameter.  flags
   * can be REF_NODEREF; it is passed to update_ref_lock().
   */
  
  /*
 + * ref_transaction_update ref_transaction_create and ref_transaction_delete
 + * all take a flag argument. Currently the only public flag is REF_NODEREF.
 + * Flag values = 0x100 are reserved for internal use.
 + */
 +/*
   * Add a reference update to transaction.  new_sha1 is the value that

The comment right before here already tries to explain the flag argument,
though it isn't in an obvious place so it's easy to miss.  Maybe the flag
argument should be explained in the overview documentation for the
ref_transaction API near the top of the file (but I haven't thought that
through, so leaving it alone).

How about this as a way to make the reserved flag values easier to
find when adding new flags?

diff --git i/refs.h w/refs.h
index 25ac4a9..dee7c8f 100644
--- i/refs.h
+++ w/refs.h
@@ -171,8 +171,17 @@ extern int ref_exists(const char *);
  */
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
-/** Locks any ref (for 'HEAD' type refs). */
+/*
+ * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
+ * ref_transaction_create(), etc.
+ * REF_NODEREF: act on the ref directly, instead of dereferencing
+ *  symbolic references.
+ *
+ * Flags = 0x100 are reserved for internal use.
+ */
 #define REF_NODEREF0x01
+
+/** Locks any ref (for 'HEAD' type refs). */
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
@@ -265,11 +274,6 @@ struct ref_transaction *ref_transaction_begin(struct 
strbuf *err);
  */
 
 /*
- * ref_transaction_update ref_transaction_create and ref_transaction_delete
- * all take a flag argument. Currently the only public flag is REF_NODEREF.
- * Flag values = 0x100 are reserved for internal use.
- */
-/*
  * Add a reference update to transaction.  new_sha1 is the value that
  * the reference should have after the update, or zeros if it should
  * be deleted.  If have_old is true, then old_sha1 holds the value
--
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 v11 31/41] refs.c: make prune_ref use a transaction to delete the ref

2014-05-28 Thread Ronnie Sahlberg
On Wed, May 28, 2014 at 2:51 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 Change prune_ref to delete the ref using a ref transaction. To do this we 
 also
 need to add a new flag REF_ISPRUNING that will tell the transaction that we
 do not want to delete this ref from the packed refs.

 s/from the packed refs/from packed-refs, nor delete the ref's reflog/

 [...]
 --- a/refs.h
 +++ b/refs.h
 @@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct 
 strbuf *err);
   * The following functions add a reference check or update to a
   * ref_transaction.  In all of them, refname is the name of the
   * reference to be affected.  The functions make internal copies of
   * refname, so the caller retains ownership of the parameter.  flags
   * can be REF_NODEREF; it is passed to update_ref_lock().
   */

  /*
 + * ref_transaction_update ref_transaction_create and ref_transaction_delete
 + * all take a flag argument. Currently the only public flag is REF_NODEREF.
 + * Flag values = 0x100 are reserved for internal use.
 + */
 +/*
   * Add a reference update to transaction.  new_sha1 is the value that

 The comment right before here already tries to explain the flag argument,
 though it isn't in an obvious place so it's easy to miss.  Maybe the flag
 argument should be explained in the overview documentation for the
 ref_transaction API near the top of the file (but I haven't thought that
 through, so leaving it alone).

 How about this as a way to make the reserved flag values easier to
 find when adding new flags?

 diff --git i/refs.h w/refs.h
 index 25ac4a9..dee7c8f 100644
 --- i/refs.h
 +++ w/refs.h
 @@ -171,8 +171,17 @@ extern int ref_exists(const char *);
   */
  extern int peel_ref(const char *refname, unsigned char *sha1);

 -/** Locks any ref (for 'HEAD' type refs). */
 +/*
 + * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
 + * ref_transaction_create(), etc.
 + * REF_NODEREF: act on the ref directly, instead of dereferencing
 + *  symbolic references.
 + *
 + * Flags = 0x100 are reserved for internal use.
 + */
  #define REF_NODEREF0x01
 +
 +/** Locks any ref (for 'HEAD' type refs). */
  extern struct ref_lock *lock_any_ref_for_update(const char *refname,
 const unsigned char *old_sha1,
 int flags, int *type_p);
 @@ -265,11 +274,6 @@ struct ref_transaction *ref_transaction_begin(struct 
 strbuf *err);
   */

  /*
 - * ref_transaction_update ref_transaction_create and ref_transaction_delete
 - * all take a flag argument. Currently the only public flag is REF_NODEREF.
 - * Flag values = 0x100 are reserved for internal use.
 - */
 -/*
   * Add a reference update to transaction.  new_sha1 is the value that
   * the reference should have after the update, or zeros if it should
   * be deleted.  If have_old is true, then old_sha1 holds the value

Thanks. Changed.
--
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 v11 31/41] refs.c: make prune_ref use a transaction to delete the ref

2014-05-27 Thread Ronnie Sahlberg
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 28 +---
 refs.h |  5 +
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 4ca84f7..1819434 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static inline int bad_ref_char(int ch)
 }
 
 /*
+ * Used as a flag to ref_transaction_delete when a loose ref is being
+ * pruned.
+ */
+#define REF_ISPRUNING  0x0100
+
+/*
  * Try to read one refname component from the front of refname.  Return
  * the length of the component found, or -1 if the component is not
  * legal.
@@ -2328,17 +2334,24 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-   struct ref_lock *lock;
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
 
if (check_refname_format(r-name + 5, 0))
return;
 
-   lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
-   if (lock) {
-   unlink_or_warn(git_path(%s, r-name));
-   unlock_ref(lock);
-   try_remove_empty_parents(r-name);
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_delete(transaction, r-name, r-sha1,
+  REF_ISPRUNING, 1, err) ||
+   ref_transaction_commit(transaction, NULL, err)) {
+   ref_transaction_free(transaction);
+   error(%s, err.buf);
+   strbuf_release(err);
+   return;
}
+   ref_transaction_free(transaction);
+   try_remove_empty_parents(r-name);
 }
 
 static void prune_refs(struct ref_to_prune *r)
@@ -3546,9 +3559,10 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
struct ref_update *update = updates[i];
 
if (update-lock) {
-   delnames[delnum++] = update-lock-ref_name;
ret |= delete_ref_loose(update-lock, update-type,
err);
+   if (!(update-flags  REF_ISPRUNING))
+   delnames[delnum++] = update-lock-ref_name;
}
}
 
diff --git a/refs.h b/refs.h
index 1e25e1c..39c97f8 100644
--- a/refs.h
+++ b/refs.h
@@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct 
strbuf *err);
  */
 
 /*
+ * ref_transaction_update ref_transaction_create and ref_transaction_delete
+ * all take a flag argument. Currently the only public flag is REF_NODEREF.
+ * Flag values = 0x100 are reserved for internal use.
+ */
+/*
  * Add a reference update to transaction.  new_sha1 is the value that
  * the reference should have after the update, or zeros if it should
  * be deleted.  If have_old is true, then old_sha1 holds the value
-- 
2.0.0.rc3.474.g0203784

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