Re: [PATCHv2 0/4] Some cleanups

2016-03-30 Thread 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
--
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: [PATCHv2 0/4] Some cleanups

2016-03-30 Thread Stefan Beller 
On Wed, Mar 30, 2016 at 10:25 AM, Jeff King  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
--
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: [PATCHv2 0/4] Some cleanups

2016-03-30 Thread 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
--
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