Thanks.
Done.
I added a function to stop leaking commands too.
On Tue, Jun 10, 2014 at 4:18 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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 *commands,
> if (!check_everything_connected(command_singleton_iterator,
> 0, &singleton))
> continue;
> - cmd->error_string = "missing necessary objects";
> + if (cmd->error_string) /* can't happen */
> + continue;
> + cmd->error_string = xstrdup("missing necessary objects");
> }
> }
>
> @@ -782,9 +784,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 = "deny deleting a hidden ref";
> + cmd->error_string = xstrdup("deny deleting a hidden ref");
> else
> - cmd->error_string = "deny updating a hidden ref";
> + cmd->error_string = xstrdup("deny updating a hidden ref");
> }
> }
>
> @@ -798,8 +800,11 @@ static void execute_commands(struct command *commands,
> struct iterate_data data;
>
> if (unpacker_error) {
> - for (cmd = commands; cmd; cmd = cmd->next)
> - cmd->error_string = "unpacker error";
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (cmd->error_string) /* can't happen */
> + continue;
> + cmd->error_string = xstrdup("unpacker error");
> + }
> return;
> }
>
> @@ -812,8 +817,9 @@ 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)
> - cmd->error_string = "pre-receive hook declined";
> + if (cmd->error_string)
> + continue;
> + cmd->error_string = xstrdup("pre-receive hook declined");
> }
> return;
> }
> @@ -1091,7 +1097,8 @@ static void update_shallow_info(struct command *commands,
> if (is_null_sha1(cmd->new_sha1))
> continue;
> if (ref_status[cmd->index]) {
> - cmd->error_string = "shallow update not allowed";
> + free(cmd->error_string);
> + cmd->error_string = xstrdup("shallow update not allowed");
> cmd->skip_update = 1;
> }
> }
> @@ -1227,6 +1234,5 @@ 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;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html