Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 20, 2014, 18:29 UTC
Message-ID: <20141120182901.GC15945@google.com>
In-Reply-To: <1416506666-5989-1-git-send-email-sbeller@google.com>

Stefan Beller wrote:

> Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed

Change-id snuck in.

[...]

Show 15 quoted lines

> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +/*
> + * Remove the refs listed in 'without' from the packed-refs file.
> + * On error, packed-refs will be unchanged, the return value is
> + * nonzero, and a message about the error is written to the 'err'
> + * strbuf.
> + *
> + * The refs in 'without' may have any order.

Tiny nit: this makes me wonder what the order represents --- how do I pick which order for the refs in without to have?

I think the idea is just that 'without' doesn't have to be sorted (it's a shame we don't have separate sorted string list and unsorted string list types or a string_list_sorted() helper to catch bad callers early to functions that care). One way to say that would be

	Remove the refs listed in the unsorted string list 'without' from the
	packed-refs file.  On error, [...]

> + * The err buffer must not be omitted.

s/buffer/strbuf/, or s/The err buffer/'err'/ s/omitted/NULL/

With the Change-Id dropped, and with or without the above comment nits addressed,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>