| Message ID | 20250204041430.36035-1-meetsoni3017@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [GSoC] remote: relocate valid_remote_name | expand |
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
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
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.
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
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
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(-) base-commit: 58b5801aa94ad5031978f8e42c1be1230b3d352f