Re: [PATCH 00/35] refactoring refspecs

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 15, 2018, 08:39 UTC
Message-ID: <87h8n92tzb.fsf@evledraar.gmail.com>
In-Reply-To: <20180514215626.164960-1-bmwill@google.com>

On Mon, May 14 2018, Brandon Williams wrote:

Show 13 quoted lines

> When working on protocol v2 I noticed that working with refspecs was a
> little difficult because of the various api's that existed.  Some
> functions expected an array of "const char *" while others expected an
> array of "struct refspec".  In all cases a length parameter needed to be
> passed as a parameter as well.  This makes working with refspecs a
> little challenging because of the different expectations different parts
> of the code base have.
>
> This series refactors how refspecs are handled through out the code base
> by renaming the existing struct refspec to refspec_item and introducing
> a new 'struct refspec' which is a container of refspec_items, much like
> how a pathspec contains pathspec_items.  This simplifies many callers
> and makes handling pathspecs a bit easier.

This looks really good to me. The API you're replacing is one of the worst I've had a chance to encounter in git.git (as noted in my https://public-inbox.org/git/87in7p2ucb.fsf@evledraar.gmail.com/ but maybe I haven't looked widely enough), and now it's really nice.

> I have some follow on work which I'll build on top of this series, but
> since this was already getting a bit lengthy at 35 patches I'll save
> that for another time.

In addition to my other suggestions for stuff to put on top, which I see now you may have just had in your local tree but didn't submit, I think this makes sense:

    diff --git a/remote.c b/remote.c
    index 946b95d18d..cb97e662e8 100644
    --- a/remote.c
    +++ b/remote.c
    @@ -77,16 +77,6 @@ static const char *alias_url(const char *url, struct rewrites *r)
     	return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
     }

    -static void add_push_refspec(struct remote *remote, const char *ref)
    -{
    -	refspec_append(&remote->push, ref);
    -}
    -
    -static void add_fetch_refspec(struct remote *remote, const char *ref)
    -{
    -	refspec_append(&remote->fetch, ref);
    -}
    -
     static void add_url(struct remote *remote, const char *url)
     {
     	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
    @@ -261,9 +251,9 @@ static void read_remotes_file(struct remote *remote)
     		if (skip_prefix(buf.buf, "URL:", &v))
     			add_url_alias(remote, xstrdup(skip_spaces(v)));
     		else if (skip_prefix(buf.buf, "Push:", &v))
    -			add_push_refspec(remote, xstrdup(skip_spaces(v)));
    +			refspec_append(&remote->push, xstrdup(skip_spaces(v)));
     		else if (skip_prefix(buf.buf, "Pull:", &v))
    -			add_fetch_refspec(remote, xstrdup(skip_spaces(v)));
    +			refspec_append(&remote->fetch, xstrdup(skip_spaces(v)));
     	}
     	strbuf_release(&buf);
     	fclose(f);
    @@ -302,14 +292,14 @@ static void read_branches_file(struct remote *remote)
     		frag = "master";

     	add_url_alias(remote, strbuf_detach(&buf, NULL));
    -	add_fetch_refspec(remote, xstrfmt("refs/heads/%s:refs/heads/%s",
    -					  frag, remote->name));
    +	refspec_append(&remote->fetch, xstrfmt("refs/heads/%s:refs/heads/%s",
    +					       frag, remote->name));

     	/*
     	 * Cogito compatible push: push current HEAD to remote #branch
     	 * (master if missing)
     	 */
    -	add_push_refspec(remote, xstrfmt("HEAD:refs/heads/%s", frag));
    +	refspec_append(&remote->push, xstrfmt("HEAD:refs/heads/%s", frag));
     	remote->fetch_tags = 1; /* always auto-follow */
     }

    @@ -395,12 +385,12 @@ static int handle_config(const char *key, const char *value, void *cb)
     		const char *v;
     		if (git_config_string(&v, key, value))
     			return -1;
    -		add_push_refspec(remote, v);
    +		refspec_append(&remote->push, v);
     	} else if (!strcmp(subkey, "fetch")) {
     		const char *v;
     		if (git_config_string(&v, key, value))
     			return -1;
    -		add_fetch_refspec(remote, v);
    +		refspec_append(&remote->fetch, v);
     	} else if (!strcmp(subkey, "receivepack")) {
     		const char *v;
     		if (git_config_string(&v, key, value))

I.e. the reason we have add_{push,fetch}_refspec() in the first place is because without your series it's tricky to add new ones, but now it's trivial, so let's not leave behind wrapper static functions whose sole purpose is to just call another exported API as-is.

I've pushed all the patches I quoted inline in this review at avar-bwill/refspec in github.com/avar/git, consider them all signed-off, and depending on whether you agree/disagree etc. please squash them/adapt them/drop them however you see fit.