On Sat, 28 Apr 2007, Junio C Hamano wrote:

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

Sure; easy enough to fix.

quoted text
> > +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.

Again easy enough. Actually, it might be nice to have some common 
functions for arrays of pointers, since I've now got 3 nearly identical 
functions, but not too important.

quoted text
> > +
> > +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"?

Yeah, I need to test !(existing remote)[len] also.

quoted text
> > +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.

"value_list" instead.

quoted text
> > +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?

I'm a bit leary of touching this code; I don't have repositories with this 
sort of config to test against. Can I just move it and have someone how 
understands the code clean it up (before or after)?

quoted text
> > +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.

I was going to set it when I figured out how to find the right value, but 
then forgot. (This would also make "git-push" use the same remote as 
"git-pull" when the current branch has a "remote" setting, which is 
obviously less surprising, but is a change in behavior)

quoted text
> Also the original value of default_remote_name leaks here.  But
> you cannot blindly free the original value, as you could end up
> storing it in ret->uri[0] when somebody calls remote_get(NULL).

Actually, read_config() can only do anything once (since it returns with 
default_remote_name not null), so handle_config can't be called again 
after remote_get() has potentially used it. So I can definitely free it.

quoted text
> > +	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).

Just replacing strchr with strrchr does the right thing, right? (If I 
start at name (== key + 7), it won't find that wrong dot.)

quoted text
> > +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.

This is to find the local tracking ref that a remote head would be fetched 
to, taking into account patterns and multiple items in the list. I should 
probably actually be sucking this code out of builtin-fetch--tool and 
connect, which I hadn't noticed handled this in C already.

	-Daniel
*This .sig left intentionally blank*
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html