Re: [PATCH] Add support for host aliases in config files
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:14
Daniel Barkalow [off-list ref] 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.
quoted hunk
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?
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
@@ -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.