{"thread":{"id":"62898","subject":"[GSoC][PATCH] remote: relocate valid_remote_name","startedAt":"2025-02-04T04:14:40Z","lastAt":"2025-02-06T10:14:36Z","messageCount":8,"participants":["Meet Soni","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511790","messageId":"20250204041430.36035-1-meetsoni3017@gmail.com","threadId":"62898","inReplyTo":null,"subject":"[GSoC][PATCH] remote: relocate valid_remote_name","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-02-04T04:14:30Z","receivedAt":"2025-02-04T04:14:40Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to\nbetter align with the separation of concerns.\n\nSigned-off-by: Meet Soni \n---\nJunio mentioned in [1], the `valid_remote_name` function belongs in remote\nheader. This patch addresses that.\n\n[1]: https://lore.kernel.org/git/xmqqikq0ruuk.fsf@gitster.g/\n refspec.c | 10 ----------\n refspec.h | 1 -\n remote.c | 10 ++++++++++\n remote.h | 2 ++\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/refspec.c b/refspec.c\nindex 6d86e04442..83ec7d7e62 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -236,16 +236,6 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \treturn ret;\n }\n \n-int valid_remote_name(const char *name)\n-{\n-\tint result;\n-\tstruct strbuf refspec = STRBUF_INIT;\n-\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n-\tresult = valid_fetch_refspec(refspec.buf);\n-\tstrbuf_release(&refspec);\n-\treturn result;\n-}\n-\n void refspec_ref_prefixes(const struct refspec *rs,\n \t\t\t struct strvec *ref_prefixes)\n {\ndiff --git a/refspec.h b/refspec.h\nindex 69d693c87d..dc428f86f2 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -61,7 +61,6 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);\n void refspec_clear(struct refspec *rs);\n \n int valid_fetch_refspec(const char *refspec);\n-int valid_remote_name(const char *name);\n \n struct strvec;\n /*\ndiff --git a/remote.c b/remote.c\nindex 0f6fba8562..3d451570cb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -3003,3 +3003,13 @@ char *relative_url(const char *remote_url, const char *url,\n \tfree(out);\n \treturn strbuf_detach(&sb, NULL);\n }\n+\n+int valid_remote_name(const char *name)\n+{\n+\tint result;\n+\tstruct strbuf refspec = STRBUF_INIT;\n+\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n+\tresult = valid_fetch_refspec(refspec.buf);\n+\tstrbuf_release(&refspec);\n+\treturn result;\n+}\ndiff --git a/remote.h b/remote.h\nindex bda10dd5c8..0c14d665b6 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -461,4 +461,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n char *relative_url(const char *remote_url, const char *url,\n \t\t const char *up_path);\n \n+int valid_remote_name(const char *name);\n+\n #endif\n\nbase-commit: 58b5801aa94ad5031978f8e42c1be1230b3d352f\n-- \n2.34.1\n\n"},{"id":"511797","messageId":"Z6HH8mWDpJUSHDd7@pks.im","threadId":"62898","inReplyTo":"20250204041430.36035-1-meetsoni3017@gmail.com","subject":"Re: [GSoC][PATCH] remote: relocate valid_remote_name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-04T07:55:30Z","receivedAt":"2025-02-04T07:55:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 04, 2025 at 09:44:30AM +0530, Meet Soni wrote:\n> Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to\n> better align with the separation of concerns.\n\nNit: you don't only move the function declaration from \"refspec.h\" to\n\"remote.h\", but also move its definition from \"refspec.c\" to \"remote.c\".\nSo you might want to instead say that you move the function between\nsubsystems, which would imply both moves.\n\nThe change itself looks straight-forward to me. Did you happen to check\nwhether this allows you to drop any includes for \"refspec.h\"?\n\nThanks!\n\nPatrick\n"},{"id":"511809","messageId":"CAPhwyn094ySxG8=p3_jF+Z+0g6h4hL5ELBYhOLv+Th8zX04Urg@mail.gmail.com","threadId":"62898","inReplyTo":"Z6HH8mWDpJUSHDd7@pks.im","subject":"Re: [GSoC][PATCH] remote: relocate valid_remote_name","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-02-04T14:06:24Z","receivedAt":"2025-02-04T14:06:39Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"On Tue, 4 Feb 2025 at 13:25, Patrick Steinhardt wrote:\n\n> Nit: you don't only move the function declaration from \"refspec.h\" to\n> \"remote.h\", but also move its definition from \"refspec.c\" to \"remote.c\".\n> So you might want to instead say that you move the function between\n> subsystems, which would imply both moves.\n>\nMakes sense. Thanks for pointing it out.\n> The change itself looks straight-forward to me. Did you happen to check\n> whether this allows you to drop any includes for \"refspec.h\"?\n>\nI think you mean refspec.c, as refspec.h doesn’t have includes.\nYeah, I did check -- no include drop found in refspec.c.\n\nThanks\nMeet\n"},{"id":"511810","messageId":"20250204142852.13035-1-meetsoni3017@gmail.com","threadId":"62898","inReplyTo":"20250204041430.36035-1-meetsoni3017@gmail.com","subject":"[GSoC][PATCH v2] remote: relocate valid_remote_name","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-02-04T14:28:52Z","receivedAt":"2025-02-04T14:29:11Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Move the `valid_remote_name()` function from the refspec subsystem to\nthe remote subsystem to better align with the separation of concerns.\n\nSigned-off-by: Meet Soni \n---\nRange-diff against v1:\n1: cbf0f21045 ! 1: 7736bae283 remote: relocate valid_remote_name\n @@ Metadata\n ## Commit message ##\n remote: relocate valid_remote_name\n \n - Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to\n - better align with the separation of concerns.\n + Move the `valid_remote_name()` function from the refspec subsystem to\n + the remote subsystem to better align with the separation of concerns.\n \n Signed-off-by: Meet Soni \n \n\n refspec.c | 10 ----------\n refspec.h | 1 -\n remote.c | 10 ++++++++++\n remote.h | 2 ++\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/refspec.c b/refspec.c\nindex 6d86e04442..83ec7d7e62 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -236,16 +236,6 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \treturn ret;\n }\n \n-int valid_remote_name(const char *name)\n-{\n-\tint result;\n-\tstruct strbuf refspec = STRBUF_INIT;\n-\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n-\tresult = valid_fetch_refspec(refspec.buf);\n-\tstrbuf_release(&refspec);\n-\treturn result;\n-}\n-\n void refspec_ref_prefixes(const struct refspec *rs,\n \t\t\t struct strvec *ref_prefixes)\n {\ndiff --git a/refspec.h b/refspec.h\nindex 69d693c87d..dc428f86f2 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -61,7 +61,6 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);\n void refspec_clear(struct refspec *rs);\n \n int valid_fetch_refspec(const char *refspec);\n-int valid_remote_name(const char *name);\n \n struct strvec;\n /*\ndiff --git a/remote.c b/remote.c\nindex 0f6fba8562..3d451570cb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -3003,3 +3003,13 @@ char *relative_url(const char *remote_url, const char *url,\n \tfree(out);\n \treturn strbuf_detach(&sb, NULL);\n }\n+\n+int valid_remote_name(const char *name)\n+{\n+\tint result;\n+\tstruct strbuf refspec = STRBUF_INIT;\n+\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n+\tresult = valid_fetch_refspec(refspec.buf);\n+\tstrbuf_release(&refspec);\n+\treturn result;\n+}\ndiff --git a/remote.h b/remote.h\nindex bda10dd5c8..0c14d665b6 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -461,4 +461,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n char *relative_url(const char *remote_url, const char *url,\n \t\t const char *up_path);\n \n+int valid_remote_name(const char *name);\n+\n #endif\n-- \n2.34.1\n\n"},{"id":"511811","messageId":"xmqqr04dvkq8.fsf@gitster.g","threadId":"62898","inReplyTo":"20250204041430.36035-1-meetsoni3017@gmail.com","subject":"Re: [GSoC][PATCH] remote: relocate valid_remote_name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-04T14:38:07Z","receivedAt":"2025-02-04T14:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Meet Soni writes:\n\n> Move the `valid_remote_name()` function from `refspec.h` to `remote.h` to\n> better align with the separation of concerns.\n>\n> Signed-off-by: Meet Soni \n> ---\n> Junio mentioned in [1], the `valid_remote_name` function belongs in remote\n> header. This patch addresses that.\n>\n> [1]: https://lore.kernel.org/git/xmqqikq0ruuk.fsf@gitster.g/\n> refspec.c | 10 ----------\n> refspec.h | 1 -\n> remote.c | 10 ++++++++++\n> remote.h | 2 ++\n> 4 files changed, 12 insertions(+), 11 deletions(-)\n>\n> diff --git a/refspec.c b/refspec.c\n> index 6d86e04442..83ec7d7e62 100644\n> --- a/refspec.c\n> +++ b/refspec.c\n> @@ -236,16 +236,6 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n> \treturn ret;\n> }\n> \n> -int valid_remote_name(const char *name)\n> -{\n> -\tint result;\n> -\tstruct strbuf refspec = STRBUF_INIT;\n> -\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n> -\tresult = valid_fetch_refspec(refspec.buf);\n> -\tstrbuf_release(&refspec);\n> -\treturn result;\n> -}\n> -\n> void refspec_ref_prefixes(const struct refspec *rs,\n> \t\t\t struct strvec *ref_prefixes)\n> {\n\nThis cuts both ways, though. As valid_remote_name() is the only\nexternal caller of valid_fetch_refspec(), without this patch, the\nformer function can become a file-scope static in refspec.c, but\nwith this patch in place, it has to stay to be public.\n\nI do see how valid_remote_name() would be useful as a public\nfunction, but I do not know how useful valid_fetch_refspec() is, as\na part of external refspec API. There is no reason other than that\nit gives us a handy way to implement valid_remote_name() for it to\nexist.\n\nSo I dunno.\n\nThanks.\n\n"},{"id":"511812","messageId":"Z6IoZ1_1wGiOo4Bi@pks.im","threadId":"62898","inReplyTo":"CAPhwyn094ySxG8=p3_jF+Z+0g6h4hL5ELBYhOLv+Th8zX04Urg@mail.gmail.com","subject":"Re: [GSoC][PATCH] remote: relocate valid_remote_name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-04T14:47:03Z","receivedAt":"2025-02-04T14:47:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 04, 2025 at 07:36:24PM +0530, Meet Soni wrote:\n> On Tue, 4 Feb 2025 at 13:25, Patrick Steinhardt wrote:\n> > The change itself looks straight-forward to me. Did you happen to check\n> > whether this allows you to drop any includes for \"refspec.h\"?\n>\n> I think you mean refspec.c, as refspec.h doesn’t have includes.\n> Yeah, I did check -- no include drop found in refspec.c.\n\nNot quite -- I meant whether any other file that previously included\n\"refspec.h\" now doesn't have to anymore because the function declaration\nwas moved.\n\nPatrick\n"},{"id":"511813","messageId":"Z6IotsaWyFo_4szp@pks.im","threadId":"62898","inReplyTo":"20250204142852.13035-1-meetsoni3017@gmail.com","subject":"Re: [GSoC][PATCH v2] remote: relocate valid_remote_name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-04T14:48:22Z","receivedAt":"2025-02-04T14:48:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 04, 2025 at 07:58:52PM +0530, Meet Soni wrote:\n> Move the `valid_remote_name()` function from the refspec subsystem to\n> the remote subsystem to better align with the separation of concerns.\n\nThanks, this version looks good to me!\n\nPatrick\n"},{"id":"511992","messageId":"CAPhwyn1_tMtZzMeNY3LhHxaVyLaD=nhWk8d-YNPHFq5ouJU7Hw@mail.gmail.com","threadId":"62898","inReplyTo":"Z6IoZ1_1wGiOo4Bi@pks.im","subject":"Re: [GSoC][PATCH] remote: relocate valid_remote_name","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-02-06T10:14:23Z","receivedAt":"2025-02-06T10:14:36Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"On Tue, 4 Feb 2025 at 20:17, Patrick Steinhardt wrote:\n>\n> On Tue, Feb 04, 2025 at 07:36:24PM +0530, Meet Soni wrote:\n> > On Tue, 4 Feb 2025 at 13:25, Patrick Steinhardt wrote:\n> > > The change itself looks straight-forward to me. Did you happen to check\n> > > whether this allows you to drop any includes for \"refspec.h\"?\n> >\n> > I think you mean refspec.c, as refspec.h doesn’t have includes.\n> > Yeah, I did check -- no include drop found in refspec.c.\n>\n> Not quite -- I meant whether any other file that previously included\n> \"refspec.h\" now doesn't have to anymore because the function declaration\n> was moved.\nOh, I misunderstood. I checked again, but I didn't find any includes that\ncould be dropped.\n\nMeet\n"}]}