From: Ronnie Sahlberg <sahlberg@google.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs
Date: Wed, 11 Jun 2014 11:44:41 -0700 [thread overview]
Message-ID: <CAL=YDWn8gFep1Hhp2gnQsd4PCuRT+vBb6wBcsbL9Sjriyf-ePw@mail.gmail.com> (raw)
In-Reply-To: <20140610231841.GC8557@google.com>
Thanks.
Done.
I added a function to stop leaking commands too.
On Tue, Jun 10, 2014 at 4:18 PM, Jonathan Nieder <jrnieder@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 *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;
> }
next prev parent reply other threads:[~2014-06-11 18:44 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-06 22:28 [PATCH v14 00/40] Use ref transactions Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 01/40] refs.c: remove ref_transaction_rollback Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 02/40] refs.c: ref_transaction_commit should not free the transaction Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 03/40] refs.c: constify the sha arguments for ref_transaction_create|delete|update Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 04/40] refs.c: allow passing NULL to ref_transaction_free Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 05/40] refs.c: add a strbuf argument to ref_transaction_commit for error logging Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs Ronnie Sahlberg
2014-06-10 20:10 ` Jonathan Nieder
2014-06-10 21:46 ` Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 07/40] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 08/40] refs.c: add an err argument to delete_ref_loose Ronnie Sahlberg
2014-06-10 22:49 ` Jonathan Nieder
2014-06-11 18:21 ` Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 09/40] refs.c: make update_ref_write update a strbuf on failure Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 10/40] update-ref: use err argument to get error from ref_transaction_commit Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 11/40] refs.c: remove the onerr argument to ref_transaction_commit Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 12/40] refs.c: change ref_transaction_update() to do error checking and return status Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 13/40] refs.c: change ref_transaction_create " Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 14/40] refs.c: update ref_transaction_delete to check for error " Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 15/40] refs.c: make ref_transaction_begin take an err argument Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 16/40] refs.c: add transaction.status and track OPEN/CLOSED/ERROR Ronnie Sahlberg
2014-06-10 22:53 ` Jonathan Nieder
2014-06-06 22:28 ` [PATCH v14 17/40] tag.c: use ref transactions when doing updates Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 18/40] replace.c: use the ref transaction functions for updates Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 19/40] commit.c: use ref transactions " Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 20/40] sequencer.c: use ref transactions for all ref updates Ronnie Sahlberg
2014-06-06 22:28 ` [PATCH v14 21/40] fast-import.c: change update_branch to use ref transactions Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 22/40] branch.c: use ref transaction for all ref updates Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 23/40] refs.c: change update_ref to use a transaction Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs Ronnie Sahlberg
2014-06-10 23:18 ` Jonathan Nieder
2014-06-11 18:44 ` Ronnie Sahlberg [this message]
2014-06-06 22:29 ` [PATCH v14 25/40] fast-import.c: use a ref transaction when dumping tags Ronnie Sahlberg
2014-06-10 23:22 ` Jonathan Nieder
2014-06-06 22:29 ` [PATCH v14 26/40] walker.c: use ref transaction for ref updates Ronnie Sahlberg
2014-06-10 23:23 ` Jonathan Nieder
2014-06-06 22:29 ` [PATCH v14 27/40] refs.c: make lock_ref_sha1 static Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 28/40] refs.c: remove the update_ref_lock function Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 29/40] refs.c: remove the update_ref_write function Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 30/40] refs.c: remove lock_ref_sha1 Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 31/40] refs.c: make prune_ref use a transaction to delete the ref Ronnie Sahlberg
2014-06-10 23:37 ` Jonathan Nieder
2014-06-06 22:29 ` [PATCH v14 32/40] refs.c: make delete_ref use a transaction Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 33/40] refs.c: pass the ref log message to _create/delete/update instead of _commit Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 34/40] refs.c: pass NULL as *flags to read_ref_full Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 35/40] refs.c: move the check for valid refname to lock_ref_sha1_basic Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 36/40] refs.c: call lock_ref_sha1_basic directly from commit Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 37/40] refs.c: pass a skip list to name_conflict_fn Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 38/40] refs.c: propagate any errno==ENOTDIR from _commit back to the callers Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 39/40] fetch.c: change s_update_ref to use a ref transaction Ronnie Sahlberg
2014-06-06 22:29 ` [PATCH v14 40/40] refs.c: make write_ref_sha1 static Ronnie Sahlberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL=YDWn8gFep1Hhp2gnQsd4PCuRT+vBb6wBcsbL9Sjriyf-ePw@mail.gmail.com' \
--to=sahlberg@google.com \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).