Re: [PATCH v6 03/32] files-backend: break out ref reading

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2016, 00:21 UTC
Message-ID: <xmqqr3g1r66h.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1456354744-8022-4-git-send-email-dturner@twopensource.com>

David Turner <dturner@twopensource.com> writes:

Show 31 quoted lines

> Refactor resolve_ref_1 in terms of a new function read_raw_ref, which
> is responsible for reading ref data from the ref storage.
>
> Later, we will make read_raw_ref a pluggable backend function, and make
> resolve_ref_unsafe common.
>
> Testing done: Hacked in code to run both old and new version of
> resolve_ref_1 and compare all outputs, failing dramatically if outputs
> differed.  Ran test suite.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> Helped-by: Duy Nguyen <pclouds@gmail.com>
> ---
>  refs/files-backend.c | 265 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 159 insertions(+), 106 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index fd664d6..ef5f28d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1377,10 +1377,9 @@ static struct ref_entry *get_packed_ref(const char *refname)
>  
>  /*
>   * A loose ref file doesn't exist; check for a packed ref.  The
> - * options are forwarded from resolve_safe_unsafe().
> + * options are forwarded from resolve_ref_unsafe().
>   */
>  static int resolve_missing_loose_ref(const char *refname,
> -				     int resolve_flags,
>  				     unsigned char *sha1,
>  				     int *flags)

The last parameter must be made "unsigned int *flags" ...

> +static int read_raw_ref(const char *refname, unsigned char *sha1,
> +			struct strbuf *symref, struct strbuf *sb_path,
> +			unsigned int *flags)
>  {

... because the last parameter here is "unsigned int *flags", and you later pass the same variable to both.

> -	for (;;) {
> -		const char *path;
> +	for(;;) {

This is an unwelcome change.

> +				break;
> +			if (resolve_missing_loose_ref(refname, sha1, flags))

This line has a trailing whitespace.

> +	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
> +		int read_flags = 0;

... and this must be "unsigned read_flags = 0".

You can of course standardize on signed int, but because this is a collection of flag bits, there is no reason not to choose unsigned.

I _think_ I can fix everything up before pushing out, so please check what will appear on 'pu' before rerolling.

Thanks.