Re: [PATCH 6/8] midx: pass down `hash_algo` to `get_midx_filename[_ext]`

From: shejialuo <shejialuo@gmail.com>
Date: Nov 19, 2024, 12:12 UTC
Message-ID: <ZzyAyMs0_A77DtGS@ArchLinux>
In-Reply-To: <CAOLa=ZTU08VGeATHDwnREiYT=PMevrTvG3BzffLCc8DBcHy-tQ@mail.gmail.com>

On Mon, Nov 18, 2024 at 10:25:09AM -0600, karthik nayak wrote:

Show 51 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > On Fri, Nov 15, 2024 at 02:42:19PM +0100, Karthik Nayak wrote:
> >
> > [snip]
> >
> >> diff --git a/midx.h b/midx.h
> >> index 78efa28d35371795fa33c68660278182debb60ab..7620820d4d0272926af9e4eeb68bfb73404c7ec2 100644
> >> --- a/midx.h
> >> +++ b/midx.h
> >> @@ -7,6 +7,7 @@ struct object_id;
> >>  struct pack_entry;
> >>  struct repository;
> >>  struct bitmapped_pack;
> >> +struct git_hash_algo;
> >>
> >>  #define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
> >>  #define MIDX_VERSION 1
> >> @@ -89,8 +90,10 @@ struct multi_pack_index {
> >>  #define MIDX_EXT_MIDX "midx"
> >>
> >>  const unsigned char *get_midx_checksum(struct multi_pack_index *m);
> >> -void get_midx_filename(struct strbuf *out, const char *object_dir);
> >> -void get_midx_filename_ext(struct strbuf *out, const char *object_dir,
> >> +void get_midx_filename(const struct git_hash_algo *hash_algo,
> >> +		       struct strbuf *out, const char *object_dir);
> >> +void get_midx_filename_ext(const struct git_hash_algo *hash_algo,
> >> +			   struct strbuf *out, const char *object_dir,
> >>  			   const unsigned char *hash, const char *ext);
> >
> > I don't think it's a good idea to put "hash_algo" in the first argument,
> > we should put it at the last to align with the code style where we use
> > "git_hash_algo".
> >
> 
> Could you elaborate on why you think it is not a good idea?
> 
> I've mostly done this to stay consistent, because I see `struct
> repository *repo` being passed as the first variable in our code base.
> 
> Roughly:
> 
>     $ grep -Iir "struct repository \*r" --include=\*.h | wc -l
>     524
> 
>     $ grep -Iir "(struct repository \*r" --include=\*.h | wc -l
>     327
> 
> Since `hash_algo` is similar, I thought it would be nicer to be
> consistent.
>

I will elaborate on this. The reason why I think this is not a good idea comes from two aspects:

    1. I have thought that we will always put "struct git_hash_algo"
       to the end of the function definition. However, when I carefully
       inspect the code today, we could put it everywhere. So, I wrongly
       made above statement.
    2. Another aspect is that I think "struct git_hash_algo" is not the
       most important parameter for these functions. When the caller
       sees this function name "get_midx_filename_ext" without any
       knowledge, passing the "hash_algo" firstly is a little wired.

However, as 1 shows, we may not care about which position we put this parameter into. So, I agree with you that we could just align with the "struct repository *".

Thanks.

> >>  void get_midx_chain_dirname(struct strbuf *buf, const char *object_dir);
> >>  void get_midx_chain_filename(struct strbuf *buf, const char *object_dir);