Re: [PATCH 2/5] Add remote functions

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > Make a struct and a set of functions to handle the parsing of remote
> > configurations (branches files, remotes files, and config sections),
> > and do some simple operations on lists of refspecs in the struct.
> >
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> 
> I do not have any objection to making a generally useful library
> function to deal with branches/remotes/config, and this is
> probably a good start.  Eventually we would want to do a built-in
> git-remote command that supports interactive editing of remote
> information, not just fetch/push in C.
> 
> But I do not think I can apply this patch as is, even for 'pu'.
> Comments below.
> 
> > diff --git a/remote.c b/remote.c
> > new file mode 100644
> > index 0000000..58e6a96
> > --- /dev/null
> > +++ b/remote.c
> > @@ -0,0 +1,241 @@
> > +#include "cache.h"
> > +#include "remote.h"
> > +
> > +#define MAX_REMOTES 16
> > +
> > +static struct remote *remotes[MAX_REMOTES];
> 
> I do not think the limitation of MAX_URI=16 in the original is
> unreasonable, but this MAX_REMOTES is way too low, if we want to
> eventually support somebody like Andrew, who has to deal with a
> few dozen git trees.

> > +static void add_uri(struct remote *remote, const char *uri)
> > +{
> > +	int i;
> > +	for (i = 0; i < MAX_REMOTE_URI; i++) {
> > +		if (!remote->uri[i]) {
> > +			remote->uri[i] = uri;
> > +			return;
> > +		}
> > +	}
> > +	error("ignoring excess uri");
> > +}
> 
> Why not do this just like refspecs, as an array of URIs?  Doing
> so would also lift the MAX_REMOTE_URI limitation.  Although more
> than one URIs per remote is not a norm but an exception (and
> makes sense only for push), it would shrink the base structure,
> and probably make the code simpler.

> > +
> > +static struct remote *make_remote(const char *name, int len)
> > +{
> > +	int i, empty = -1;
> > +
> > +	for (i = 0; i < MAX_REMOTES; i++) {
> > +		if (!remotes[i]) {
> > +			if (empty < 0)
> > +				empty = i;
> > +		} else {
> > +			if (len ? !strncmp(name, remotes[i]->name, len) :
> > +			    !strcmp(name, remotes[i]->name))
> > +				return remotes[i];
> > +		}
> 
> If you have "remote.foobar.url = /git/repo.git", this is called
> with "foobar.url" in name and 6 in len.  Do you want to match an
> existing remote whose name is "foobarbaz"?

> > +static void read_remotes_file(struct remote *remote)
> > +{
> > +	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
> > +
> > +	if (!f)
> > +		return;
> > +	while (fgets(buffer, BUF_SIZE, f)) {
> > +		int is_refspec;
> 
> The original dealt with URL or refspec, so "is_refspec" made
> some sense.  It isn't anymore as you differentiate between push
> and fetch refspecs.

> > +static void read_branches_file(struct remote *remote)
> > +{
> > +	const char *slash = strchr(remote->name, '/');
> > +	int n = slash ? slash - remote->name : 1000;
> > +	FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
> > +	char *s, *p;
> > +	int len;
> 
> I know you inherited this from builtin-push.c, but can't we
> clean up this magic 1000 somehow, please?

> > +static const char *default_remote_name = NULL;
> > +static const char *current_branch = NULL;
> > +static int current_branch_len = 0;
> > +
> > +static int handle_config(const char *key, const char *value)
> > +{
> > +	const char *name;
> > +	const char *subkey;
> > +	struct remote *remote;
> > +	if (!prefixcmp(key, "branch.") && current_branch &&
> > +	    !strncmp(key + 7, current_branch, current_branch_len) &&
> > +	    !strcmp(key + 7 + current_branch_len, ".remote")) {
> > +		default_remote_name = xstrdup(value);
> > +	}
> 
> Who sets current_branch to non-NULL value?  It is static and I
> do not see anybody setting it in this file.

> > +	if (prefixcmp(key,  "remote."))
> > +		return 0;
> > +	name = key + 7;
> > +	subkey = strchr(name, '.');
> > +	if (!subkey)
> > +		return error("Config with no key for remote %s", name);
> 
> This is not right.
> 
> 	[section "section name"]
>                  variable = value
> 
> allows dot in "section name" part, so you would want strrchr()
> to find out the end of it, not the first dot with strchr().  You
> found an input error (missing subkey) if strrchr() finds the dot
> at the end of "remote." (i.e. subkey == key + 6).

> > +char *remote_fetch_to(struct remote *remote, const char *ref)
> > +{
> > +	int i;
> > +	for (i = 0; i < remote->fetch_refspec_nr; i++) {
> > +		const char *refspec = remote->fetch_refspec[i];
> > +		const char *cons = strchr(refspec, ':');
> > +		const char *glob = strchr(refspec, '*');
> > +		if (*refspec == '+')
> > +			refspec++;
> 
> I cannot offhand tell what this function is about...
> 
> Would there be callers who are interested in finding out if
> refspec is forcing or not?
> 
> You are not protecting yourself from refspecs without tracking
> branch (i.e. sans any colon) and would segfault in the rest of
> this function.