Re: [PATCH 2/2] fetch-pack: refactor writing promisor file

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> Let's replace the 2 different pieces of code that write a
> promisor file in 'builtin/repack.c' and 'fetch-pack.c'
> with a new function called 'write_promisor_file()' in
> 'pack-write.c' and 'pack.h'.
>
> This might also help us in the future, if we want to put
> back the ref names and associated hashes that were in
> the promisor files we are repacking in 'builtin/repack.c'
> as suggested by a NEEDSWORK comment just above the code
> we are refactoring.

As soon as you say "might", my reading goes "Meh", but the real
issue/question I have about this is 

> +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
> +{
> +	int i;
> +	FILE *output = xfopen(promisor_name, "w");
> +
> +	for (i = 0; i < nr_sought; i++)
> +		fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
> +			sought[i]->name);
> +	fclose(output);
> +}

If this function is so useful to be factored out, it must have
potential use cases where callers would want to write out the
promisor file [*1*].  Is it reasonable to assume that all of these
callers have an array of refs, or even _know_ about what "ref" is?
There still is just one "real" caller to this helper after this
patch, so it is too early to tell.

Seeing that the declaration (below) is made in <pack.h>, I think it
is fair to assume all the callers would know what "struct oid" is,
but I do not get the feeling that "an array of ='struct ref' pointers"
is something we can expect callers to have commonly.  And expecting
and/or requiring the potential callers to have its data in an unusual
shape would be a barrier for the helper's adoption.

Let's not do this change (yet) before we see a new potential caller
or two and know what kind of API they want this helper to have.
Without knowing them, my gut reaction is that it would be more
widely usable if it took an array of "struct object_id" pointers,
but if we make this function to take "struct object_id **sought"
plus "int nr_sought", it would mean that the only real caller that
currently exists needs to prepare a separate array out of the array
of "struct ref" poihtners it has.  That is way too premature
generalization.


> diff --git a/pack.h b/pack.h
> index 9fc0945ac9..9ae640f417 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -87,6 +87,10 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
>  void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
>  char *index_pack_lockfile(int fd);
>  
> +struct ref;
> +
> +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
> +
>  /*
>   * The "hdr" output buffer should be at least this big, which will handle sizes
>   * up to 2^67.


[Footnote]

*1* The "we just make sure the file exists by calling this function
with no information about any objects" case I do not count as an
interesting caller---it could just have been done with a simple
"touch".