| Message ID | 47afc6a6c8757032d9d69a2f9aaaeb427c5a003f.1680571352.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Header cleanups (splitting up cache.h) | expand |
Instead of moving these declarations, can we move strbuf_repo_add_unique_abbrev() and strbuf_add_unique_abbrev() to object-name.[ch]? These functions are related to both strbuf and object-name, but object-name should be a higher level API than strbuf so it seems more natural to belong in there. "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > strbuf.h | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/strbuf.h b/strbuf.h > index 3dfeadb44c2..547696fb233 100644 > --- a/strbuf.h > +++ b/strbuf.h > @@ -1,6 +1,8 @@ > #ifndef STRBUF_H > #define STRBUF_H > > +struct object_id; > +struct repository; > struct string_list; > > /** > @@ -72,12 +74,6 @@ struct strbuf { > extern char strbuf_slopbuf[]; > #define STRBUF_INIT { .buf = strbuf_slopbuf } > > -/* > - * Predeclare this here, since cache.h includes this file before it defines the > - * struct. > - */ > -struct object_id; > - > /** > * Life Cycle Functions > * -------------------- > @@ -634,7 +630,6 @@ void strbuf_list_free(struct strbuf **list); > * Add the abbreviation, as generated by repo_find_unique_abbrev(), of `sha1` to > * the strbuf `sb`. > */ > -struct repository; > void strbuf_repo_add_unique_abbrev(struct strbuf *sb, struct repository *repo, > const struct object_id *oid, int abbrev_len); > void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid, > -- > gitgitgadget > > >
On Wed, Apr 5, 2023 at 10:28 AM Calvin Wan <calvinwan@google.com> wrote: > > Instead of moving these declarations, can we move > strbuf_repo_add_unique_abbrev() and strbuf_add_unique_abbrev() to > object-name.[ch]? These functions are related to both strbuf and > object-name, but object-name should be a higher level API than strbuf > so it seems more natural to belong in there. I like that suggestion; that would be better overall. However, should it be in this (already lengthy) series? I'm guessing you likely already have such a change in the series you're including, and if I were to add it to mine (and risk doing it slightly differently), that might increase the conflicts you need to deal with. Would it perhaps be easier to keep this small change for this series, or even drop this particular patch, and then let you address this improved direction with your strbuf work? > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > From: Elijah Newren <newren@gmail.com> > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > strbuf.h | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/strbuf.h b/strbuf.h > > index 3dfeadb44c2..547696fb233 100644 > > --- a/strbuf.h > > +++ b/strbuf.h > > @@ -1,6 +1,8 @@ > > #ifndef STRBUF_H > > #define STRBUF_H > > > > +struct object_id; > > +struct repository; > > struct string_list; > > > > /** > > @@ -72,12 +74,6 @@ struct strbuf { > > extern char strbuf_slopbuf[]; > > #define STRBUF_INIT { .buf = strbuf_slopbuf } > > > > -/* > > - * Predeclare this here, since cache.h includes this file before it defines the > > - * struct. > > - */ > > -struct object_id; > > - > > /** > > * Life Cycle Functions > > * -------------------- > > @@ -634,7 +630,6 @@ void strbuf_list_free(struct strbuf **list); > > * Add the abbreviation, as generated by repo_find_unique_abbrev(), of `sha1` to > > * the strbuf `sb`. > > */ > > -struct repository; > > void strbuf_repo_add_unique_abbrev(struct strbuf *sb, struct repository *repo, > > const struct object_id *oid, int abbrev_len); > > void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid, > > -- > > gitgitgadget
I do have this change in one of my series already, and since I'm planning on removing the declarations anyways, dropping this patch for now would be my recommendation. On Thu, Apr 6, 2023 at 11:08 PM Elijah Newren <newren@gmail.com> wrote: > > On Wed, Apr 5, 2023 at 10:28 AM Calvin Wan <calvinwan@google.com> wrote: > > > > Instead of moving these declarations, can we move > > strbuf_repo_add_unique_abbrev() and strbuf_add_unique_abbrev() to > > object-name.[ch]? These functions are related to both strbuf and > > object-name, but object-name should be a higher level API than strbuf > > so it seems more natural to belong in there. > > I like that suggestion; that would be better overall. However, should > it be in this (already lengthy) series? I'm guessing you likely > already have such a change in the series you're including, and if I > were to add it to mine (and risk doing it slightly differently), that > might increase the conflicts you need to deal with. Would it perhaps > be easier to keep this small change for this series, or even drop this > particular patch, and then let you address this improved direction > with your strbuf work? > > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > > --- > > > strbuf.h | 9 ++------- > > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > > > diff --git a/strbuf.h b/strbuf.h > > > index 3dfeadb44c2..547696fb233 100644 > > > --- a/strbuf.h > > > +++ b/strbuf.h > > > @@ -1,6 +1,8 @@ > > > #ifndef STRBUF_H > > > #define STRBUF_H > > > > > > +struct object_id; > > > +struct repository; > > > struct string_list; > > > > > > /** > > > @@ -72,12 +74,6 @@ struct strbuf { > > > extern char strbuf_slopbuf[]; > > > #define STRBUF_INIT { .buf = strbuf_slopbuf } > > > > > > -/* > > > - * Predeclare this here, since cache.h includes this file before it defines the > > > - * struct. > > > - */ > > > -struct object_id; > > > - > > > /** > > > * Life Cycle Functions > > > * -------------------- > > > @@ -634,7 +630,6 @@ void strbuf_list_free(struct strbuf **list); > > > * Add the abbreviation, as generated by repo_find_unique_abbrev(), of `sha1` to > > > * the strbuf `sb`. > > > */ > > > -struct repository; > > > void strbuf_repo_add_unique_abbrev(struct strbuf *sb, struct repository *repo, > > > const struct object_id *oid, int abbrev_len); > > > void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid, > > > -- > > > gitgitgadget
On Mon, Apr 10, 2023 at 8:52 AM Calvin Wan <calvinwan@google.com> wrote: > > I do have this change in one of my series already, and since I'm > planning on removing the declarations anyways, dropping this patch for > now would be my recommendation. Cool, I'll submit a reroll tonight with this patch dropped.
diff --git a/strbuf.h b/strbuf.h index 3dfeadb44c2..547696fb233 100644 --- a/strbuf.h +++ b/strbuf.h @@ -1,6 +1,8 @@ #ifndef STRBUF_H #define STRBUF_H +struct object_id; +struct repository; struct string_list; /** @@ -72,12 +74,6 @@ struct strbuf { extern char strbuf_slopbuf[]; #define STRBUF_INIT { .buf = strbuf_slopbuf } -/* - * Predeclare this here, since cache.h includes this file before it defines the - * struct. - */ -struct object_id; - /** * Life Cycle Functions * -------------------- @@ -634,7 +630,6 @@ void strbuf_list_free(struct strbuf **list); * Add the abbreviation, as generated by repo_find_unique_abbrev(), of `sha1` to * the strbuf `sb`. */ -struct repository; void strbuf_repo_add_unique_abbrev(struct strbuf *sb, struct repository *repo, const struct object_id *oid, int abbrev_len); void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid,