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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> From: Elijah Newren <newren@xxxxxxxxx>
>
> [...]
> 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:

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

> 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?



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux