Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 20, 2014 at 10:10 AM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> If we don't pass in the error string buffer, we skip over all
> parts dealing with preparing error messages.
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
>
> This goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs
> if that makes sense.
>
>  refs.c | 8 ++++----
>  refs.h | 1 -
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index ebcd90f..3c85ea6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2646,8 +2646,6 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
>         struct string_list_item *ref_to_delete;
>         int ret, needs_repacking = 0, removed = 0;
>
> -       assert(err);
> -
>         /* Look for a packed ref */
>         for_each_string_list_item(ref_to_delete, without) {
>                 if (get_packed_ref(ref_to_delete->string)) {
> @@ -2661,7 +2659,9 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
>                 return 0;
>
>         if (lock_packed_refs(0)) {
> -               unable_to_lock_message(git_path("packed-refs"), errno, err);
> +               if (err)
> +                       unable_to_lock_message(git_path("packed-refs"),
> +                                              errno, err);
>                 return -1;
>         }
>         packed = get_packed_refs(&ref_cache);
> @@ -2688,7 +2688,7 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
>
>         /* Write what remains */
>         ret = commit_packed_refs();
> -       if (ret)
> +       if (ret && err)
>                 strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
>                             strerror(errno));
>         return ret;
> diff --git a/refs.h b/refs.h
> index c7323ff..b71fb79 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);
>   * strbuf.
>   *
>   * The refs in 'without' may have any order.
> - * The err buffer must not be omitted.
>   */
>  extern int repack_without_refs(struct string_list *without, struct strbuf *err);
>
> --
> 2.2.0.rc2.23.gca0107e
>

LGTM
Reviewed-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>

Nit:
While it does not hurt to allow passing NULL,  at some stage later
this function will become
private to refs.c and ONLY be called from within transaction_commit()
which will always
pass a non-NULL err argument.
At that stage we will not strictly need to allow err==NULL since all
callers are guaranteed to
always pass err!=NULL.

That said, having err being optional is probably a better API. Maybe
err should be made optional for all other functions that take
an err strbuf too so that the calling conventions become more consistent?
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]