[PATCHv2 0/4] Some cleanups

[PATCHv2 0/4] Some cleanups

[Stefan Beller]

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

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

[Stefan Beller]

`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

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

[Eric Sunshine]

On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller <sbeller@google.com> wrote:
> `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.

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

[Junio C Hamano]

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.

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

[Stefan Beller]

On Wed, Mar 30, 2016 at 2:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Stefan Beller]

`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

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

[Eric Sunshine]

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.

> 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);
>  }

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

[Stefan Beller]

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

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

[Jeff King]

On Wed, Mar 30, 2016 at 10:05:17AM -0700, Stefan Beller wrote:

> 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

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

[Eric Sunshine]

On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller <sbeller@google.com> wrote:
> 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?

> +               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;
>  }

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

[Stefan Beller]

On Wed, Mar 30, 2016 at 10:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> -       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.

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

[Philip Oakley]

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.

> ---
> 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
> 

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

[Stefan Beller]

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 :(

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

[Philip Oakley]

From: "Stefan Beller" <sbeller@google.com>
> 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 

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

[Stefan Beller]

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

Re: [PATCHv2 0/4] Some cleanups

[Jeff King]

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.

-Peff

Re: [PATCHv2 0/4] Some cleanups

[Stefan Beller]

On Wed, Mar 30, 2016 at 10:25 AM, Jeff King <peff@peff.net> wrote:
> 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

Re: [PATCHv2 0/4] Some cleanups

[Jeff King]

On Wed, Mar 30, 2016 at 10:32:40AM -0700, Stefan Beller wrote:

> > 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