[GSoC][PATCH] remote: relocate valid_remote_name

[GSoC][PATCH] remote: relocate valid_remote_name

[Meet Soni]

Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to
better align with the separation of concerns.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
Junio mentioned in [1], the `valid_remote_name` function belongs in remote
header. This patch addresses that.

[1]: https://lore.kernel.org/git/xmqqikq0ruuk.fsf@gitster.g/
 refspec.c | 10 ----------
 refspec.h |  1 -
 remote.c  | 10 ++++++++++
 remote.h  |  2 ++
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/refspec.c b/refspec.c
index 6d86e04442..83ec7d7e62 100644
--- a/refspec.c
+++ b/refspec.c
@@ -236,16 +236,6 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	return ret;
 }
 
-int valid_remote_name(const char *name)
-{
-	int result;
-	struct strbuf refspec = STRBUF_INIT;
-	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
-	result = valid_fetch_refspec(refspec.buf);
-	strbuf_release(&refspec);
-	return result;
-}
-
 void refspec_ref_prefixes(const struct refspec *rs,
 			  struct strvec *ref_prefixes)
 {
diff --git a/refspec.h b/refspec.h
index 69d693c87d..dc428f86f2 100644
--- a/refspec.h
+++ b/refspec.h
@@ -61,7 +61,6 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
 void refspec_clear(struct refspec *rs);
 
 int valid_fetch_refspec(const char *refspec);
-int valid_remote_name(const char *name);
 
 struct strvec;
 /*
diff --git a/remote.c b/remote.c
index 0f6fba8562..3d451570cb 100644
--- a/remote.c
+++ b/remote.c
@@ -3003,3 +3003,13 @@ char *relative_url(const char *remote_url, const char *url,
 	free(out);
 	return strbuf_detach(&sb, NULL);
 }
+
+int valid_remote_name(const char *name)
+{
+	int result;
+	struct strbuf refspec = STRBUF_INIT;
+	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
+	result = valid_fetch_refspec(refspec.buf);
+	strbuf_release(&refspec);
+	return result;
+}
diff --git a/remote.h b/remote.h
index bda10dd5c8..0c14d665b6 100644
--- a/remote.h
+++ b/remote.h
@@ -461,4 +461,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+int valid_remote_name(const char *name);
+
 #endif

base-commit: 58b5801aa94ad5031978f8e42c1be1230b3d352f
-- 
2.34.1

Re: [GSoC][PATCH] remote: relocate valid_remote_name

[Patrick Steinhardt]

On Tue, Feb 04, 2025 at 09:44:30AM +0530, Meet Soni wrote:
> Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to
> better align with the separation of concerns.

Nit: you don't only move the function declaration from "refspec.h" to
"remote.h", but also move its definition from "refspec.c" to "remote.c".
So you might want to instead say that you move the function between
subsystems, which would imply both moves.

The change itself looks straight-forward to me. Did you happen to check
whether this allows you to drop any includes for "refspec.h"?

Thanks!

Patrick

Re: [GSoC][PATCH] remote: relocate valid_remote_name

[Meet Soni]

On Tue, 4 Feb 2025 at 13:25, Patrick Steinhardt <ps@pks.im> wrote:

> Nit: you don't only move the function declaration from "refspec.h" to
> "remote.h", but also move its definition from "refspec.c" to "remote.c".
> So you might want to instead say that you move the function between
> subsystems, which would imply both moves.
>
Makes sense. Thanks for pointing it out.
> The change itself looks straight-forward to me. Did you happen to check
> whether this allows you to drop any includes for "refspec.h"?
>
I think you mean refspec.c, as refspec.h doesn’t have includes.
Yeah, I did check -- no include drop found in refspec.c.

Thanks
Meet

[GSoC][PATCH v2] remote: relocate valid_remote_name

[Meet Soni]

Move the `valid_remote_name()` function from the refspec subsystem to
the remote subsystem to better align with the separation of concerns.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
Range-diff against v1:
1:  cbf0f21045 ! 1:  7736bae283 remote: relocate valid_remote_name
    @@ Metadata
      ## Commit message ##
         remote: relocate valid_remote_name
     
    -    Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to
    -    better align with the separation of concerns.
    +    Move the `valid_remote_name()` function from the refspec subsystem to
    +    the remote subsystem to better align with the separation of concerns.
     
         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
     

 refspec.c | 10 ----------
 refspec.h |  1 -
 remote.c  | 10 ++++++++++
 remote.h  |  2 ++
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/refspec.c b/refspec.c
index 6d86e04442..83ec7d7e62 100644
--- a/refspec.c
+++ b/refspec.c
@@ -236,16 +236,6 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	return ret;
 }
 
-int valid_remote_name(const char *name)
-{
-	int result;
-	struct strbuf refspec = STRBUF_INIT;
-	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
-	result = valid_fetch_refspec(refspec.buf);
-	strbuf_release(&refspec);
-	return result;
-}
-
 void refspec_ref_prefixes(const struct refspec *rs,
 			  struct strvec *ref_prefixes)
 {
diff --git a/refspec.h b/refspec.h
index 69d693c87d..dc428f86f2 100644
--- a/refspec.h
+++ b/refspec.h
@@ -61,7 +61,6 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
 void refspec_clear(struct refspec *rs);
 
 int valid_fetch_refspec(const char *refspec);
-int valid_remote_name(const char *name);
 
 struct strvec;
 /*
diff --git a/remote.c b/remote.c
index 0f6fba8562..3d451570cb 100644
--- a/remote.c
+++ b/remote.c
@@ -3003,3 +3003,13 @@ char *relative_url(const char *remote_url, const char *url,
 	free(out);
 	return strbuf_detach(&sb, NULL);
 }
+
+int valid_remote_name(const char *name)
+{
+	int result;
+	struct strbuf refspec = STRBUF_INIT;
+	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
+	result = valid_fetch_refspec(refspec.buf);
+	strbuf_release(&refspec);
+	return result;
+}
diff --git a/remote.h b/remote.h
index bda10dd5c8..0c14d665b6 100644
--- a/remote.h
+++ b/remote.h
@@ -461,4 +461,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+int valid_remote_name(const char *name);
+
 #endif
-- 
2.34.1

Re: [GSoC][PATCH] remote: relocate valid_remote_name

[Junio C Hamano]

Meet Soni <meetsoni3017@gmail.com> writes:

> Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to
> better align with the separation of concerns.
>
> Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> ---
> Junio mentioned in [1], the `valid_remote_name` function belongs in remote
> header. This patch addresses that.
>
> [1]: https://lore.kernel.org/git/xmqqikq0ruuk.fsf@gitster.g/
>  refspec.c | 10 ----------
>  refspec.h |  1 -
>  remote.c  | 10 ++++++++++
>  remote.h  |  2 ++
>  4 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/refspec.c b/refspec.c
> index 6d86e04442..83ec7d7e62 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -236,16 +236,6 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
>  	return ret;
>  }
>  
> -int valid_remote_name(const char *name)
> -{
> -	int result;
> -	struct strbuf refspec = STRBUF_INIT;
> -	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
> -	result = valid_fetch_refspec(refspec.buf);
> -	strbuf_release(&refspec);
> -	return result;
> -}
> -
>  void refspec_ref_prefixes(const struct refspec *rs,
>  			  struct strvec *ref_prefixes)
>  {

This cuts both ways, though.  As valid_remote_name() is the only
external caller of valid_fetch_refspec(), without this patch, the
former function can become a file-scope static in refspec.c, but
with this patch in place, it has to stay to be public.

I do see how valid_remote_name() would be useful as a public
function, but I do not know how useful valid_fetch_refspec() is, as
a part of external refspec API.  There is no reason other than that
it gives us a handy way to implement valid_remote_name() for it to
exist.

So I dunno.

Thanks.

Re: [GSoC][PATCH] remote: relocate valid_remote_name

[Patrick Steinhardt]

On Tue, Feb 04, 2025 at 07:36:24PM +0530, Meet Soni wrote:
> On Tue, 4 Feb 2025 at 13:25, Patrick Steinhardt <ps@pks.im> wrote:
> > The change itself looks straight-forward to me. Did you happen to check
> > whether this allows you to drop any includes for "refspec.h"?
>
> I think you mean refspec.c, as refspec.h doesn’t have includes.
> Yeah, I did check -- no include drop found in refspec.c.

Not quite -- I meant whether any other file that previously included
"refspec.h" now doesn't have to anymore because the function declaration
was moved.

Patrick

Re: [GSoC][PATCH v2] remote: relocate valid_remote_name

[Patrick Steinhardt]

On Tue, Feb 04, 2025 at 07:58:52PM +0530, Meet Soni wrote:
> Move the `valid_remote_name()` function from the refspec subsystem to
> the remote subsystem to better align with the separation of concerns.

Thanks, this version looks good to me!

Patrick

Re: [GSoC][PATCH] remote: relocate valid_remote_name

[Meet Soni]

On Tue, 4 Feb 2025 at 20:17, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Feb 04, 2025 at 07:36:24PM +0530, Meet Soni wrote:
> > On Tue, 4 Feb 2025 at 13:25, Patrick Steinhardt <ps@pks.im> wrote:
> > > The change itself looks straight-forward to me. Did you happen to check
> > > whether this allows you to drop any includes for "refspec.h"?
> >
> > I think you mean refspec.c, as refspec.h doesn’t have includes.
> > Yeah, I did check -- no include drop found in refspec.c.
>
> Not quite -- I meant whether any other file that previously included
> "refspec.h" now doesn't have to anymore because the function declaration
> was moved.
Oh, I misunderstood. I checked again, but I didn't find any includes that
could be dropped.

Meet