Daniel Barkalow <barkalow@xxxxxxxxxxxx> writes:
> ..., but I'd like to get the
> code portion out there are reviewed, at least, since I think last time,
> the patch only got as far as a discussion of how I should explain what it
> does.
I actually think that is the most important part to get it right
first. I usually do not have to read the patch text to reject
an ill-conceived idea if the description is unclear and/or
unconvincing.
I think the documentation (I removed from the quote) shows the
feature is unambiguously good.
> diff --git a/remote.c b/remote.c
> index 6b56473..59338a3 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2,6 +2,15 @@
> #include "remote.h"
> #include "refs.h"
>
> +struct host {
> + const char *name;
> +
> + const char *base;
> +
> + const char **alias;
> + int alias_nr;
> +};
Extra blank lines?
> @@ -11,9 +20,32 @@ static int allocated_branches;
> static struct branch *current_branch;
> static const char *default_remote_name;
>
> +static struct host **hosts;
> +static int allocated_hosts;
The allocation in remote.c file is unusual from the rest of git
that usually follow the technical/api-allocation-growing
convention (the comment unfortunately applies to the code we
already have there).
> +static const char *alias_url(const char *url)
> +{
> + int i, j;
> + for (i = 0; i < allocated_hosts; i++) {
> + if (!hosts[i])
> + continue;
> + for (j = 0; j < hosts[i]->alias_nr; j++) {
> + if (!prefixcmp(url, hosts[i]->alias[j])) {
> + char *ret = malloc(strlen(hosts[i]->base) -
> + strlen(hosts[i]->alias[j]) +
> + strlen(url) + 1);
> + strcpy(ret, hosts[i]->base);
> + strcat(ret, url + strlen(hosts[i]->alias[j]));
> + return ret;
> + }
First match semantics is fine during runtime but at some point
we would want a "config file lint" that points out ambiguous
aliases perhaps?
> +static struct host *make_host(const char *name, int len)
> +{
> ...
> + if (empty < 0) {
> + empty = allocated_hosts;
> + allocated_hosts += allocated_hosts ? allocated_hosts : 1;
> + hosts = xrealloc(hosts,
> + sizeof(*hosts) * allocated_hosts);
This hand-rolled allocation growing is quite different from the
rest of our codebase.
> +static void add_alias(struct host *host, const char *name)
> +{
> + int nr = host->alias_nr + 1;
> + host->alias =
> + xrealloc(host->alias, nr * sizeof(char *));
> + host->alias[nr-1] = name;
> + host->alias_nr = nr;
> +}
And this "add one-by-one" allocation, too.
> @@ -154,7 +233,7 @@ static void read_remotes_file(struct remote *remote)
>
> switch (value_list) {
> case 0:
> - add_url(remote, xstrdup(s));
> + add_url_alias(remote, xstrdup(s));
> break;
> case 1:
> add_push_refspec(remote, xstrdup(s));
> @@ -206,7 +285,7 @@ static void read_branches_file(struct remote *remote)
> } else {
> branch = "refs/heads/master";
> }
> - add_url(remote, p);
> + add_url_alias(remote, p);
> add_fetch_refspec(remote, branch);
> remote->fetch_tags = 1; /* always auto-follow */
> }
These two are logical and clean updates.
> @@ -236,6 +315,20 @@ static int handle_config(const char *key, const char *value)
> }
> return 0;
> }
> + if (!prefixcmp(key, "host.")) {
> + struct host *host;
> + name = key + 5;
> + subkey = strrchr(name, '.');
> + if (!subkey)
> + return 0;
This "ignore this entry" is good. We might later want to add
host.<var> that is not about a specific host, and we would want
to be able to read a configuration that is written for such a
future version of git.
> + host = make_host(name, subkey - name);
> + if (!value)
> + return 0;
This is not.
(1) We should barf saying "host.<this-host>.<var> configuration
should be a string", for base and rewritebase, instead of
silently ignoring such a misconfiguration;
(2) We should _not_ do such barfing for future variables that
are not base nor rewritebase, so this "return" is at a
wrong place.
So the code should do this part (perhaps with git_config_string())
first,...
> + if (!strcmp(subkey, ".base"))
> + host->base = xstrdup(value);
> + else if (!strcmp(subkey, ".rewritebase"))
> + add_alias(host, xstrdup(value));
... and then ignore anything other than host.<this-host>.{base,rewritebase}
that may mean something in future versions of git.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
