Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>> ref-transaction-1 (2014-07-16) 20 commits
>> -------------------------------------------------------------
>> Second batch of ref transactions
>>
>> - refs.c: make delete_ref use a transaction
>> - refs.c: make prune_ref use a transaction to delete the ref
>> - refs.c: remove lock_ref_sha1
>> - refs.c: remove the update_ref_write function
>> - refs.c: remove the update_ref_lock function
>> - refs.c: make lock_ref_sha1 static
>> - walker.c: use ref transaction for ref updates
>> - fast-import.c: use a ref transaction when dumping tags
>> - receive-pack.c: use a reference transaction for updating the refs
>> - refs.c: change update_ref to use a transaction
>> - branch.c: use ref transaction for all ref updates
>> - fast-import.c: change update_branch to use ref transactions
>> - sequencer.c: use ref transactions for all ref updates
>> - commit.c: use ref transactions for updates
>> - replace.c: use the ref transaction functions for updates
>> - tag.c: use ref transactions when doing updates
>> - refs.c: add transaction.status and track OPEN/CLOSED/ERROR
>> - refs.c: make ref_transaction_begin take an err argument
>> - refs.c: update ref_transaction_delete to check for error and return status
>> - refs.c: change ref_transaction_create to do error checking and return
>> status
>> (this branch is used by rs/ref-transaction, rs/ref-transaction-multi,
>> rs/ref-transaction-reflog and rs/ref-transaction-rename.)
[...]
> I've having trouble keeping track of how patches change, which patches
> have been reviewed and which haven't, unaddressed comments, and so on,
> so as an experiment I've pushed this part of the series to the Gerrit
> server at
>
> https://code-review.googlesource.com/#/q/topic:ref-transaction-1
Outcome of the experiment: patches gained some minor changes and then
1-12
Reviewed-by: Jonathan Nieder <[email protected]>
13
Reviewed-by: Michael Haggerty <[email protected]>
Reviewed-by: Jonathan Nieder <[email protected]>
14
Reviewed-by: Jonathan Nieder <[email protected]>
15-16
Reviewed-by: Michael Haggerty <[email protected]>
Reviewed-by: Jonathan Nieder <[email protected]>
17
Reviewed-by: Michael Haggerty <[email protected]>
18
Reviewed-by: Michael Haggerty <[email protected]>
Reviewed-by: Jonathan Nieder <[email protected]>
19
Reviewed-by: Jonathan Nieder <[email protected]>
20
Reviewed-by: Michael Haggerty <[email protected]>
Reviewed-by: Jonathan Nieder <[email protected]>
I found the web UI helpful in seeing how each patch evolved since I
last looked at it. Interdiff relative to the version in pu is below.
I'm still hoping for a tweak in response to a minor comment and then I
can put up a copy of the updated series to pull.
The next series from Ronnie's collection is available at
https://code-review.googlesource.com/#/q/topic:ref-transaction in case
someone wants a fresh series to look at.
diff --git a/branch.c b/branch.c
index c1eae00..37ac555 100644
--- a/branch.c
+++ b/branch.c
@@ -305,6 +305,7 @@ void create_branch(const char *head,
ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
+ strbuf_release(&err);
}
if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index 668fa6a..9bf1003 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1765,8 +1765,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, "HEAD", sha1,
- current_head ?
- current_head->object.sha1 : NULL,
+ current_head
+ ? current_head->object.sha1 : NULL,
0, !!current_head, &err) ||
ref_transaction_commit(transaction, sb.buf, &err)) {
rollback_index_files();
@@ -1801,5 +1801,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
if (!quiet)
print_summary(prefix, sha1, !current_head);
+ strbuf_release(&err);
return 0;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 91099ad..224fadc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -194,7 +194,7 @@ static void write_head_info(void)
struct command {
struct command *next;
- char *error_string;
+ const char *error_string;
unsigned int skip_update:1,
did_not_exist:1;
int index;
@@ -468,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static char *update(struct command *cmd, struct shallow_info *si)
+static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
@@ -479,7 +479,7 @@ static char *update(struct command *cmd, struct
shallow_info *si)
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
rp_error("refusing to create funny ref '%s' remotely", name);
- return xstrdup("funny refname");
+ return "funny refname";
}
strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
@@ -497,20 +497,20 @@ static char *update(struct command *cmd, struct
shallow_info *si)
rp_error("refusing to update checked out branch: %s",
name);
if (deny_current_branch == DENY_UNCONFIGURED)
refuse_unconfigured_deny();
- return xstrdup("branch is currently checked out");
+ return "branch is currently checked out";
}
}
if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
error("unpack should have generated %s, "
"but I can't find it!", sha1_to_hex(new_sha1));
- return xstrdup("bad pack");
+ return "bad pack";
}
if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
if (deny_deletes && starts_with(name, "refs/heads/")) {
rp_error("denying ref deletion for %s", name);
- return xstrdup("deletion prohibited");
+ return "deletion prohibited";
}
if (!strcmp(namespaced_name, head_name)) {
@@ -525,7 +525,7 @@ static char *update(struct command *cmd, struct
shallow_info *si)
if (deny_delete_current == DENY_UNCONFIGURED)
refuse_unconfigured_deny_delete_current();
rp_error("refusing to delete the current
branch: %s", name);
- return xstrdup("deletion of the current branch
prohibited");
+ return "deletion of the current branch
prohibited";
}
}
}
@@ -543,19 +543,19 @@ static char *update(struct command *cmd, struct
shallow_info *si)
old_object->type != OBJ_COMMIT ||
new_object->type != OBJ_COMMIT) {
error("bad sha1 objects for %s", name);
- return xstrdup("bad ref");
+ return "bad ref";
}
old_commit = (struct commit *)old_object;
new_commit = (struct commit *)new_object;
if (!in_merge_bases(old_commit, new_commit)) {
rp_error("denying non-fast-forward %s"
" (you should pull first)", name);
- return xstrdup("non-fast-forward");
+ return "non-fast-forward";
}
}
if (run_update_hook(cmd)) {
rp_error("hook declined to update %s", name);
- return xstrdup("hook declined");
+ return "hook declined";
}
if (is_null_sha1(new_sha1)) {
@@ -570,7 +570,7 @@ static char *update(struct command *cmd, struct
shallow_info *si)
}
if (delete_ref(namespaced_name, old_sha1, 0)) {
rp_error("failed to delete %s", name);
- return xstrdup("failed to delete");
+ return "failed to delete";
}
return NULL; /* good */
}
@@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct
shallow_info *si)
if (shallow_update && si->shallow_ref[cmd->index] &&
update_shallow_ref(cmd, si))
- return xstrdup("shallow error");
+ return "shallow error";
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
new_sha1, old_sha1, 0, 1, &err) ||
ref_transaction_commit(transaction, "push", &err)) {
- char *str = strbuf_detach(&err, NULL);
ref_transaction_free(transaction);
- rp_error("%s", str);
- return str;
+ rp_error("%s", err.buf);
+ strbuf_release(&err);
+ return "failed to update ref";
}
ref_transaction_free(transaction);
@@ -654,9 +654,6 @@ static void check_aliased_update(struct command *cmd,
struct string_list *list)
char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
int flag;
- if (cmd->error_string)
- die("BUG: check_aliased_update called with failed cmd");
-
strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
dst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);
strbuf_release(&buf);
@@ -668,7 +665,7 @@ static void check_aliased_update(struct command *cmd,
struct string_list *list)
if (!dst_name) {
rp_error("refusing update to broken symref '%s'",
cmd->ref_name);
cmd->skip_update = 1;
- cmd->error_string = xstrdup("broken symref");
+ cmd->error_string = "broken symref";
return;
}
@@ -694,9 +691,8 @@ static void check_aliased_update(struct command *cmd,
struct string_list *list)
cmd->ref_name, cmd_oldh, cmd_newh,
dst_cmd->ref_name, dst_oldh, dst_newh);
- cmd->error_string = xstrdup("inconsistent aliased update");
- free(dst_cmd->error_string);
- dst_cmd->error_string = xstrdup("inconsistent aliased update");
+ cmd->error_string = dst_cmd->error_string =
+ "inconsistent aliased update";
}
static void check_aliased_updates(struct command *commands)
@@ -744,9 +740,7 @@ static void set_connectivity_errors(struct command
*commands,
if (!check_everything_connected(command_singleton_iterator,
0, &singleton))
continue;
- if (cmd->error_string) /* can't happen */
- continue;
- cmd->error_string = xstrdup("missing necessary objects");
+ cmd->error_string = "missing necessary objects";
}
}
@@ -783,9 +777,9 @@ static void reject_updates_to_hidden(struct command
*commands)
if (cmd->error_string || !ref_is_hidden(cmd->ref_name))
continue;
if (is_null_sha1(cmd->new_sha1))
- cmd->error_string = xstrdup("deny deleting a hidden
ref");
+ cmd->error_string = "deny deleting a hidden ref";
else
- cmd->error_string = xstrdup("deny updating a hidden
ref");
+ cmd->error_string = "deny updating a hidden ref";
}
}
@@ -799,11 +793,8 @@ static void execute_commands(struct command *commands,
struct iterate_data data;
if (unpacker_error) {
- for (cmd = commands; cmd; cmd = cmd->next) {
- if (cmd->error_string) /* can't happen */
- continue;
- cmd->error_string = xstrdup("unpacker error");
- }
+ for (cmd = commands; cmd; cmd = cmd->next)
+ cmd->error_string = "unpacker error";
return;
}
@@ -816,9 +807,8 @@ static void execute_commands(struct command *commands,
if (run_receive_hook(commands, "pre-receive", 0)) {
for (cmd = commands; cmd; cmd = cmd->next) {
- if (cmd->error_string)
- continue;
- cmd->error_string = xstrdup("pre-receive hook
declined");
+ if (!cmd->error_string)
+ cmd->error_string = "pre-receive hook declined";
}
return;
}
@@ -1096,8 +1086,7 @@ static void update_shallow_info(struct command *commands,
if (is_null_sha1(cmd->new_sha1))
continue;
if (ref_status[cmd->index]) {
- free(cmd->error_string);
- cmd->error_string = xstrdup("shallow update not
allowed");
+ cmd->error_string = "shallow update not allowed";
cmd->skip_update = 1;
}
}
@@ -1138,17 +1127,6 @@ static int delete_only(struct command *commands)
return 1;
}
-static void free_commands(struct command *commands)
-{
- while (commands) {
- struct command *next = commands->next;
-
- free(commands->error_string);
- free(commands);
- commands = next;
- }
-}
-
int cmd_receive_pack(int argc, const char **argv, const char *prefix)
{
int advertise_refs = 0;
@@ -1244,6 +1222,5 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
packet_flush(1);
sha1_array_clear(&shallow);
sha1_array_clear(&ref);
- free_commands(commands);
return 0;
}
diff --git a/builtin/replace.c b/builtin/replace.c
index 7528f3d..1fcd06d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -169,8 +169,7 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, ref, repl, prev,
- 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_update(transaction, ref, repl, prev, 0, 1, &err) ||
ref_transaction_commit(transaction, NULL, &err))
die("%s", err.buf);
diff --git a/builtin/tag.c b/builtin/tag.c
index 1aa88a2..f3f172f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -712,6 +712,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
+ strbuf_release(&err);
strbuf_release(&buf);
strbuf_release(&ref);
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c6ad0be..96a53b9 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,6 +366,8 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (read_stdin) {
transaction = ref_transaction_begin(&err);
+ if (!transaction)
+ die("%s", err.buf);
if (delete || no_deref || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
@@ -374,6 +376,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
+ strbuf_release(&err);
return 0;
}
diff --git a/fast-import.c b/fast-import.c
index a95e1be..e7f6e37 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1716,6 +1716,7 @@ static int update_branch(struct branch *b)
return -1;
}
ref_transaction_free(transaction);
+ strbuf_release(&err);
return 0;
}
diff --git a/refs.c b/refs.c
index 0017d9c..819e25f 100644
--- a/refs.c
+++ b/refs.c
@@ -2074,7 +2074,10 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
return logs_found;
}
-/* This function should make sure errno is meaningful on error */
+/*
+ * Locks a "refs/" ref returning the lock on success and NULL on failure.
+ * On failure errno is set to something meaningful.
+ */
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2401,6 +2404,7 @@ static void prune_ref(struct ref_to_prune *r)
return;
}
ref_transaction_free(transaction);
+ strbuf_release(&err);
try_remove_empty_parents(r->name);
}
@@ -2575,6 +2579,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
return 1;
}
ref_transaction_free(transaction);
+ strbuf_release(&err);
return 0;
}
@@ -3352,19 +3357,15 @@ struct ref_update {
* Transaction states.
* OPEN: The transaction is in a valid state and can accept new updates.
* An OPEN transaction can be committed.
- * CLOSED: If an open transaction is successfully committed the state will
- * change to CLOSED. No further changes can be made to a CLOSED
- * transaction.
- * CLOSED means that all updates have been successfully committed and
- * the only thing that remains is to free the completed transaction.
- * ERROR: The transaction has failed and is no longer committable.
- * No further changes can be made to a CLOSED transaction and it must
- * be rolled back using transaction_free.
+ * CLOSED: A closed transaction is no longer active and no other operations
+ * than free can be used on it in this state.
+ * A transaction can either become closed by successfully committing
+ * an active transaction or if there is a failure while building
+ * the transaction thus rendering it failed/inactive.
*/
enum ref_transaction_state {
REF_TRANSACTION_OPEN = 0,
- REF_TRANSACTION_CLOSED = 1,
- REF_TRANSACTION_ERROR = 2,
+ REF_TRANSACTION_CLOSED = 1
};
/*
@@ -3509,6 +3510,7 @@ int update_ref(const char *action, const char *refname,
strbuf_release(&err);
return 1;
}
+ strbuf_release(&err);
return 0;
}
@@ -3588,10 +3590,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
msg);
update->lock = NULL; /* freed by write_ref_sha1 */
if (ret) {
- const char *str = "Cannot update the ref '%s'.";
-
if (err)
- strbuf_addf(err, str, update->refname);
+ strbuf_addf(err, "Cannot update the "
+ "ref '%s'.",
+ update->refname);
goto cleanup;
}
}
@@ -3614,8 +3616,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(&ref_cache);
cleanup:
- transaction->state = ret ? REF_TRANSACTION_ERROR
- : REF_TRANSACTION_CLOSED;
+ transaction->state = REF_TRANSACTION_CLOSED;
for (i = 0; i < n; i++)
if (updates[i]->lock)
diff --git a/refs.h b/refs.h
index aad846c..69ef28c 100644
--- a/refs.h
+++ b/refs.h
@@ -180,8 +180,7 @@ extern int peel_ref(const char *refname, unsigned char
*sha1);
*/
#define REF_NODEREF 0x01
/*
- * Locks any ref (for 'HEAD' type refs) and sets errno to something
- * meaningful on failure.
+ * This function sets errno to something meaningful on failure.
*/
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
diff --git a/sequencer.c b/sequencer.c
index cf17c69..5e93b6a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -296,6 +296,7 @@ static int fast_forward_to(const unsigned char *to, const
unsigned char *from,
}
strbuf_release(&sb);
+ strbuf_release(&err);
ref_transaction_free(transaction);
return 0;
}
diff --git a/walker.c b/walker.c
index 60d9f9e..b8a5441 100644
--- a/walker.c
+++ b/walker.c
@@ -251,12 +251,12 @@ 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 strbuf ref_name = STRBUF_INIT;
+ struct strbuf refname = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
struct ref_transaction *transaction = NULL;
unsigned char *sha1 = xmalloc(targets * 20);
char *msg = NULL;
- int i;
+ int i, ret = -1;
save_commit_buffer = 0;
@@ -264,7 +264,7 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
transaction = ref_transaction_begin(&err);
if (!transaction) {
error("%s", err.buf);
- goto rollback_and_fail;
+ goto done;
}
}
if (!walker->get_recover)
@@ -273,15 +273,18 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
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 rollback_and_fail;
+ goto done;
}
if (process(walker, lookup_unknown_object(&sha1[20 * i])))
- goto rollback_and_fail;
+ goto done;
}
if (loop(walker))
- goto rollback_and_fail;
-
+ goto done;
+ if (!write_ref) {
+ ret = 0;
+ goto done;
+ }
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
sprintf(msg, "fetch from %s", write_ref_log_details);
@@ -289,37 +292,33 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
msg = NULL;
}
for (i = 0; i < targets; i++) {
- if (!write_ref || !write_ref[i])
+ if (!write_ref[i])
continue;
- strbuf_reset(&ref_name);
- strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
- if (ref_transaction_update(transaction, ref_name.buf,
+ strbuf_reset(&refname);
+ strbuf_addf(&refname, "refs/%s", write_ref[i]);
+ if (ref_transaction_update(transaction, refname.buf,
&sha1[20 * i], NULL, 0, 0,
&err)) {
error("%s", err.buf);
- goto rollback_and_fail;
+ goto done;
}
}
- 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);
+ if (ref_transaction_commit(transaction,
+ msg ? msg : "fetch (unknown)",
+ &err)) {
+ error("%s", err.buf);
+ goto done;
}
- free(msg);
- return 0;
+ ret = 0;
-rollback_and_fail:
+done:
ref_transaction_free(transaction);
free(msg);
+ free(sha1);
strbuf_release(&err);
- strbuf_release(&ref_name);
-
- return -1;
+ strbuf_release(&refname);
+ return ret;
}
void walker_free(struct walker *walker)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
