Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer
- From
Ronnie Sahlberg <sahlberg@google.com>
- Date
- Nov 20, 2014, 18:15 UTC
- Message-ID
- <CAL=YDWkOa4wmQdq5wOUZVQUUhnm49AVpf6gGM=5RO9Qi5dCzew@mail.gmail.com>
- In-Reply-To
- <1416507040-6576-1-git-send-email-sbeller@google.com>
On Thu, Nov 20, 2014 at 10:10 AM, Stefan Beller <sbeller@google.com> wrote:
Show 61 quoted lines
> 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@google.com>
> ---
>
> 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@google.com>
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?