Re: [PATCH v8 21/44] refs.c: ref_transaction_commit should not free the transaction
On Thu, May 15, 2014 at 5:20 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.
Can I always use ref_transaction_rollback instead of ref_transaction_free?
It would be convenient if my cleanup code could always call _rollback
instead of having to do something different for success and errors.
Currently, yes.
But it might make sense to change these so rollback only clears the
updates that are in flight from the transaction and
free only frees the transaction itself iff there are no updates in flight.
I.e. the success and error would then differ like this :
...
if (transaction_commit()) {
transaction_rollback()
transaction_free()
return error(some error)
}
transaction_free()
Another way to ask the question: what is it valid to do with a
transaction after commiting?
Right now the only valid thing to do is either rollback or free. But
we could allow other things too :
re-usable transactions.
-
I don't know if this is a good reason or not, but one reason we might
want to keep
two different names could be if we want to start allowing to re-use
transactions.
For example for cases/backends where transaction_begin() might be very
expensive.
For that case I would imagine we could allow to do things such as
t = transaction_begin()
...
/* first transaction */
transaction_update(...)
transaction_commit(...)
if transaction failed transaction_rollback(...)
/* second transaction, first transaction cleared all updates in
flight either through commit or through rollback */
transaction_update()
transaction_commit()
if transaction failed transaction_rollback(...)
...
transaction_free()
(In order to do something like this we would still need to do some
changes so that rollback will only free the updates that were in
flight but not free the transaction itself.)
Thanks,
Jonathan
--
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 v8 21/44] refs.c: ref_transaction_commit should not free the transaction
On Fri, May 16, 2014 at 8:02 AM, Ronnie Sahlberg sahlb...@google.com wrote:
On Thu, May 15, 2014 at 5:20 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.
Can I always use ref_transaction_rollback instead of ref_transaction_free?
It would be convenient if my cleanup code could always call _rollback
instead of having to do something different for success and errors.
Currently, yes.
But it might make sense to change these so rollback only clears the
updates that are in flight from the transaction and
free only frees the transaction itself iff there are no updates in flight.
I.e. the success and error would then differ like this :
...
if (transaction_commit()) {
transaction_rollback()
transaction_free()
return error(some error)
}
transaction_free()
But we do not really need rollback right now. If / when we decide to
need re-startable/re-usable transactions we can add it back when
needed.
Let me update the patch series and remove transaction_rollback and
replace all calls to it with calls to transaction_free instead.
Another way to ask the question: what is it valid to do with a
transaction after commiting?
Right now the only valid thing to do is either rollback or free. But
we could allow other things too :
re-usable transactions.
-
I don't know if this is a good reason or not, but one reason we might
want to keep
two different names could be if we want to start allowing to re-use
transactions.
For example for cases/backends where transaction_begin() might be very
expensive.
For that case I would imagine we could allow to do things such as
t = transaction_begin()
...
/* first transaction */
transaction_update(...)
transaction_commit(...)
if transaction failed transaction_rollback(...)
/* second transaction, first transaction cleared all updates in
flight either through commit or through rollback */
transaction_update()
transaction_commit()
if transaction failed transaction_rollback(...)
...
transaction_free()
(In order to do something like this we would still need to do some
changes so that rollback will only free the updates that were in
flight but not free the transaction itself.)
Thanks,
Jonathan
--
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 v8 21/44] refs.c: ref_transaction_commit should not free the transaction
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.
By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_rollback
safely. Instead of this horribleness
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval)) ||
(ref_transaction_commit(t, action, err) !(t = NULL))) {
ref_transaction_rollback(t);
we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval) ||
ref_transaction_commit(t, action, err)) {
ref_transaction_rollback(t);
... die/return ...
ref_transaction_free(transaction);
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 1 +
builtin/commit.c | 1 +
builtin/replace.c| 1 +
builtin/tag.c| 1 +
builtin/update-ref.c | 1 +
fast-import.c| 8
refs.c | 14 ++
refs.h | 14 +-
sequencer.c | 8
9 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/branch.c b/branch.c
index 2aa5c82..8e58908 100644
--- a/branch.c
+++ b/branch.c
@@ -305,6 +305,7 @@ void create_branch(const char *head,
ref_transaction_commit(transaction, msg, err))
die_errno(_(%s: failed to write ref: %s),
ref.buf, err.buf);
+ ref_transaction_free(transaction);
}
if (real_ref track)
diff --git a/builtin/commit.c b/builtin/commit.c
index 592f019..16fadbb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1726,6 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
rollback_index_files();
die(%s, err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
diff --git a/builtin/replace.c b/builtin/replace.c
index ee34d5d..91354a7 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -164,6 +164,7 @@ static int replace_object_sha1(const char *object_ref,
ref_transaction_commit(transaction, NULL, err))
die(%s: failed to replace ref: %s, ref, err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index bf2a5c3..fbd2989 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
0, !is_null_sha1(prev)) ||
ref_transaction_commit(transaction, NULL, err))
die(_(%s: cannot update the ref: %s), ref.buf, err.buf);
+ ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2a51df1..b5f4731 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/fast-import.c b/fast-import.c
index 79d219b..3e356da 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1708,16 +1708,16 @@ static int update_branch(struct branch *b)
}
}
transaction = ref_transaction_begin();
- if ((!transaction ||
+ if (!transaction ||
ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
- 0, 1)) ||
- (ref_transaction_commit(transaction, msg, err)
-!(transaction = NULL))) {
+ 0, 1) ||
+ ref_transaction_commit(transaction, msg, err)) {
ref_transaction_rollback(transaction);
error(Unable to update branch %s: %s, b-name, err.buf);
strbuf_release(err);
return -1;
}
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/refs.c b/refs.c
index 4a37f87..82a8d4e 100644
--- a/refs.c
+++ b/refs.c
@@ -3322,7 +3322,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void ref_transaction_free(struct ref_transaction
Re: [PATCH v8 21/44] refs.c: ref_transaction_commit should not free the transaction
Ronnie Sahlberg wrote: Change ref_transaction_commit so that it does not free the transaction. Instead require that a caller will end a transaction by either calling ref_transaction_rollback or ref_transaction_free. Can I always use ref_transaction_rollback instead of ref_transaction_free? It would be convenient if my cleanup code could always call _rollback instead of having to do something different for success and errors. Another way to ask the question: what is it valid to do with a transaction after commiting? Thanks, Jonathan -- 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
