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

[Jonathan Nieder <jrnieder@gmail.com>]

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