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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 30, 2020, 03:26 UTC
Message-ID: <20200430032611.GF115238@google.com>
In-Reply-To: <8ac4c63735f2e48dc83e9f31c9fbbbfb47d7e79d.1588199705.git.me@ttaylorr.com>

Hi,

Taylor Blau wrote:

Show 18 quoted lines

>            [...] 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