Re: [PATCH 2/5] Add remote functions
- From
Junio C Hamano <junkio@cox.net>
- Date
- Apr 29, 2007, 05:52 UTC
- Message-ID
- <7virbfg22c.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <Pine.LNX.4.64.0704281303570.28708@iabervon.org>
Daniel Barkalow <barkalow@iabervon.org> writes:
Show 5 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 11 quoted lines
> +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.
Show 14 quoted lines
> +
> +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"?
Show 8 quoted lines
> +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.
Show 7 quoted lines
> +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?
Show 14 quoted lines
> +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.
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).
Show 6 quoted lines
> + 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 = valueallows 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).
Show 9 quoted lines
> +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.
Show 17 quoted lines
> + if (glob) {
> + if (!strncmp(ref, refspec, glob - refspec)) {
> + const char *cons_glob = strchr(cons, '*');
> + char *ret =
> + xmalloc(cons_glob - cons + strlen(ref) - (glob - refspec));
> + memcpy(ret, cons + 1, cons_glob - cons - 1);
> + memcpy(ret + (cons_glob - cons) - 1,
> + ref + (glob - refspec),
> + strlen(ref) - (glob - refspec) + 1);
> + return ret;
> + }
> + } else if (!strncmp(ref, refspec, cons - refspec)) {
> + return xstrdup(cons + 1);
> + }
> + }
> + return NULL;
> +}Show 8 quoted lines
> diff --git a/remote.h b/remote.h > new file mode 100644 > index 0000000..7e881f7 > --- /dev/null > +++ b/remote.h > @@ -0,0 +1,25 @@ > +#ifndef _REMOTE_H > +#define _REMOTE_H
We do not seem to use leading underscore in most our header files. Let's be consistent (we need to fix diffcore.h and path-list.h).