Re: [PATCH 10/15] refs.c: allow multiple reflog updates during a single transaction

Ronnie Sahlberg [off-list ref] writes:

Not a very informative comment, I'd have to say.  How are users of
this API expected to avoid stepping on each others' and API
implementation's toes?

What is this change about?

Does the lifetime rule for "struct lock_file" described in
Documentation/technical/api-lockfile.txt, namely, "once you call
hold_lock_file_* family on it, you cannot free it yourself", have
any implication on this?

+	if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+		update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+

Hmph, does this mean that the caller needs to keep track of the refs
it ever touched inside a single transaction, call this without nolock
on the first invocation on a particular ref and with nolock on the
subsequent invocation?

Or is the "caller" just implementation detail of the API and higher level
callers do not have to care?

On Thu, Oct 23, 2014 at 11:54 AM, Junio C Hamano [off-list ref] wrote:

Ronnie Sahlberg [off-list ref] writes:

quoted

@@ -3531,7 +3537,7 @@ struct ref_update {
      enum transaction_update_type update_type;
      unsigned char new_sha1[20];
      unsigned char old_sha1[20];
-     int flags; /* REF_NODEREF? */
+     int flags; /* REF_NODEREF? or private flags */

Not a very informative comment, I'd have to say.  How are users of
this API expected to avoid stepping on each others' and API
implementation's toes?

That is an old and bitrotted comment.
I changed it to point to the canonical definition of these flags instead :
  int flags;  /* The flags to transaction_update_ref[log] are defined
                  * in refs.h
                  */

quoted

@@ -3539,8 +3545,9 @@ struct ref_update {

      /* used by reflog updates */
      int reflog_fd;
-     struct lock_file reflog_lock;
+     struct lock_file *reflog_lock;

What is this change about?

We have one update entry for each line we want to write to the reflog.
So for the first update to a reflog we allocate a lock_file structure.
For any subsequent reflog entry to the same ref we just let the
pointer point to the previous structure we already allocated.

I.e. a way to have only one lock_file structure and share it across
multiple struct ref_update structures.

Does the lifetime rule for "struct lock_file" described in
Documentation/technical/api-lockfile.txt, namely, "once you call
hold_lock_file_* family on it, you cannot free it yourself", have
any implication on this?

Nope.

quoted

+     if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+             update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+

Hmph, does this mean that the caller needs to keep track of the refs
it ever touched inside a single transaction, call this without nolock
on the first invocation on a particular ref and with nolock on the
subsequent invocation?

Nope. This is not visible to the caller and is managed transparently
inside the transaction code.

Or is the "caller" just implementation detail of the API and higher level
callers do not have to care?

The latter,    the higher level do not have to care.


regards
ronnie sahlberg