Re: [PATCH 1/2] Add fetch.updateHead option

[Patrick Steinhardt <ps@xxxxxx>]

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: signature.asc
Description: PGP signature