Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2012, 20:56 UTC
Message-ID: <7vipblmwaq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1347171589-13327-6-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 7 quoted lines

> Instead of juggling <nr_heads,heads> (sometimes called
> <nr_match,match>), pass around the list of references to be sought in
> a single string_list variable called "sought".  Future commits will
> make more use of string_list functionality.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

The earlier bikeshedding-fest on variable names seems to have produced a winner ;-) I think "sought" captures what it is about very well.

Show 21 quoted lines

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