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

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 30, 2020, 19:22 UTC
Message-ID: <20200430192251.GA6280@syl.local>
In-Reply-To: <20200430032611.GF115238@google.com>

On Wed, Apr 29, 2020 at 08:26:11PM -0700, Jonathan Nieder wrote:

Show 33 quoted lines

> 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