Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Sep 17, 2012, 12:24 UTC
- Message-ID
- <5057167A.6040403@alum.mit.edu>
- In-Reply-To
- <7vipblmwaq.fsf@alter.siamese.dyndns.org>
On 09/10/2012 10:56 PM, Junio C Hamano wrote:
Show 41 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> diff --git a/fetch-pack.h b/fetch-pack.h
>> index 1dbe90f..a6a8a73 100644
>> --- a/fetch-pack.h
>> +++ b/fetch-pack.h
>> @@ -1,6 +1,8 @@
>> #ifndef FETCH_PACK_H
>> #define FETCH_PACK_H
>>
>> +#include "string-list.h"
>> +
>> struct fetch_pack_args {
>> const char *uploadpack;
>> int unpacklimit;
>> @@ -21,8 +23,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>> int fd[], struct child_process *conn,
>> const struct ref *ref,
>> const char *dest,
>> - int nr_heads,
>> - char **heads,
>> + struct string_list *sought,
>> char **pack_lockfile);
>
> This is a tangent, but I _think_ our header files ignore the dogma
> some other projects follow that insists on each header file to be
> self sufficient, i.e.
>
> gcc fetch-pack.h
>
> should pass. Instead, our *.c files that include fetch-pack.h are
> responsible for including everything the headers they include need.
> So even though fetch-pack.h does not include run-command.h, it
> declares a function that takes "struct child_process *" in its
> arguments. The new "struct string_list *" falls into the same camp.
>
> Given that, I'd prefer to see the scope of this patch series shrunk
> and have the caller include string-list.h, not here.
>
> Updating the headers and sources so that each to be self sufficient
> is a different topic, and I do not think there is a consensus yet if
> we want to go that route.The include line can just be deleted, because the three files that include it already get string-list.h from somewhere:
* builtin/clone.c, builtin/fetch-pack.c includes builtin.h includes notes.h includes string-list.h
* transport.c includes string-list.h
Patch forthcoming.
But how far should this policy be taken? It seems to me that strict adherence to the policy would dictate that *.h files should *never* include other git project files.
For example, while working on this, I noticed that notes.h includes string-list.h and also contains a forward declaration for "struct string_list". Obviously, the latter could be deleted. Alternatively, both could probably be deleted by ensuring that the relevant *.c files include string-list.h before including notes.h.
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/