Re: [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs
Thanks.
Done.
I added a function to stop leaking commands too.
On Tue, Jun 10, 2014 at 4:18 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,7 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static struct string_list error_strings = STRING_LIST_INIT_DUP;
[...]
@@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
[...]
+ 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)) {
+
+ const char *str;
+ string_list_append(error_strings, err.buf);
+ str = error_strings.items[error_strings.nr - 1].string;
[...]
+ return str;
[...]
@@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv,
const char *prefix)
packet_flush(1);
sha1_array_clear(shallow);
sha1_array_clear(ref);
+ string_list_clear(error_strings, 0);
return 0;
I think it's okay to let error strings accumulate like this because
there will not be too many of them. Still I wonder, would it work to
change the convention to transfer ownership of the string to the caller?
Ultimately 'commands' is leaked so we could even avoid the strdups but
I'd rather leave it possible to clean up.
Something like this:
diff --git i/builtin/receive-pack.c w/builtin/receive-pack.c
index 13f4a63..d8ab7b2 100644
--- i/builtin/receive-pack.c
+++ w/builtin/receive-pack.c
@@ -46,7 +46,6 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
-static struct string_list error_strings = STRING_LIST_INIT_DUP;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -195,7 +194,7 @@ static void write_head_info(void)
struct command {
struct command *next;
- const char *error_string;
+ char *error_string;
unsigned int skip_update:1,
did_not_exist:1;
int index;
@@ -469,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static const char *update(struct command *cmd, struct shallow_info *si)
+static char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd-ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
@@ -589,12 +588,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
new_sha1, old_sha1, 0, 1, err) ||
ref_transaction_commit(transaction, push, err)) {
- const char *str;
- string_list_append(error_strings, err.buf);
- str = error_strings.items[error_strings.nr -
1].string;
- strbuf_release(err);
-
+ char *str = strbuf_detach(err, NULL);
ref_transaction_free(transaction);
+
rp_error(%s, str);
return str;
}
@@ -659,6 +655,9 @@ 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_alised_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);
@@ -670,7 +669,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 = broken symref;
+ cmd-error_string = xstrdup(broken symref);
return;
}
@@ -696,8 +695,9 @@ 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 = dst_cmd-error_string =
- inconsistent aliased update;
+ cmd-error_string = xstrdup(inconsistent aliased update);
+ free(dst_cmd-error_string);
+ dst_cmd-error_string = xstrdup(inconsistent aliased update);
}
static void check_aliased_updates(struct command *commands)
@@ -745,7 +745,9 @@ static void set_connectivity_errors(struct command
Re: [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs
Ronnie Sahlberg wrote:
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,7 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static struct string_list error_strings = STRING_LIST_INIT_DUP;
[...]
@@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
[...]
+ 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)) {
+
+ const char *str;
+ string_list_append(error_strings, err.buf);
+ str = error_strings.items[error_strings.nr - 1].string;
[...]
+ return str;
[...]
@@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
packet_flush(1);
sha1_array_clear(shallow);
sha1_array_clear(ref);
+ string_list_clear(error_strings, 0);
return 0;
I think it's okay to let error strings accumulate like this because
there will not be too many of them. Still I wonder, would it work to
change the convention to transfer ownership of the string to the caller?
Ultimately 'commands' is leaked so we could even avoid the strdups but
I'd rather leave it possible to clean up.
Something like this:
diff --git i/builtin/receive-pack.c w/builtin/receive-pack.c
index 13f4a63..d8ab7b2 100644
--- i/builtin/receive-pack.c
+++ w/builtin/receive-pack.c
@@ -46,7 +46,6 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
-static struct string_list error_strings = STRING_LIST_INIT_DUP;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -195,7 +194,7 @@ static void write_head_info(void)
struct command {
struct command *next;
- const char *error_string;
+ char *error_string;
unsigned int skip_update:1,
did_not_exist:1;
int index;
@@ -469,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static const char *update(struct command *cmd, struct shallow_info *si)
+static char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd-ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
@@ -589,12 +588,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
new_sha1, old_sha1, 0, 1, err) ||
ref_transaction_commit(transaction, push, err)) {
- const char *str;
- string_list_append(error_strings, err.buf);
- str = error_strings.items[error_strings.nr - 1].string;
- strbuf_release(err);
-
+ char *str = strbuf_detach(err, NULL);
ref_transaction_free(transaction);
+
rp_error(%s, str);
return str;
}
@@ -659,6 +655,9 @@ 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_alised_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);
@@ -670,7 +669,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 = broken symref;
+ cmd-error_string = xstrdup(broken symref);
return;
}
@@ -696,8 +695,9 @@ 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 = dst_cmd-error_string =
- inconsistent aliased update;
+ cmd-error_string = xstrdup(inconsistent aliased update);
+ free(dst_cmd-error_string);
+ dst_cmd-error_string = xstrdup(inconsistent aliased update);
}
static void check_aliased_updates(struct command *commands)
@@ -745,7 +745,9 @@ static void set_connectivity_errors(struct command
*commands,
if (!check_everything_connected(command_singleton_iterator,
0, singleton))
continue;
- cmd-error_string = missing
[PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/receive-pack.c | 31 ++-
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..13f4a63 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,7 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static struct string_list error_strings = STRING_LIST_INIT_DUP;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -475,7 +476,6 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd-old_sha1;
unsigned char *new_sha1 = cmd-new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, refs/) || check_refname_format(name + 5, 0)) {
@@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return NULL; /* good */
}
else {
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+
if (shallow_update si-shallow_ref[cmd-index]
update_shallow_ref(cmd, si))
return shallow error;
- lock = lock_any_ref_for_update(namespaced_name, old_sha1,
- 0, NULL);
- if (!lock) {
- rp_error(failed to lock %s, name);
- return failed to lock;
- }
- if (write_ref_sha1(lock, new_sha1, push)) {
- return failed to write; /* error() already called */
+ 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)) {
+
+ const char *str;
+ string_list_append(error_strings, err.buf);
+ str = error_strings.items[error_strings.nr - 1].string;
+ strbuf_release(err);
+
+ ref_transaction_free(transaction);
+ rp_error(%s, str);
+ return str;
}
+
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return NULL; /* good */
}
}
@@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
packet_flush(1);
sha1_array_clear(shallow);
sha1_array_clear(ref);
+ string_list_clear(error_strings, 0);
return 0;
}
--
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
