Re: [PATCH v4 15/27] fast-import.c: change update_branch to use ref transactions
Ronnie Sahlberg writes: > + transaction = ref_transaction_begin(); > + if ((!transaction || > + ref_transaction_update(transaction, b->name, b->sha1, old_sha1, > +0, 1)) || > + (ref_transaction_commit(transaction, msg, &err) && > + !(transaction = NULL))) { > + ref_transaction_rollback(transaction); > + return error("Unable to update branch %s: %s", b->name, > + strbuf_detach(&err, NULL)); > + } The assigning of NULL to transaction and always returning true looks very suspicious. Also, if we couldn't get transaction what happens? I think you wanted to avoid passing NULL to rollback but at the same time I suspect you wanted to fall into that "return error()" part, but apparently you are failing to have that cake and eat it too, no? -- 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 v4 15/27] fast-import.c: change update_branch to use ref transactions
On Mon, Apr 28, 2014 at 7:18 PM, Eric Sunshine wrote: > On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote: >> Change update_branch() to use ref transactions for updates. >> >> Signed-off-by: Ronnie Sahlberg >> --- >> fast-import.c | 20 >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/fast-import.c b/fast-import.c >> index fb4738d..300c8dc 100644 >> --- a/fast-import.c >> +++ b/fast-import.c >> @@ -1678,36 +1678,40 @@ found_entry: >> static int update_branch(struct branch *b) >> { >> static const char *msg = "fast-import"; >> - struct ref_lock *lock; >> + struct ref_transaction *transaction; >> unsigned char old_sha1[20]; >> + struct strbuf err = STRBUF_INIT; >> >> if (is_null_sha1(b->sha1)) >> return 0; >> if (read_ref(b->name, old_sha1)) >> hashclr(old_sha1); >> - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL); >> - if (!lock) >> - return error("Unable to lock %s", b->name); >> if (!force_update && !is_null_sha1(old_sha1)) { >> struct commit *old_cmit, *new_cmit; >> >> old_cmit = lookup_commit_reference_gently(old_sha1, 0); >> new_cmit = lookup_commit_reference_gently(b->sha1, 0); >> if (!old_cmit || !new_cmit) { >> - unlock_ref(lock); >> return error("Branch %s is missing commits.", >> b->name); >> } >> >> if (!in_merge_bases(old_cmit, new_cmit)) { >> - unlock_ref(lock); >> warning("Not updating %s" >> " (new tip %s does not contain %s)", >> b->name, sha1_to_hex(b->sha1), >> sha1_to_hex(old_sha1)); >> return -1; >> } >> } >> - if (write_ref_sha1(lock, b->sha1, msg) < 0) >> - return error("Unable to update %s", b->name); >> + transaction = ref_transaction_begin(); >> + if ((!transaction || >> + ref_transaction_update(transaction, b->name, b->sha1, old_sha1, >> + 0, 1)) || >> + (ref_transaction_commit(transaction, msg, &err) && >> +!(transaction = NULL))) { >> + ref_transaction_rollback(transaction); >> + return error("Unable to update branch %s: %s", b->name, >> +strbuf_detach(&err, NULL)); > > Iffy strbuf handling. The strbuf content is being leaked here whether > detached or not. > Thanks! I have updated this and all other patches to make sure we do a strbuf_release() any time we have added to the string. I also did a quick audit of the strbuf_detach() use in builtin/* (which I based my use on) and there seems to be quite common that strbuf_detach() is used in a way that will leak memory. I will make a note and perhaps audit all the other strbuf_detach() calls for a future patch series. >> + } >> return 0; >> } >> >> -- >> 1.9.1.528.g98b8868.dirty -- 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 v4 15/27] fast-import.c: change update_branch to use ref transactions
On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote: > Change update_branch() to use ref transactions for updates. > > Signed-off-by: Ronnie Sahlberg > --- > fast-import.c | 20 > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fast-import.c b/fast-import.c > index fb4738d..300c8dc 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -1678,36 +1678,40 @@ found_entry: > static int update_branch(struct branch *b) > { > static const char *msg = "fast-import"; > - struct ref_lock *lock; > + struct ref_transaction *transaction; > unsigned char old_sha1[20]; > + struct strbuf err = STRBUF_INIT; > > if (is_null_sha1(b->sha1)) > return 0; > if (read_ref(b->name, old_sha1)) > hashclr(old_sha1); > - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL); > - if (!lock) > - return error("Unable to lock %s", b->name); > if (!force_update && !is_null_sha1(old_sha1)) { > struct commit *old_cmit, *new_cmit; > > old_cmit = lookup_commit_reference_gently(old_sha1, 0); > new_cmit = lookup_commit_reference_gently(b->sha1, 0); > if (!old_cmit || !new_cmit) { > - unlock_ref(lock); > return error("Branch %s is missing commits.", > b->name); > } > > if (!in_merge_bases(old_cmit, new_cmit)) { > - unlock_ref(lock); > warning("Not updating %s" > " (new tip %s does not contain %s)", > b->name, sha1_to_hex(b->sha1), > sha1_to_hex(old_sha1)); > return -1; > } > } > - if (write_ref_sha1(lock, b->sha1, msg) < 0) > - return error("Unable to update %s", b->name); > + transaction = ref_transaction_begin(); > + if ((!transaction || > + ref_transaction_update(transaction, b->name, b->sha1, old_sha1, > + 0, 1)) || > + (ref_transaction_commit(transaction, msg, &err) && > +!(transaction = NULL))) { > + ref_transaction_rollback(transaction); > + return error("Unable to update branch %s: %s", b->name, > +strbuf_detach(&err, NULL)); Iffy strbuf handling. The strbuf content is being leaked here whether detached or not. > + } > return 0; > } > > -- > 1.9.1.528.g98b8868.dirty -- 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 v4 15/27] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates. Signed-off-by: Ronnie Sahlberg --- fast-import.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/fast-import.c b/fast-import.c index fb4738d..300c8dc 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1678,36 +1678,40 @@ found_entry: static int update_branch(struct branch *b) { static const char *msg = "fast-import"; - struct ref_lock *lock; + struct ref_transaction *transaction; unsigned char old_sha1[20]; + struct strbuf err = STRBUF_INIT; if (is_null_sha1(b->sha1)) return 0; if (read_ref(b->name, old_sha1)) hashclr(old_sha1); - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL); - if (!lock) - return error("Unable to lock %s", b->name); if (!force_update && !is_null_sha1(old_sha1)) { struct commit *old_cmit, *new_cmit; old_cmit = lookup_commit_reference_gently(old_sha1, 0); new_cmit = lookup_commit_reference_gently(b->sha1, 0); if (!old_cmit || !new_cmit) { - unlock_ref(lock); return error("Branch %s is missing commits.", b->name); } if (!in_merge_bases(old_cmit, new_cmit)) { - unlock_ref(lock); warning("Not updating %s" " (new tip %s does not contain %s)", b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1)); return -1; } } - if (write_ref_sha1(lock, b->sha1, msg) < 0) - return error("Unable to update %s", b->name); + transaction = ref_transaction_begin(); + if ((!transaction || + ref_transaction_update(transaction, b->name, b->sha1, old_sha1, + 0, 1)) || + (ref_transaction_commit(transaction, msg, &err) && +!(transaction = NULL))) { + ref_transaction_rollback(transaction); + return error("Unable to update branch %s: %s", b->name, +strbuf_detach(&err, NULL)); + } return 0; } -- 1.9.1.528.g98b8868.dirty -- 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