On Fri, May 31, 2024 at 08:27:13AM -0700, Junio C Hamano wrote:
> I wonder if we can do something to separate these two concerns
> apart, using a trick similar to what we often use with an extra
> variable "to_free". Doing so would bloat the refspec_item, but
> unlike the references themselves, there won't be thousands of them,
> so it may not be an issue, perhaps?
I had a similar thought while looking at this spot a while ago, so I dug
this attempt out of my stash. It's quite ugly, as you need to keep the
storage pointer and the const pointer in sync. Especially because
there's a lot of clever pointer indirection via match_name_with_pattern().
So I don't think it's the best way to refactor this code, but I present
it here for your amusement/disgust. This is on top of what you have
queued in ps/no-writable-strings.
---
branch.c | 6 +++---
builtin/fetch.c | 8 ++++----
builtin/remote.c | 6 +++---
checkout.c | 13 +++++++------
refspec.c | 6 ++++--
refspec.h | 6 ++++--
remote.c | 30 +++++++++++++++++-------------
transport.c | 2 +-
8 files changed, 43 insertions(+), 34 deletions(-)
diff --git a/branch.c b/branch.c
index df5d24fec6..e1585b10c2 100644
--- a/branch.c
+++ b/branch.c
@@ -38,7 +38,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
if (!remote_find_tracking(remote, &tracking->spec)) {
switch (++tracking->matches) {
case 1:
- string_list_append_nodup(tracking->srcs, tracking->spec.src);
+ string_list_append_nodup(tracking->srcs, tracking->spec.src_storage);
tracking->remote = remote->name;
break;
case 2:
@@ -47,7 +47,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
/* fall through */
default:
string_list_append(&ftb->ambiguous_remotes, remote->name);
- free(tracking->spec.src);
+ free(tracking->spec.src_storage);
string_list_clear(tracking->srcs, 0);
break;
}
@@ -489,7 +489,7 @@ static int check_tracking_branch(struct remote *remote, void *cb_data)
memset(&query, 0, sizeof(struct refspec_item));
query.dst = tracking_branch;
res = !remote_find_tracking(remote, &query);
- free(query.src);
+ free(query.src_storage);
return res;
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 06b60867f5..00a67c99ef 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -443,7 +443,7 @@ static void filter_prefetch_refspec(struct refspec *rs)
for (i = 0; i < rs->nr; i++) {
struct strbuf new_dst = STRBUF_INIT;
- char *old_dst;
+ const char *old_dst;
const char *sub = NULL;
if (rs->items[i].negative)
@@ -454,8 +454,8 @@ static void filter_prefetch_refspec(struct refspec *rs)
ref_namespace[NAMESPACE_TAGS].ref))) {
int j;
- free(rs->items[i].src);
- free(rs->items[i].dst);
+ free(rs->items[i].src_storage);
+ free(rs->items[i].dst_storage);
for (j = i + 1; j < rs->nr; j++) {
rs->items[j - 1] = rs->items[j];
@@ -481,7 +481,7 @@ static void filter_prefetch_refspec(struct refspec *rs)
rs->items[i].dst = strbuf_detach(&new_dst, NULL);
rs->items[i].force = 1;
- free(old_dst);
+ free(rs->items[i].dst_storage);
}
}
diff --git a/builtin/remote.c b/builtin/remote.c
index b44f580b8c..87b97f5ef1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -496,8 +496,8 @@ static int get_head_names(const struct ref *remote_refs, struct ref_states *stat
struct refspec_item refspec = {
.force = 0,
.pattern = 1,
- .src = (char *) "refs/heads/*",
- .dst = (char *) "refs/heads/*",
+ .src = "refs/heads/*",
+ .dst = "refs/heads/*",
};
get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0);
@@ -981,7 +981,7 @@ static int append_ref_to_tracked_list(const char *refname,
return 0;
memset(&refspec, 0, sizeof(refspec));
- refspec.dst = (char *)refname;
+ refspec.dst = refname;
if (!remote_find_tracking(states->remote, &refspec))
string_list_append(&states->tracked, abbrev_branch(refspec.src));
diff --git a/checkout.c b/checkout.c
index cfaea4bd10..327ab5f6e3 100644
--- a/checkout.c
+++ b/checkout.c
@@ -8,7 +8,7 @@
#include "strbuf.h"
struct tracking_name_data {
- /* const */ char *src_ref;
+ const char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
int num_matches;
@@ -27,7 +27,7 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
query.src = cb->src_ref;
if (remote_find_tracking(remote, &query) ||
repo_get_oid(the_repository, query.dst, cb->dst_oid)) {
- free(query.dst);
+ free(query.dst_storage);
return 0;
}
cb->num_matches++;
@@ -38,10 +38,10 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
cb->default_dst_oid = dst;
}
if (cb->dst_ref) {
- free(query.dst);
+ free(query.dst_storage);
return 0;
}
- cb->dst_ref = query.dst;
+ cb->dst_ref = query.dst_storage;
return 0;
}
@@ -50,14 +50,15 @@ char *unique_tracking_name(const char *name, struct object_id *oid,
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
const char *default_remote = NULL;
+ char *src_ref;
if (!git_config_get_string_tmp("checkout.defaultremote", &default_remote))
cb_data.default_remote = default_remote;
- cb_data.src_ref = xstrfmt("refs/heads/%s", name);
+ cb_data.src_ref = src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
if (dwim_remotes_matched)
*dwim_remotes_matched = cb_data.num_matches;
- free(cb_data.src_ref);
+ free(src_ref);
if (cb_data.num_matches == 1) {
free(cb_data.default_dst_ref);
free(cb_data.default_dst_oid);
diff --git a/refspec.c b/refspec.c
index 1df5de6c2f..a7473c9628 100644
--- a/refspec.c
+++ b/refspec.c
@@ -163,8 +163,10 @@ void refspec_item_init_or_die(struct refspec_item *item, const char *refspec,
void refspec_item_clear(struct refspec_item *item)
{
- FREE_AND_NULL(item->src);
- FREE_AND_NULL(item->dst);
+ FREE_AND_NULL(item->src_storage);
+ item->src = NULL;
+ FREE_AND_NULL(item->dst_storage);
+ item->dst = NULL;
item->force = 0;
item->pattern = 0;
item->matching = 0;
diff --git a/refspec.h b/refspec.h
index 754be45cee..fbc4352397 100644
--- a/refspec.h
+++ b/refspec.h
@@ -24,8 +24,10 @@ struct refspec_item {
unsigned exact_sha1 : 1;
unsigned negative : 1;
- char *src;
- char *dst;
+ const char *src;
+ const char *dst;
+ char *src_storage;
+ char *dst_storage;
};
#define REFSPEC_FETCH 1
diff --git a/remote.c b/remote.c
index d319f28757..5cdc3a17cc 100644
--- a/remote.c
+++ b/remote.c
@@ -827,7 +827,8 @@ int remote_has_url(struct remote *remote, const char *url)
}
static int match_name_with_pattern(const char *key, const char *name,
- const char *value, char **result)
+ const char *value, char **result,
+ const char **result_const)
{
const char *kstar = strchr(key, '*');
size_t klen;
@@ -850,6 +851,8 @@ static int match_name_with_pattern(const char *key, const char *name,
strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen);
strbuf_addstr(&sb, vstar + 1);
*result = strbuf_detach(&sb, NULL);
+ if (result_const)
+ *result_const = *result;
}
return ret;
}
@@ -858,7 +861,7 @@ static int refspec_match(const struct refspec_item *refspec,
const char *name)
{
if (refspec->pattern)
- return match_name_with_pattern(refspec->src, name, NULL, NULL);
+ return match_name_with_pattern(refspec->src, name, NULL, NULL, NULL);
return !strcmp(refspec->src, name);
}
@@ -927,7 +930,7 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
const char *key = refspec->dst ? refspec->dst : refspec->src;
const char *value = refspec->src;
- if (match_name_with_pattern(key, needle, value, &expn_name))
+ if (match_name_with_pattern(key, needle, value, &expn_name, NULL))
string_list_append_nodup(&reversed, expn_name);
} else if (refspec->matching) {
/* For the special matching refspec, any query should match */
@@ -967,12 +970,12 @@ static void query_refspecs_multiple(struct refspec *rs,
const char *key = find_src ? refspec->dst : refspec->src;
const char *value = find_src ? refspec->src : refspec->dst;
const char *needle = find_src ? query->dst : query->src;
- char **result = find_src ? &query->src : &query->dst;
+ char **result = find_src ? &query->src_storage : &query->dst_storage;
if (!refspec->dst || refspec->negative)
continue;
if (refspec->pattern) {
- if (match_name_with_pattern(key, needle, value, result))
+ if (match_name_with_pattern(key, needle, value, result, NULL))
string_list_append_nodup(results, *result);
} else if (!strcmp(needle, key)) {
string_list_append(results, value);
@@ -985,7 +988,8 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)
int i;
int find_src = !query->src;
const char *needle = find_src ? query->dst : query->src;
- char **result = find_src ? &query->src : &query->dst;
+ char **result = find_src ? &query->src_storage : &query->dst_storage;
+ const char **result_const = find_src ? &query->src : &query->dst;
if (find_src && !query->dst)
BUG("query_refspecs: need either src or dst");
@@ -1001,12 +1005,12 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)
if (!refspec->dst || refspec->negative)
continue;
if (refspec->pattern) {
- if (match_name_with_pattern(key, needle, value, result)) {
+ if (match_name_with_pattern(key, needle, value, result, result_const)) {
query->force = refspec->force;
return 0;
}
} else if (!strcmp(needle, key)) {
- *result = xstrdup(value);
+ *result_const = *result = xstrdup(value);
query->force = refspec->force;
return 0;
}
@@ -1019,12 +1023,12 @@ char *apply_refspecs(struct refspec *rs, const char *name)
struct refspec_item query;
memset(&query, 0, sizeof(struct refspec_item));
- query.src = (char *)name;
+ query.src = name;
if (query_refspecs(rs, &query))
return NULL;
- return query.dst;
+ return query.dst_storage;
}
int remote_find_tracking(struct remote *remote, struct refspec_item *refspec)
@@ -1399,9 +1403,9 @@ static char *get_ref_match(const struct refspec *rs, const struct ref *ref,
const char *dst_side = item->dst ? item->dst : item->src;
int match;
if (direction == FROM_SRC)
- match = match_name_with_pattern(item->src, ref->name, dst_side, &name);
+ match = match_name_with_pattern(item->src, ref->name, dst_side, &name, NULL);
else
- match = match_name_with_pattern(dst_side, ref->name, item->src, &name);
+ match = match_name_with_pattern(dst_side, ref->name, item->src, &name, NULL);
if (match) {
matching_refs = i;
break;
@@ -2020,7 +2024,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
if (strchr(ref->name, '^'))
continue; /* a dereference item */
if (match_name_with_pattern(refspec->src, ref->name,
- refspec->dst, &expn_name) &&
+ refspec->dst, &expn_name, NULL) &&
!ignore_symref_update(expn_name, &scratch)) {
struct ref *cpy = copy_ref(ref);
diff --git a/transport.c b/transport.c
index 83ddea8fbc..0cedda425c 100644
--- a/transport.c
+++ b/transport.c
@@ -550,7 +550,7 @@ static void update_one_tracking_ref(struct remote *remote, char *refname,
refs_update_ref(get_main_ref_store(the_repository),
"update by push", rs.dst, new_oid,
NULL, 0, 0);
- free(rs.dst);
+ free(rs.dst_storage);
}
}
