Re: [PATCH v2 13/22] hash-ll.h: split out of hash.h to remove dependency on repository.h

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 1, 2023, 17:17 UTC
Message-ID: <230501.86a5yohsme.gmgdl@evledraar.gmail.com>
In-Reply-To: <d95f97f5e90da40b285aa8fd678efcff0ae11033.1682194651.git.gitgitgadget@gmail.com>

On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:

Show 13 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> [...]
> diff --git a/checkout.h b/checkout.h
> index 1917f3b3230..3c514a5ab4f 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -1,7 +1,7 @@
>  #ifndef CHECKOUT_H
>  #define CHECKOUT_H
>  
> -#include "hash.h"
> +#include "hash-ll.h"

The end-state of this topic is oddly inconsistent in when it uses includes, and when it uses forward declarations.

As I note in a reply to 20/22 you're adding forward declares there, I think that's fine, but if you opdet for that why not do that here. The body of this header only defines one function, which takes a pointer to a "struct object_id".

Whereas above you did this change:

Show 13 quoted lines

> diff --git a/apply.h b/apply.h
> index b9f18ce87d1..7cd38b1443c 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -1,7 +1,7 @@
>  #ifndef APPLY_H
>  #define APPLY_H
>  
> -#include "hash.h"
> +#include "hash-ll.h"
>  #include "lockfile.h"
>  #include "string-list.h"
>  #include "strmap.h"

There we really should include it, as we're not dealing with a pointer to the "struct object_id", but the struct itself, so we need its definition, and don't want to find it implicitly.

Show 13 quoted lines

> diff --git a/chunk-format.h b/chunk-format.h
> index 025c38f938e..c7794e84add 100644
> --- a/chunk-format.h
> +++ b/chunk-format.h
> @@ -1,7 +1,7 @@
>  #ifndef CHUNK_FORMAT_H
>  #define CHUNK_FORMAT_H
>  
> -#include "hash.h"
> +#include "hash-ll.h"
>  
>  struct hashfile;
>  struct chunkfile;

Then we have this, where we seemingly could avoid the include as well, and just add a:

	struct git_hash_algo;

Anyway, I'm not saying one is better than the other, I'm just wondering why you're picking one, but not the other.

Is it because you know that hash-ll.h doesn't bring in other headers, so its inclusion is OK, whereas later in e.g. 20/22 you avoid including strbuf.h, because you know that'll bring in string-list.h?

If that's the case maybe we should just move strbuf_add_separated_string_list() into some "used by merge-ort.c and merge-recursive.c" file, remove the string-list.h includion from strbuf.h, and then include "strbuf.h" without fearing the side-effects elsewhere?