Re: [PATCH v3 5/8] fetch: refactor fetch_refs into two functions

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> [Subject: fetch: refactor fetch_refs into two functions]
>
> Refactor the fetch_refs function into a function that does the fetching
> of refs and another function that stores them.
>
> Signed-off-by: Brandon Williams <bmw...@google.com>
> ---
>  builtin/fetch.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)

It's hard to understand the context for this patch based on this
description alone.  E.g. is fetch_refs too long to follow?  Or are we
about to expand it?  Or are we going to use the factored-out subpart
for something else?

[...]
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, 
> struct ref *ref_map)
>       int ret = quickfetch(ref_map);
>       if (ret)
>               ret = transport_fetch_refs(transport, ref_map);
> -     if (!ret)
> -             ret |= store_updated_refs(transport->url,
> -                             transport->remote->name,
> -                             ref_map);
> +     if (ret)
> +             transport_unlock_pack(transport);
> +     return ret;
> +}

Paraphrasing the old code:

        try quickfetch
        if that fails, we have to fetch for real

        both of the above "lock" a pack using a .keep file to
        avoid races

        if the fetch succeeded, now update the refs

        finally, "unlock" the pack by rm-ing the .keep file

Paraphrasing the new code:

        try quickfetch
        if that fails, we have to fetch for real

        both of the above "lock" a pack using a .keep file to
        avoid races

        if the fetch failed, "unlock" the pack by rm-ing the
        .keep file.

Do I understand correctly that this is preparation for changing the
'update refs' step?

As a minor nit, I think this would be easier to read if we treat
the unlock_pack as a destructor.  Something like this:

        int ret = quickfetch(ref_map);
        if (ret)
                ret = transport_fetch_refs(transport, ref_map);
        if (!ret)
                /*
                 * Keep the new pack's ".keep" file around to allow
                 * the caller time to update refs to reference the new
                 * objects.
                 */
                return 0;
        transport_unlock_pack(transport);
        return ret;

> +
> +static int consume_refs(struct transport *transport, struct ref *ref_map)

The name consume_refs doesn't make it clear to me what this function
is going to do.  Maybe a function comment can help.

I.e. either the name or a comment would tell me that this is going to
update local refs based on the ref values that the remote end told us.

The rest of the patch looks good.

Thanks and hope that helps,
Jonathan