Re: [PATCH v20 27/48] sequencer.c: use ref transactions for all ref updates

On Tue, Jul 8, 2014 at 5:23 AM, Michael Haggerty [off-list ref] wrote:

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:

quoted

Change to use ref transactions for all updates to refs.

Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
 sequencer.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0a80c58..fd8acaf 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
                      int unborn, struct replay_opts *opts)
 {
-     struct ref_lock *ref_lock;
+     struct ref_transaction *transaction;
      struct strbuf sb = STRBUF_INIT;
-     int ret;
+     struct strbuf err = STRBUF_INIT;

      read_cache();
      if (checkout_fast_forward(from, to, 1))
              exit(1); /* the callee should have complained already */
-     ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
-                                        0, NULL);
-     if (!ref_lock)
-             return error(_("Failed to lock HEAD during fast_forward_to"));

I think you've changed the semantics when unborn is set.  Please note
that lock_any_ref_for_update() behaves differently if old_sha1 is NULL
(when no check is done) vs. when it is null_sha1 (when it verifies that
the reference didn't previously exist).  So when unborn is true, the old
code verifies that the reference previously didn't exist...

quoted

      strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-     ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+     transaction = ref_transaction_begin(&err);
+     if (!transaction ||
+         ref_transaction_update(transaction, "HEAD", to, from,
+                                0, !unborn, &err) ||

...whereas when unborn is true, the new code does no check at all.  I
think you want

    ref_transaction_update(transaction, "HEAD",
                           to, unborn ? null_sha1 : from,
                           0, 1, &err) ||

Right.  Fixed.

Thanks.