Re: [PATCH v14 26/40] walker.c: use ref transaction for ref updates

2014-06-10 Thread Jonathan Nieder
Ronnie Sahlberg wrote:

 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  walker.c | 59 +++
  1 file changed, 35 insertions(+), 24 deletions(-)

Reviewed-by: Jonathan Nieder jrnie...@gmail.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


[PATCH v14 26/40] walker.c: use ref transaction for ref updates

2014-06-06 Thread Ronnie Sahlberg
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
protect against and cause the fetch to fail for to be even more rare.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 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;
}
}
-
if (!walker-get_recover)
for_each_ref(mark_complete, NULL);
 
for (i = 0; i  targets; i++) {
if (interpret_target(walker, target[i], sha1[20 * i])) {
error(Could not interpret response from server '%s' as 
something to pull, target[i]);
-   goto unlock_and_fail;
+   goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
-   goto unlock_and_fail;
+   goto rollback_and_fail;
}
 
if (loop(walker))
-   goto unlock_and_fail;
+   goto rollback_and_fail;
 
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char 
**target,
for (i = 0; i  targets; i++) {
if (!write_ref || !write_ref[i])
continue;
-   ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch 
(unknown));
-   lock[i] = NULL;
-   if (ret)
-   goto unlock_and_fail;
+   strbuf_reset(ref_name);
+   strbuf_addf(ref_name, refs/%s, write_ref[i]);
+   if (ref_transaction_update(transaction, ref_name.buf,
+  sha1[20 * i], NULL, 0, 0,
+  err)) {
+   error(%s, err.buf);
+   goto rollback_and_fail;
+   }
+   }
+   if (write_ref) {
+   if (ref_transaction_commit(transaction,
+  msg ? msg : fetch (unknown),
+  err)) {
+   error(%s, err.buf);
+   goto rollback_and_fail;
+   }
+   ref_transaction_free(transaction);
}
-   free(msg);
 
+   free(msg);
return 0;
 
-unlock_and_fail:
-   for (i = 0; i  targets; i++)
-   if (lock[i])
-   unlock_ref(lock[i]);
+rollback_and_fail:
+   ref_transaction_free(transaction);
+   free(msg);
+   strbuf_release(err);
+   strbuf_release(ref_name);
 
return -1;
 }
-- 
2.0.0.582.ge25c160

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