threads / discuss / 41877

[PATCHv2 2/4] abbrev_sha1_in_line: don't leak memory

Subject: [PATCHv2 2/4] abbrev_sha1_in_line: don't leak memory

## tl;dr

20 messages between Mar 30, 2016 and Apr 1, 2016.

replies: 19people: 5as markdown or json

Stefan Beller· Mar 30, 2016, 17:05 UTC · lore

[PATCHv2 0/4] Some cleanups

v2: Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here is a v2.

* drop the overallocation patches (1&2)
* use git_config_get_string instead of its _const equivalent, such that
  we don't need a cast when freeing in git_config_get_notes_strategy
* Use strbuf_list_free instead of cooking our own.
* have a dedicated error exit path in bundle.c, create_bundle

v1: One of my first patches to Git were cleanup patches, and I fell back to my old pattern here, while thinking on how to write better commit messages for the submodule bugfixes I currently have in flight.

Just some one liners to not leak memory or file descriptors.
They are bundled as a series, but no patch relies on any predessor.
This applies on v2.8.0.

Thanks, Stefan

Stefan Beller (4):
  notes: don't leak memory in git_config_get_notes_strategy
  abbrev_sha1_in_line: don't leak memory
  bundle: don't leak an fd in case of early return
  credential-cache, send_request: close fd when done
 builtin/notes.c    |  5 +++--
 bundle.c           | 23 +++++++++++++++++------
 credential-cache.c |  1 +
 wt-status.c        |  4 +---
 4 files changed, 22 insertions(+), 11 deletions(-)
-- 
2.8.0.2.gb331331
Stefan Beller· Mar 30, 2016, 17:05 UTC · re: Stefan Beller · lore

[PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

`value` is just a temporary scratchpad, so we need to make sure it doesn't leak. It is xstrdup'd in `git_config_get_string` and `parse_notes_merge_strategy` just compares the string against predefined values, so no need to keep it around longer. Make `value` non-const to avoid the cast in the free.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/notes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..6fd058d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)
 static int git_config_get_notes_strategy(const char *key,
 					 enum notes_merge_strategy *strategy)
 {
-	const char *value;
+	char *value;
 
-	if (git_config_get_string_const(key, &value))
+	if (git_config_get_string(key, &value))
 		return 1;
 	if (parse_notes_merge_strategy(value, strategy))
 		git_die_config(key, "unknown notes merge strategy %s", value);
 
+	free(value);
 	return 0;
 }
 

-- 
2.8.0.2.gb331331
Eric Sunshine· Mar 30, 2016, 17:32 UTC · re: Stefan Beller · lore

Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller <sbeller@google.com> wrote:
Show 25 quoted lines
> `value` is just a temporary scratchpad, so we need to make sure it doesn't
> leak. It is xstrdup'd in `git_config_get_string` and
> `parse_notes_merge_strategy` just compares the string against predefined
> values, so no need to keep it around longer. Make `value` non-const to
> avoid the cast in the free.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)
>  static int git_config_get_notes_strategy(const char *key,
>                                          enum notes_merge_strategy *strategy)
>  {
> -       const char *value;
> +       char *value;
>
> -       if (git_config_get_string_const(key, &value))
> +       if (git_config_get_string(key, &value))
>                 return 1;
>         if (parse_notes_merge_strategy(value, strategy))
>                 git_die_config(key, "unknown notes merge strategy %s", value);
>
> +       free(value);
>         return 0;
>  }

Hmm, I thought Peff's suggestion of using git_config_get_value() was accepted as superior since it avoids the allocation altogether, thus no need for free() and no leak.

Junio C Hamano· Mar 30, 2016, 21:07 UTC · re: Eric Sunshine · lore

Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 21 quoted lines
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)
>>  static int git_config_get_notes_strategy(const char *key,
>>                                          enum notes_merge_strategy *strategy)
>>  {
>> -       const char *value;
>> +       char *value;
>>
>> -       if (git_config_get_string_const(key, &value))
>> +       if (git_config_get_string(key, &value))
>>                 return 1;
>>         if (parse_notes_merge_strategy(value, strategy))
>>                 git_die_config(key, "unknown notes merge strategy %s", value);
>>
>> +       free(value);
>>         return 0;
>>  }
>
> Hmm, I thought Peff's suggestion of using git_config_get_value() was
> accepted as superior since it avoids the allocation altogether, thus
> no need for free() and no leak.

I agree that this caller can avoid taking ownership of value by using git_config_get_value() and that would be a cleaner solution here.

This is a tangent, but am I the only one who finds that the naming of functions in config-get API is confusing? Just wondering if we should rename the ones that keeps the memory ownership to the config subsystem with s/get/peek/ or something.

Stefan Beller· Mar 30, 2016, 21:10 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

On Wed, Mar 30, 2016 at 2:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> diff --git a/builtin/notes.c b/builtin/notes.c
>>> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)
>>>  static int git_config_get_notes_strategy(const char *key,
>>>                                          enum notes_merge_strategy *strategy)
>>>  {
>>> -       const char *value;
>>> +       char *value;
>>>
>>> -       if (git_config_get_string_const(key, &value))
>>> +       if (git_config_get_string(key, &value))
>>>                 return 1;
>>>         if (parse_notes_merge_strategy(value, strategy))
>>>                 git_die_config(key, "unknown notes merge strategy %s", value);
>>>
>>> +       free(value);
>>>         return 0;
>>>  }
>>
>> Hmm, I thought Peff's suggestion of using git_config_get_value() was
>> accepted as superior since it avoids the allocation altogether, thus
>> no need for free() and no leak.
>
> I agree that this caller can avoid taking ownership of value by
> using git_config_get_value() and that would be a cleaner solution
> here.
>
> This is a tangent, but am I the only one who finds that the naming
> of functions in config-get API is confusing?  Just wondering if we
> should rename the ones that keeps the memory ownership to the config
> subsystem with s/get/peek/ or something.
>

I demonstrated the confusion and disability to read and distinguish between those names clearly already.

So I'd strongly favor a better naming for functions in config.c
Jeff King· Mar 31, 2016, 01:06 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

On Wed, Mar 30, 2016 at 02:07:14PM -0700, Junio C Hamano wrote:
> This is a tangent, but am I the only one who finds that the naming
> of functions in config-get API is confusing?  Just wondering if we
> should rename the ones that keeps the memory ownership to the config
> subsystem with s/get/peek/ or something.
You are definitely not the only one.

I think the get/peek thing would help, but I also get confused between git_config_string() and git_config_get_string().

-Peff
Junio C Hamano· Mar 31, 2016, 02:59 UTC · re: Jeff King · lore

Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> On Wed, Mar 30, 2016 at 02:07:14PM -0700, Junio C Hamano wrote:
>
>> This is a tangent, but am I the only one who finds that the naming
>> of functions in config-get API is confusing?  Just wondering if we
>> should rename the ones that keeps the memory ownership to the config
>> subsystem with s/get/peek/ or something.
>
> You are definitely not the only one.
>
> I think the get/peek thing would help, but I also get confused between
> git_config_string() and git_config_get_string().

I actually do not have problems with the ones with names that do not have "get" in.

git_config_$type() is always a helper function that are designed to be called by config callback to parse (and complain) a <var, val> pair that is expected to be of $type, and git_$family_config() is always a helper function that are designed to be used as the callback of git_config() to handle the configuration related to the named command family.

Stefan Beller· Mar 30, 2016, 17:05 UTC · re: Stefan Beller · lore

`split` is of type `struct strbuf **`, which we have a dedicated free function for, which takes care of freeing all related memory.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 wt-status.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index ef74864..1ea2ebe 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 				strbuf_addf(line, "%s", split[i]->buf);
 		}
 	}
-	for (i = 0; split[i]; i++)
-		strbuf_release(split[i]);
-
+	strbuf_list_free(split);
 }
 
 static void read_rebase_todolist(const char *fname, struct string_list *lines)

-- 
2.8.0.2.gb331331
Eric Sunshine· Mar 30, 2016, 17:35 UTC · re: Stefan Beller · lore

Re: [PATCHv2 2/4] abbrev_sha1_in_line: don't leak memory

On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller <sbeller@google.com> wrote:
> `split` is of type `struct strbuf **`, which we have a dedicated free
> function for, which takes care of freeing all related memory.

I think it's important to explain that 'split' and each split[] element were being leaked (despite the existing strbuf_release()) as justification for why this change is beneficial.

Show 19 quoted lines
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  wt-status.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ef74864..1ea2ebe 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
>                                 strbuf_addf(line, "%s", split[i]->buf);
>                 }
>         }
> -       for (i = 0; split[i]; i++)
> -               strbuf_release(split[i]);
> -
> +       strbuf_list_free(split);
>  }
Stefan Beller· Mar 30, 2016, 17:05 UTC · re: Stefan Beller · lore

[PATCHv2 3/4] bundle: don't leak an fd in case of early return

In successful operation `write_pack_data` will close the `bundle_fd`, but when we exit early, we need to take care of the file descriptor as well as the lock file ourselves. The lock file may be deleted at the end of running the program, but we are in library code, so we should not rely on that.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 bundle.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/bundle.c b/bundle.c
index 506ac49..fbc8593 100644
--- a/bundle.c
+++ b/bundle.c
@@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	int bundle_to_stdout;
 	int ref_count = 0;
 	struct rev_info revs;
+	int ret = -1;
 
 	bundle_to_stdout = !strcmp(path, "-");
 	if (bundle_to_stdout)
@@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* write prerequisites */
 	if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
-		return -1;
+		goto err;
 
 	argc = setup_revisions(argc, argv, &revs, NULL);
 
-	if (argc > 1)
-		return error(_("unrecognized argument: %s"), argv[1]);
+	if (argc > 1) {
+		ret = error(_("unrecognized argument: %s"), argv[1]);
+		goto err;
+	}
 
 	object_array_remove_duplicates(&revs.pending);
 
 	ref_count = write_bundle_refs(bundle_fd, &revs);
 	if (!ref_count)
 		die(_("Refusing to create empty bundle."));
-	else if (ref_count < 0)
-		return -1;
+	else if (ref_count < 0) {
+		if (!bundle_to_stdout)
+			close(bundle_fd);
+		goto err;
+	}
 
 	/* write pack */
 	if (write_pack_data(bundle_fd, &revs))
-		return -1;
+		goto err;
 
 	if (!bundle_to_stdout) {
 		if (commit_lock_file(&lock))
 			die_errno(_("cannot create '%s'"), path);
 	}
 	return 0;
+err:
+	if (!bundle_to_stdout)
+		close(bundle_fd);
+	rollback_lock_file(&lock);
+	return ret;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd, int flags)

-- 
2.8.0.2.gb331331
Jeff King· Mar 30, 2016, 17:23 UTC · re: Stefan Beller · lore

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

On Wed, Mar 30, 2016 at 10:05:17AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> diff --git a/bundle.c b/bundle.c
> index 506ac49..fbc8593 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const char *path,
>  	int bundle_to_stdout;
>  	int ref_count = 0;
>  	struct rev_info revs;
> +	int ret = -1;

A minor nit, but I don't think we ever put anything but "-1" in this variable. It could go away and we can just "return -1" in the "err" path.

-Peff
Eric Sunshine· Mar 30, 2016, 17:41 UTC · re: Stefan Beller · lore

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller <sbeller@google.com> wrote:
Show 36 quoted lines
> In successful operation `write_pack_data` will close the `bundle_fd`,
> but when we exit early, we need to take care of the file descriptor
> as well as the lock file ourselves. The lock file may be deleted at the
> end of running the program, but we are in library code, so we should
> not rely on that.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/bundle.c b/bundle.c
> @@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, const char *path,
>
>         /* write prerequisites */
>         if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
> -               return -1;
> +               goto err;
>
>         argc = setup_revisions(argc, argv, &revs, NULL);
>
> -       if (argc > 1)
> -               return error(_("unrecognized argument: %s"), argv[1]);
> +       if (argc > 1) {
> +               ret = error(_("unrecognized argument: %s"), argv[1]);
> +               goto err;
> +       }
>
>         object_array_remove_duplicates(&revs.pending);
>
>         ref_count = write_bundle_refs(bundle_fd, &revs);
>         if (!ref_count)
>                 die(_("Refusing to create empty bundle."));
> -       else if (ref_count < 0)
> -               return -1;
> +       else if (ref_count < 0) {
> +               if (!bundle_to_stdout)
> +                       close(bundle_fd);
Why is this close() here considering that it gets closed by the 'err' path?
Show 19 quoted lines
> +               goto err;
> +       }
>
>         /* write pack */
>         if (write_pack_data(bundle_fd, &revs))
> -               return -1;
> +               goto err;
>
>         if (!bundle_to_stdout) {
>                 if (commit_lock_file(&lock))
>                         die_errno(_("cannot create '%s'"), path);
>         }
>         return 0;
> +err:
> +       if (!bundle_to_stdout)
> +               close(bundle_fd);
> +       rollback_lock_file(&lock);
> +       return ret;
>  }
Stefan Beller· Mar 31, 2016, 17:47 UTC · re: Eric Sunshine · lore

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

On Wed, Mar 30, 2016 at 10:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 7 quoted lines
>> -       else if (ref_count < 0)
>> -               return -1;
>> +       else if (ref_count < 0) {
>> +               if (!bundle_to_stdout)
>> +                       close(bundle_fd);
>
> Why is this close() here considering that it gets closed by the 'err' path?
Thanks for pointing out; fixed in a reroll.
Philip Oakley· Mar 31, 2016, 19:00 UTC · re: Stefan Beller · lore

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

From: "Stefan Beller" <sbeller@google.com>
Show 8 quoted lines
> In successful operation `write_pack_data` will close the `bundle_fd`,
> but when we exit early, we need to take care of the file descriptor
> as well as the lock file ourselves. The lock file may be deleted at the
> end of running the program, but we are in library code, so we should
> not rely on that.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>

Has this been tested on Windows? I had a similar problem very recently https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ where a bad rev-list-arg would cause the `bundle create` to die, and, on windows, leave the incomplete bundle file locked.

dscho suggested one possible solution for that, but I haven't had any time to try any patches.

Show 73 quoted lines
> ---
> bundle.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 506ac49..fbc8593 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>  int bundle_to_stdout;
>  int ref_count = 0;
>  struct rev_info revs;
> + int ret = -1;
>
>  bundle_to_stdout = !strcmp(path, "-");
>  if (bundle_to_stdout)
> @@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, 
> const char *path,
>
>  /* write prerequisites */
>  if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
> - return -1;
> + goto err;
>
>  argc = setup_revisions(argc, argv, &revs, NULL);
>
> - if (argc > 1)
> - return error(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + ret = error(_("unrecognized argument: %s"), argv[1]);
> + goto err;
> + }
>
>  object_array_remove_duplicates(&revs.pending);
>
>  ref_count = write_bundle_refs(bundle_fd, &revs);
>  if (!ref_count)
>  die(_("Refusing to create empty bundle."));
> - else if (ref_count < 0)
> - return -1;
> + else if (ref_count < 0) {
> + if (!bundle_to_stdout)
> + close(bundle_fd);
> + goto err;
> + }
>
>  /* write pack */
>  if (write_pack_data(bundle_fd, &revs))
> - return -1;
> + goto err;
>
>  if (!bundle_to_stdout) {
>  if (commit_lock_file(&lock))
>  die_errno(_("cannot create '%s'"), path);
>  }
>  return 0;
> +err:
> + if (!bundle_to_stdout)
> + close(bundle_fd);
> + rollback_lock_file(&lock);
> + return ret;
> }
>
> int unbundle(struct bundle_header *header, int bundle_fd, int flags)
> -- 
> 2.8.0.2.gb331331
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Stefan Beller· Apr 1, 2016, 00:25 UTC · re: Philip Oakley · lore

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley <philipoakley@iee.org> wrote:
Show 19 quoted lines
> From: "Stefan Beller" <sbeller@google.com>
>>
>> In successful operation `write_pack_data` will close the `bundle_fd`,
>> but when we exit early, we need to take care of the file descriptor
>> as well as the lock file ourselves. The lock file may be deleted at the
>> end of running the program, but we are in library code, so we should
>> not rely on that.
>>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
>
> Has this been tested on Windows? I had a similar problem very recently
> https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ
> where a bad rev-list-arg would cause the `bundle create` to die, and, on
> windows, leave the incomplete bundle file locked.
>
> dscho suggested one possible solution for that, but I haven't had any time
> to try any patches.

I think with Jeffs suggestion to only rollback the lock in case of (!bundle_to_stdout) we're not making it worse. I do not have a Windows machine at hand to test it :(

Philip Oakley· Apr 1, 2016, 07:26 UTC · re: Stefan Beller · lore

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

From: "Stefan Beller" <sbeller@google.com>
Show 27 quoted lines
> On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley <philipoakley@iee.org> 
> wrote:
>> From: "Stefan Beller" <sbeller@google.com>
>>>
>>> In successful operation `write_pack_data` will close the `bundle_fd`,
>>> but when we exit early, we need to take care of the file descriptor
>>> as well as the lock file ourselves. The lock file may be deleted at the
>>> end of running the program, but we are in library code, so we should
>>> not rely on that.
>>>
>>> Helped-by: Jeff King <peff@peff.net>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>
>>
>> Has this been tested on Windows? I had a similar problem very recently
>> https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ
>> where a bad rev-list-arg would cause the `bundle create` to die, and, on
>> windows, leave the incomplete bundle file locked.
>>
>> dscho suggested one possible solution for that, but I haven't had any 
>> time
>> to try any patches.
>
> I think with Jeffs suggestion to only rollback the lock in case of
> (!bundle_to_stdout)
> we're not making it worse. I do not have a Windows machine at hand to test 
> it :(

Thant's fine. I just wanted to make folks aware of the problem so it doesn't get worse.

It looks like the cause is that the bundle.c:compute_and_write_prerequisites(), which includes parsing the arg-list (and die on bad arg), is called after the creation and locking of the bundle file descriptor and header, thus leaving it hanging on Windows.

--
Philip
Stefan Beller· Mar 30, 2016, 17:05 UTC · re: Stefan Beller · lore

[PATCHv2 4/4] credential-cache, send_request: close fd when done

No need to keep it open any further.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 credential-cache.c | 1 +
 1 file changed, 1 insertion(+)
diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..86e21de 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct strbuf *out)
 		write_or_die(1, in, r);
 		got_data = 1;
 	}
+	close(fd);
 	return got_data;
 }
 

-- 
2.8.0.2.gb331331
Jeff King· Mar 30, 2016, 17:25 UTC · re: Stefan Beller · lore

Re: [PATCHv2 0/4] Some cleanups

On Wed, Mar 30, 2016 at 10:05:14AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> v2:
> Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here
> is a v2.
> 
> * drop the overallocation patches (1&2)
> * use git_config_get_string instead of its _const equivalent, such that
>   we don't need a cast when freeing in git_config_get_notes_strategy
> * Use strbuf_list_free instead of cooking our own.
> * have a dedicated error exit path in bundle.c, create_bundle

I'm OK with all of these as-is, though I did mention a nit in the third one. I also like Junio's rewrite instead of using strbuf_list_free.

-Peff
Stefan Beller· Mar 30, 2016, 17:32 UTC · re: Jeff King · lore

Re: [PATCHv2 0/4] Some cleanups

On Wed, Mar 30, 2016 at 10:25 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Wed, Mar 30, 2016 at 10:05:14AM -0700, Stefan Beller wrote:
>
>> v2:
>> Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here
>> is a v2.
>>
>> * drop the overallocation patches (1&2)
>> * use git_config_get_string instead of its _const equivalent, such that
>>   we don't need a cast when freeing in git_config_get_notes_strategy
>> * Use strbuf_list_free instead of cooking our own.
>> * have a dedicated error exit path in bundle.c, create_bundle
>
> I'm OK with all of these as-is, though I did mention a nit in the third
> one. I also like Junio's rewrite instead of using strbuf_list_free.

I'm fine using the rewritten version instead of using strbuf_list_free. :) On the third one, there is one case, where we have

  if (..)
    return error(_(text));
and that is an exit(128); eventually.

I thought it is worth preserving the difference (as it is a faithful bug fix not a change to make it better or more uniform).

Thanks, Stefan

>
> -Peff
Jeff King· Mar 30, 2016, 17:38 UTC · re: Stefan Beller · lore

Re: [PATCHv2 0/4] Some cleanups

On Wed, Mar 30, 2016 at 10:32:40AM -0700, Stefan Beller wrote:
Show 10 quoted lines
> > I'm OK with all of these as-is, though I did mention a nit in the third
> > one. I also like Junio's rewrite instead of using strbuf_list_free.
> 
> I'm fine using the rewritten version instead of using strbuf_list_free. :)
> On the third one, there is one case, where we have
> 
>   if (..)
>     return error(_(text));
> 
> and that is an exit(128); eventually.
In the caller perhaps, but isn't that equivalent to:
  error(_(text));
  return -1;
?

I think it is OK to make assumptions about error()'s return value; that is what it is there for.

-Peff

← back to recent threads