Re: [PATCH 00/35] refactoring refspecs

[Brandon Williams <bmwill@google.com>]

On 05/15, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, May 14 2018, Brandon Williams wrote:
> 
> > 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.

Thanks! Yeah its one of the rougher edges I've worked with and I'm glad
I finally got around to fixing it.

> 
> > 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:

Yes these changes make sense, though I'll need to tweak them to avoid
some memory leaks.  I'll probably go ahead and squash it into the two
patches which effect those two functions.

Thanks for catching this.

> 
>     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.

-- 
Brandon Williams