Re: [PATCH 2/2] fetch-pack: refactor writing promisor file
From: Junio C Hamano <hidden>
Date: 2021-01-12 21:41:59
Christian Couder [off-list ref] 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.
quoted hunk
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".