Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

[Jonathan Nieder <jrnieder@xxxxxxxxx>]

Hi,

Christian Couder wrote:
 
> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx>
Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
---
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);
--
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