| Message ID | 8ac4c63735f2e48dc83e9f31c9fbbbfb47d7e79d.1588199705.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shallow: extract a header file | expand |
Hi, Taylor Blau wrote: > [...] by making 'commit_graft_pos' non-static so that it can > be called from both 'commit.c' and 'shallow.c'. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > commit.c | 2 +- > commit.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/commit.h b/commit.h > index ab91d21131..eb42e8b6d2 100644 > --- a/commit.h > +++ b/commit.h > @@ -236,6 +236,7 @@ struct commit_graft { > typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *); > > struct commit_graft *read_graft_line(struct strbuf *line); > +int commit_graft_pos(struct repository *r, const unsigned char *sha1); Now that this function isn't file-local, its name becomes more significant. What array does this represent a position in? What does it return if the graft isn't found? From a call site it's not necessarily obvious. Ideas: - could include a comment saying that it's an index into r->parsed_objects->grafts - I'm usually loathe to suggest unnecessary duplication of code, but it might make sense to duplicate the function into shallow.c. Or even to inline it there (in the single call site, that ends up being pretty readable). Thoughts? Thanks, Jonathan
On Wed, Apr 29, 2020 at 08:26:11PM -0700, Jonathan Nieder wrote: > Hi, > > Taylor Blau wrote: > > > > [...] by making 'commit_graft_pos' non-static so that it can > > be called from both 'commit.c' and 'shallow.c'. > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > commit.c | 2 +- > > commit.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/commit.h b/commit.h > > index ab91d21131..eb42e8b6d2 100644 > > --- a/commit.h > > +++ b/commit.h > > @@ -236,6 +236,7 @@ struct commit_graft { > > typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *); > > > > struct commit_graft *read_graft_line(struct strbuf *line); > > +int commit_graft_pos(struct repository *r, const unsigned char *sha1); > > Now that this function isn't file-local, its name becomes more > significant. What array does this represent a position in? What does > it return if the graft isn't found? From a call site it's not > necessarily obvious. > > Ideas: > > - could include a comment saying that it's an index into > r->parsed_objects->grafts This and the below are both good ideas to me. I prefer this one, since we'd have to duplicate yet another static function ('commit_graft_sha1_access()' directly above) that is called by this one. > - I'm usually loathe to suggest unnecessary duplication of code, but > it might make sense to duplicate the function into shallow.c. Or > even to inline it there (in the single call site, that ends up > being pretty readable). I am not at all offended by duplication of code where it makes sense to do so, but having to duplicate two functions seems like we'd be better off simply documenting the function in commit.h. > Thoughts? > > Thanks, > Jonathan Thanks, Taylor
diff --git a/commit.c b/commit.c index c7099daeac..b76f7d72be 100644 --- a/commit.c +++ b/commit.c @@ -110,7 +110,7 @@ static const unsigned char *commit_graft_sha1_access(size_t index, void *table) return commit_graft_table[index]->oid.hash; } -static int commit_graft_pos(struct repository *r, const unsigned char *sha1) +int commit_graft_pos(struct repository *r, const unsigned char *sha1) { return sha1_pos(sha1, r->parsed_objects->grafts, r->parsed_objects->grafts_nr, diff --git a/commit.h b/commit.h index ab91d21131..eb42e8b6d2 100644 --- a/commit.h +++ b/commit.h @@ -236,6 +236,7 @@ struct commit_graft { typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *); struct commit_graft *read_graft_line(struct strbuf *line); +int commit_graft_pos(struct repository *r, const unsigned char *sha1); int register_commit_graft(struct repository *r, struct commit_graft *, int); void prepare_commit_graft(struct repository *r); struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
In the next patch, some functions will be moved from 'commit.c' to have prototypes in a new 'shallow.h' and their implementations in 'shallow.c'. Three functions in 'commit.c' use 'commit_graft_pos()' (they are 'register_commit_graft()', 'lookup_commit_graft()', and 'unregister_shallow()'). The first two of these will stay in 'commit.c', but the latter will move to 'shallow.c', and thus needs 'commit_graft_pos' to be non-static. Prepare for that by making 'commit_graft_pos' non-static so that it can be called from both 'commit.c' and 'shallow.c'. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- commit.c | 2 +- commit.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)