Re: [PATCH 1/5] commit: make 'commit_graft_pos' non-static

[Taylor Blau <me@ttaylorr.com>]

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