Re: [PATCH v20 33/48] walker.c: use ref transaction for ref updates

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

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

quoted

Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.

Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would

s/collissions/collisions/

quoted

protect against and cause the fetch to fail for to be even more rare.

Grammatico: s/to be/are/ ?

Thanks.  Fixed.

quoted

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

diff --git a/walker.c b/walker.c
index 1dd86b8..60d9f9e 100644
--- a/walker.c
+++ b/walker.c

@@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
 int walker_fetch(struct walker *walker, int targets, char **target,
               const char **write_ref, const char *write_ref_log_details)
 {
-     struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+     struct strbuf ref_name = STRBUF_INIT;
+     struct strbuf err = STRBUF_INIT;
+     struct ref_transaction *transaction = NULL;
      unsigned char *sha1 = xmalloc(targets * 20);
-     char *msg;
-     int ret;
+     char *msg = NULL;
      int i;

      save_commit_buffer = 0;

-     for (i = 0; i < targets; i++) {
-             if (!write_ref || !write_ref[i])
-                     continue;
-
-             lock[i] = lock_ref_sha1(write_ref[i], NULL);
-             if (!lock[i]) {
-                     error("Can't lock ref %s", write_ref[i]);
-                     goto unlock_and_fail;
+     if (write_ref) {
+             transaction = ref_transaction_begin(&err);
+             if (!transaction) {
+                     error("%s", err.buf);
+                     goto rollback_and_fail;
              }
      }
-

Is there some reason why the transaction cannot be built up during a
single iteration over targets, thereby also avoiding the need for the
sha1[20*i] stuff?  This seems like exactly the kind of situation where
transactions should *save* code.  But perhaps I've overlooked a
dependency between the two loops.

I did it this way to keep the changes minimal. But you are right that
with this we can do a larger refactoring and start saving some code.
I can add changes to a later series to do that change but I want to
keep this change as small as possible for now.

regards
ronnie sahlberg