From 3809633d0adb77b02ba8cfe87578134e6a30f54d Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 18 Mar 2025 18:50:18 -0400 Subject: [PATCH 1/4] refspec: treat 'fetch' as a Boolean value Since 6d4c057859 (refspec: introduce struct refspec, 2018-05-16), we have macros called REFSPEC_FETCH and REFSPEC_PUSH. This confusingly suggests that we might introduce other modes in the future, which, while possible, is highly unlikely. But these values are treated as a Boolean, and stored in a struct field called 'fetch'. So the following: if (refspec->fetch == REFSPEC_FETCH) { ... } , and if (refspec->fetch) { ... } are equivalent. Let's avoid renaming the Boolean values "true" and "false" here and remove the two REFSPEC_ macros mentioned above. Since this value is truly a Boolean and will only ever take on a value of 0 or 1, we can declare it as a single bit unsigned field. In practice this won't shrink the size of 'struct refspec', but it more clearly indicates the intent. Note that this introduces some awkwardness like: refspec_item_init_or_die(&spec, refspec, 1); , where it's unclear what the final "1" does. This will be addressed in the following commits. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 2 +- refspec.c | 4 ++-- refspec.h | 9 +++------ remote.c | 4 ++-- transport-helper.c | 2 +- 5 files changed, 9 insertions(+), 12 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 9c4a00620a..8bbfcce729 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -738,7 +738,7 @@ static const char *get_tracking_branch(const char *remote, const char *refspec) const char *spec_src; const char *merge_branch; - refspec_item_init_or_die(&spec, refspec, REFSPEC_FETCH); + refspec_item_init_or_die(&spec, refspec, 1); spec_src = spec.src; if (!*spec_src || !strcmp(spec_src, "HEAD")) spec_src = "HEAD"; diff --git a/refspec.c b/refspec.c index c6ad515f04..db5a1c34a5 100644 --- a/refspec.c +++ b/refspec.c @@ -233,7 +233,7 @@ void refspec_clear(struct refspec *rs) int valid_fetch_refspec(const char *fetch_refspec_str) { struct refspec_item refspec; - int ret = refspec_item_init(&refspec, fetch_refspec_str, REFSPEC_FETCH); + int ret = refspec_item_init(&refspec, fetch_refspec_str, 1); refspec_item_clear(&refspec); return ret; } @@ -249,7 +249,7 @@ void refspec_ref_prefixes(const struct refspec *rs, if (item->negative) continue; - if (rs->fetch == REFSPEC_FETCH) { + if (rs->fetch) { if (item->exact_sha1) continue; prefix = item->src; diff --git a/refspec.h b/refspec.h index e2b5cc54ef..155494cd3a 100644 --- a/refspec.h +++ b/refspec.h @@ -32,11 +32,8 @@ struct refspec_item { struct string_list; -#define REFSPEC_FETCH 1 -#define REFSPEC_PUSH 0 - -#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH } -#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH } +#define REFSPEC_INIT_FETCH { .fetch = 1 } +#define REFSPEC_INIT_PUSH { .fetch = 0 } /** * An array of strings can be parsed into a struct refspec using @@ -47,7 +44,7 @@ struct refspec { int alloc; int nr; - int fetch; + unsigned fetch : 1; }; int refspec_item_init(struct refspec_item *item, const char *refspec, diff --git a/remote.c b/remote.c index e609cf5c56..addd4a9999 100644 --- a/remote.c +++ b/remote.c @@ -143,8 +143,8 @@ static struct remote *make_remote(struct remote_state *remote_state, ret->prune = -1; /* unspecified */ ret->prune_tags = -1; /* unspecified */ ret->name = xstrndup(name, len); - refspec_init(&ret->push, REFSPEC_PUSH); - refspec_init(&ret->fetch, REFSPEC_FETCH); + refspec_init(&ret->push, 0); + refspec_init(&ret->fetch, 1); string_list_init_dup(&ret->server_options); ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1, diff --git a/transport-helper.c b/transport-helper.c index d457b42550..43cd760119 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -162,7 +162,7 @@ static struct child_process *get_helper(struct transport *transport) data->helper = helper; data->no_disconnect_req = 0; - refspec_init(&data->rs, REFSPEC_FETCH); + refspec_init(&data->rs, 1); /* * Open the output as FILE* so strbuf_getline_*() family of From 0baad1f3aee508d84bf74b9670f283f8c91e55dd Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 18 Mar 2025 18:50:21 -0400 Subject: [PATCH 2/4] refspec: replace `refspec_init()` with fetch/push variants To avoid having a Boolean argument in the refspec_init() function, replace it with two variants: - `refspec_init_fetch()` - `refspec_init_push()` to codify the meaning of that Boolean into the function's name itself. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- refspec.c | 12 +++++++++--- refspec.h | 3 ++- remote.c | 4 ++-- transport-helper.c | 2 +- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/refspec.c b/refspec.c index db5a1c34a5..f6be0c54d7 100644 --- a/refspec.c +++ b/refspec.c @@ -178,10 +178,16 @@ void refspec_item_clear(struct refspec_item *item) item->exact_sha1 = 0; } -void refspec_init(struct refspec *rs, int fetch) +void refspec_init_fetch(struct refspec *rs) { - memset(rs, 0, sizeof(*rs)); - rs->fetch = fetch; + struct refspec blank = REFSPEC_INIT_FETCH; + memcpy(rs, &blank, sizeof(*rs)); +} + +void refspec_init_push(struct refspec *rs) +{ + struct refspec blank = REFSPEC_INIT_PUSH; + memcpy(rs, &blank, sizeof(*rs)); } void refspec_append(struct refspec *rs, const char *refspec) diff --git a/refspec.h b/refspec.h index 155494cd3a..7db68e56c8 100644 --- a/refspec.h +++ b/refspec.h @@ -52,7 +52,8 @@ int refspec_item_init(struct refspec_item *item, const char *refspec, void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, int fetch); void refspec_item_clear(struct refspec_item *item); -void refspec_init(struct refspec *rs, int fetch); +void refspec_init_fetch(struct refspec *rs); +void refspec_init_push(struct refspec *rs); void refspec_append(struct refspec *rs, const char *refspec); __attribute__((format (printf,2,3))) void refspec_appendf(struct refspec *rs, const char *fmt, ...); diff --git a/remote.c b/remote.c index addd4a9999..25af97a44b 100644 --- a/remote.c +++ b/remote.c @@ -143,8 +143,8 @@ static struct remote *make_remote(struct remote_state *remote_state, ret->prune = -1; /* unspecified */ ret->prune_tags = -1; /* unspecified */ ret->name = xstrndup(name, len); - refspec_init(&ret->push, 0); - refspec_init(&ret->fetch, 1); + refspec_init_push(&ret->push); + refspec_init_fetch(&ret->fetch); string_list_init_dup(&ret->server_options); ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1, diff --git a/transport-helper.c b/transport-helper.c index 43cd760119..69391ee7d2 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -162,7 +162,7 @@ static struct child_process *get_helper(struct transport *transport) data->helper = helper; data->no_disconnect_req = 0; - refspec_init(&data->rs, 1); + refspec_init_fetch(&data->rs); /* * Open the output as FILE* so strbuf_getline_*() family of From ec6829e4849feb7b0343940e00896055027b06eb Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 18 Mar 2025 18:50:24 -0400 Subject: [PATCH 3/4] refspec: remove refspec_item_init_or_die() There are two callers of this function, which ensures that a dispatched call to refspec_item_init() does not fail. In the following commit, we're going to add fetch/push-specific variants of refspec_item_init(), which will turn one function into two. To avoid introducing yet another pair of new functions (such as refspec_item_init_push_or_die() and refspec_item_init_fetch_or_die()), let's remove the thin wrapper entirely. This duplicates a single line of code among two callers, but thins the refspec.h API by one function, and prevents introducing two more in the following commit. Note that we still have a trailing Boolean argument in the function `refspec_item_init()`. The following commit will address this. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 3 ++- refspec.c | 10 ++-------- refspec.h | 2 -- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 8bbfcce729..a68a9955de 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -738,7 +738,8 @@ static const char *get_tracking_branch(const char *remote, const char *refspec) const char *spec_src; const char *merge_branch; - refspec_item_init_or_die(&spec, refspec, 1); + if (!refspec_item_init(&spec, refspec, 1)) + die(_("invalid refspec '%s'"), refspec); spec_src = spec.src; if (!*spec_src || !strcmp(spec_src, "HEAD")) spec_src = "HEAD"; diff --git a/refspec.c b/refspec.c index f6be0c54d7..3aeb697505 100644 --- a/refspec.c +++ b/refspec.c @@ -160,13 +160,6 @@ int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch) return parse_refspec(item, refspec, fetch); } -void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, - int fetch) -{ - if (!refspec_item_init(item, refspec, fetch)) - die(_("invalid refspec '%s'"), refspec); -} - void refspec_item_clear(struct refspec_item *item) { FREE_AND_NULL(item->src); @@ -194,7 +187,8 @@ void refspec_append(struct refspec *rs, const char *refspec) { struct refspec_item item; - refspec_item_init_or_die(&item, refspec, rs->fetch); + if (!refspec_item_init(&item, refspec, rs->fetch)) + die(_("invalid refspec '%s'"), refspec); ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc); rs->items[rs->nr] = item; diff --git a/refspec.h b/refspec.h index 7db68e56c8..614f34554e 100644 --- a/refspec.h +++ b/refspec.h @@ -49,8 +49,6 @@ struct refspec { int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch); -void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, - int fetch); void refspec_item_clear(struct refspec_item *item); void refspec_init_fetch(struct refspec *rs); void refspec_init_push(struct refspec *rs); From 459e54b5497b53f298fe9164112f9bcb33bedb8d Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 18 Mar 2025 18:50:27 -0400 Subject: [PATCH 4/4] refspec: replace `refspec_item_init()` with fetch/push variants For similar reasons as in the previous refactoring of `refspec_init()` into `refspec_init_fetch()` and `refspec_init_push()`, apply the same refactoring to `refspec_item_init()`. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- builtin/pull.c | 2 +- refspec.c | 22 +++++++++++++++++++--- refspec.h | 4 ++-- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 02af505469..9830c09011 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -586,7 +586,7 @@ static struct ref *get_ref_map(struct remote *remote, struct refspec_item tag_refspec; /* also fetch all tags */ - refspec_item_init(&tag_refspec, TAG_REFSPEC, 0); + refspec_item_init_push(&tag_refspec, TAG_REFSPEC); get_fetch_map(remote_refs, &tag_refspec, &tail, 0); refspec_item_clear(&tag_refspec); } else if (tags == TAGS_DEFAULT && *autotags) { diff --git a/builtin/pull.c b/builtin/pull.c index a68a9955de..a1ebc6ad33 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -738,7 +738,7 @@ static const char *get_tracking_branch(const char *remote, const char *refspec) const char *spec_src; const char *merge_branch; - if (!refspec_item_init(&spec, refspec, 1)) + if (!refspec_item_init_fetch(&spec, refspec)) die(_("invalid refspec '%s'"), refspec); spec_src = spec.src; if (!*spec_src || !strcmp(spec_src, "HEAD")) diff --git a/refspec.c b/refspec.c index 3aeb697505..0775358d96 100644 --- a/refspec.c +++ b/refspec.c @@ -153,13 +153,24 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet return 1; } -int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch) +static int refspec_item_init(struct refspec_item *item, const char *refspec, + int fetch) { memset(item, 0, sizeof(*item)); item->raw = xstrdup(refspec); return parse_refspec(item, refspec, fetch); } +int refspec_item_init_fetch(struct refspec_item *item, const char *refspec) +{ + return refspec_item_init(item, refspec, 1); +} + +int refspec_item_init_push(struct refspec_item *item, const char *refspec) +{ + return refspec_item_init(item, refspec, 0); +} + void refspec_item_clear(struct refspec_item *item) { FREE_AND_NULL(item->src); @@ -186,8 +197,13 @@ void refspec_init_push(struct refspec *rs) void refspec_append(struct refspec *rs, const char *refspec) { struct refspec_item item; + int ret; - if (!refspec_item_init(&item, refspec, rs->fetch)) + if (rs->fetch) + ret = refspec_item_init_fetch(&item, refspec); + else + ret = refspec_item_init_push(&item, refspec); + if (!ret) die(_("invalid refspec '%s'"), refspec); ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc); @@ -233,7 +249,7 @@ void refspec_clear(struct refspec *rs) int valid_fetch_refspec(const char *fetch_refspec_str) { struct refspec_item refspec; - int ret = refspec_item_init(&refspec, fetch_refspec_str, 1); + int ret = refspec_item_init_fetch(&refspec, fetch_refspec_str); refspec_item_clear(&refspec); return ret; } diff --git a/refspec.h b/refspec.h index 614f34554e..8b04f9995e 100644 --- a/refspec.h +++ b/refspec.h @@ -47,8 +47,8 @@ struct refspec { unsigned fetch : 1; }; -int refspec_item_init(struct refspec_item *item, const char *refspec, - int fetch); +int refspec_item_init_fetch(struct refspec_item *item, const char *refspec); +int refspec_item_init_push(struct refspec_item *item, const char *refspec); void refspec_item_clear(struct refspec_item *item); void refspec_init_fetch(struct refspec *rs); void refspec_init_push(struct refspec *rs);