From: Patrick Steinhardt <ps@pks•im>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail•com>
Cc: "Felipe Contreras" <felipe.contreras@gmail•com>,
git@vger•kernel.org, "Jeff King" <peff@peff•net>,
"Jonathan Nieder" <jrnieder@gmail•com>,
"Daniel Martí" <mvdan@mvdan•cc>
Subject: Re: [PATCH 1/2] Add fetch.updateHead option
Date: Wed, 5 Apr 2023 12:15:55 +0200 [thread overview]
Message-ID: <ZC1KW3oN1JgrvTfn@ncase> (raw)
In-Reply-To: <230405.86fs9evfte.gmgdl@evledraar.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4005 bytes --]
On Wed, Apr 05, 2023 at 11:16:12AM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Apr 04 2023, Felipe Contreras wrote:
[snip]
> > @@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,
> > return retcode;
> > }
> >
> > +static void update_head(int config, const struct ref *head, const struct remote *remote)
>
> Here you pass a "const struct remote".
>
> > +{
> > + char *ref, *target;
> > + const char *r;
> > + int flags;
> > +
> > + if (!head || !head->symref || !remote)
> > + return;
> > +
> > + ref = apply_refspecs((struct refspec *)&remote->fetch, "refs/heads/HEAD");
> > + target = apply_refspecs((struct refspec *)&remote->fetch, head->symref);
>
> But here we end up with this cast, as it's not const after all, we're
> modifying it.
>
> I think this sort of thing makes the code harder to read & reason about,
> and adds cast verbosity.
>
> If you want to clearly communicate that the "remote->name" and
> "remote->mirror" you're using are "const" I think a better way to do
> this is to pass those as explicit parameters to this new static helper
> function, and then just pass a "struct refspec *fetch_rs" directly.
I think the underlying problem is that `apply_refspecs()` and
transitively called functions expect the argument to be non-const even
though they never modify it.
So maybe the proper way to handle this would be to add a preparatory
patch that constifies the parameter. Something like what I've attached
to the end of this mail.
Patrick
-- >8 --
diff --git a/remote.c b/remote.c
index b04e5da338..1752c391c3 100644
--- a/remote.c
+++ b/remote.c
@@ -851,7 +851,7 @@ static int refspec_match(const struct refspec_item *refspec,
return !strcmp(refspec->src, name);
}
-int omit_name_by_refspec(const char *name, struct refspec *rs)
+int omit_name_by_refspec(const char *name, const struct refspec *rs)
{
int i;
@@ -880,7 +880,7 @@ struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs)
return ref_map;
}
-static int query_matches_negative_refspec(struct refspec *rs, struct refspec_item *query)
+static int query_matches_negative_refspec(const struct refspec *rs, struct refspec_item *query)
{
int i, matched_negative = 0;
int find_src = !query->src;
@@ -968,7 +968,7 @@ static void query_refspecs_multiple(struct refspec *rs,
}
}
-int query_refspecs(struct refspec *rs, struct refspec_item *query)
+int query_refspecs(const struct refspec *rs, struct refspec_item *query)
{
int i;
int find_src = !query->src;
@@ -1002,7 +1002,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)
return -1;
}
-char *apply_refspecs(struct refspec *rs, const char *name)
+char *apply_refspecs(const struct refspec *rs, const char *name)
{
struct refspec_item query;
diff --git a/remote.h b/remote.h
index 5b38ee20b8..cd3c1439ab 100644
--- a/remote.h
+++ b/remote.h
@@ -253,7 +253,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
* Check whether a name matches any negative refspec in rs. Returns 1 if the
* name matches at least one negative refspec, and 0 otherwise.
*/
-int omit_name_by_refspec(const char *name, struct refspec *rs);
+int omit_name_by_refspec(const char *name, const struct refspec *rs);
/*
* Remove all entries in the input list which match any negative refspec in
@@ -261,8 +261,8 @@ int omit_name_by_refspec(const char *name, struct refspec *rs);
*/
struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs);
-int query_refspecs(struct refspec *rs, struct refspec_item *query);
-char *apply_refspecs(struct refspec *rs, const char *name);
+int query_refspecs(const struct refspec *rs, struct refspec_item *query);
+char *apply_refspecs(const struct refspec *rs, const char *name);
int check_push_refs(struct ref *src, struct refspec *rs);
int match_push_refs(struct ref *src, struct ref **dst,
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-04-05 10:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-05 1:27 [PATCH 0/2] Add fetch.updateHead option Felipe Contreras
2023-04-05 1:27 ` [PATCH 1/2] " Felipe Contreras
2023-04-05 9:16 ` Ævar Arnfjörð Bjarmason
2023-04-05 10:15 ` Patrick Steinhardt [this message]
2023-04-05 14:55 ` Felipe Contreras
2023-04-06 7:33 ` Ævar Arnfjörð Bjarmason
2023-04-07 2:41 ` Felipe Contreras
2023-04-05 9:28 ` Ævar Arnfjörð Bjarmason
2023-04-05 1:27 ` [PATCH 2/2] fetch: add support for HEAD update on mirrors Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZC1KW3oN1JgrvTfn@ncase \
--to=ps@pks$(echo .)im \
--cc=avarab@gmail$(echo .)com \
--cc=felipe.contreras@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
--cc=mvdan@mvdan$(echo .)cc \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox