Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 22, 2012, 14:17 UTC
- Message-ID
- <20120522141735.GA6660@burratino>
- In-Reply-To
- <20120521145610.1911.61154.chriscool@tuxfamily.org>
Hi,
Christian Couder wrote:
Show 14 quoted lines
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> sequencer.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 762c527..ad1bbea 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -741,6 +741,7 @@ static void save_head(const char *head)
> die_errno(_("Could not write to %s"), head_file);
> if (commit_lock_file(&head_lock) < 0)
> die(_("Error wrapping up %s."), head_file);
> + strbuf_release(&buf);Makes good sense. Actually, I'm not sure why this allocation is needed in the first place. Would something like the following work?
-- >8 -- Subject: sequencer: avoid a one-time leak in save_head()
This function uses the lockfile API to make its change to the .git/sequencer/head file effectively atomic, so there is no need to gather output intended for that file in a new buffer to write it in one go.
Noticed with valgrind.
Reported-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- By the way, the lockfile API usage looks fishy. What kind of races is the locking meant to prevent? What happens if someone tries "git cherry-pick --abort" in another window after my multipick has started and before it finishes?
sequencer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git i/sequencer.c w/sequencer.c index 3c384b94..75b6a995 100644 --- i/sequencer.c +++ w/sequencer.c @@ -732,12 +732,11 @@ static void save_head(const char *head) { const char *head_file = git_path(SEQ_HEAD_FILE); static struct lock_file head_lock; - struct strbuf buf = STRBUF_INIT; int fd; fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR); - strbuf_addf(&buf, "%s\n", head); - if (write_in_full(fd, buf.buf, buf.len) < 0) + if (write_in_full(fd, head, strlen(head)) < 0 || + write_in_full(fd, "\n", 1) < 0) die_errno(_("Could not write to %s"), head_file); if (commit_lock_file(&head_lock) < 0) die(_("Error wrapping up %s."), head_file);