{"thread":{"id":"54976","subject":"[PATCH 1/2] fetch-pack: rename helper to create_promisor_file()","startedAt":"2021-01-12T08:23:02Z","lastAt":"2021-01-13T15:21:09Z","messageCount":7,"participants":["Christian Couder","Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"414098","messageId":"20210112082159.2277214-1-chriscool@tuxfamily.org","threadId":"54976","inReplyTo":null,"subject":"[PATCH 1/2] fetch-pack: rename helper to create_promisor_file()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-12T08:21:58Z","receivedAt":"2021-01-12T08:23:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"As we are going to refactor the code that actually writes\nthe promisor file into a separate function in a following\ncommit, let's rename the current write_promisor_file()\nfunction to create_promisor_file().\n\nSigned-off-by: Christian Couder \n---\n fetch-pack.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 876f90c759..c5fa4992a6 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -772,8 +772,8 @@ static int sideband_demux(int in, int out, void *data)\n \treturn ret;\n }\n \n-static void write_promisor_file(const char *keep_name,\n-\t\t\t\tstruct ref **sought, int nr_sought)\n+static void create_promisor_file(const char *keep_name,\n+\t\t\t\t struct ref **sought, int nr_sought)\n {\n \tstruct strbuf promisor_name = STRBUF_INIT;\n \tint suffix_stripped;\n@@ -875,7 +875,7 @@ static int get_pack(struct fetch_pack_args *args,\n \n \t\tif (args->from_promisor)\n \t\t\t/*\n-\t\t\t * write_promisor_file() may be called afterwards but\n+\t\t\t * create_promisor_file() may be called afterwards but\n \t\t\t * we still need index-pack to know that this is a\n \t\t\t * promisor pack. For example, if transfer.fsckobjects\n \t\t\t * is true, index-pack needs to know that .gitmodules\n@@ -943,7 +943,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t * obtained .keep filename if necessary\n \t */\n \tif (do_keep && pack_lockfiles && pack_lockfiles->nr && args->from_promisor)\n-\t\twrite_promisor_file(pack_lockfiles->items[0].string, sought, nr_sought);\n+\t\tcreate_promisor_file(pack_lockfiles->items[0].string, sought, nr_sought);\n \n \treturn 0;\n }\n-- \n2.30.0.83.g36fd80b35a\n\n"},{"id":"414099","messageId":"20210112082159.2277214-2-chriscool@tuxfamily.org","threadId":"54976","inReplyTo":"20210112082159.2277214-1-chriscool@tuxfamily.org","subject":"[PATCH 2/2] fetch-pack: refactor writing promisor file","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-12T08:21:59Z","receivedAt":"2021-01-12T08:23:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Let's replace the 2 different pieces of code that write a\npromisor file in 'builtin/repack.c' and 'fetch-pack.c'\nwith a new function called 'write_promisor_file()' in\n'pack-write.c' and 'pack.h'.\n\nThis might also help us in the future, if we want to put\nback the ref names and associated hashes that were in\nthe promisor files we are repacking in 'builtin/repack.c'\nas suggested by a NEEDSWORK comment just above the code\nwe are refactoring.\n\nSigned-off-by: Christian Couder \n---\n builtin/repack.c | 8 +++-----\n fetch-pack.c | 8 +-------\n pack-write.c | 12 ++++++++++++\n pack.h | 4 ++++\n 4 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 279be11a16..2158b48f4c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -14,6 +14,7 @@\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"shallow.h\"\n+#include \"pack.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -263,7 +264,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tstruct string_list_item *item;\n \t\tchar *promisor_name;\n-\t\tint fd;\n+\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n \t\titem = string_list_append(names, line.buf);\n@@ -281,10 +282,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t */\n \t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n \t\t\t\t\t line.buf);\n-\t\tfd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(_(\"unable to create '%s'\"), promisor_name);\n-\t\tclose(fd);\n+\t\twrite_promisor_file(promisor_name, NULL, 0);\n \n \t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex c5fa4992a6..1eaedcb5dc 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -777,8 +777,6 @@ static void create_promisor_file(const char *keep_name,\n {\n \tstruct strbuf promisor_name = STRBUF_INIT;\n \tint suffix_stripped;\n-\tFILE *output;\n-\tint i;\n \n \tstrbuf_addstr(&promisor_name, keep_name);\n \tsuffix_stripped = strbuf_strip_suffix(&promisor_name, \".keep\");\n@@ -787,11 +785,7 @@ static void create_promisor_file(const char *keep_name,\n \t\t keep_name);\n \tstrbuf_addstr(&promisor_name, \".promisor\");\n \n-\toutput = xfopen(promisor_name.buf, \"w\");\n-\tfor (i = 0; i < nr_sought; i++)\n-\t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n-\t\t\tsought[i]->name);\n-\tfclose(output);\n+\twrite_promisor_file(promisor_name.buf, sought, nr_sought);\n \n \tstrbuf_release(&promisor_name);\n }\ndiff --git a/pack-write.c b/pack-write.c\nindex 3513665e1e..db3ff9980f 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pack.h\"\n #include \"csum-file.h\"\n+#include \"remote.h\"\n \n void reset_pack_idx_option(struct pack_idx_option *opts)\n {\n@@ -367,3 +368,14 @@ void finish_tmp_packfile(struct strbuf *name_buffer,\n \n \tfree((void *)idx_tmp_name);\n }\n+\n+void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n+{\n+\tint i;\n+\tFILE *output = xfopen(promisor_name, \"w\");\n+\n+\tfor (i = 0; i < nr_sought; i++)\n+\t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n+\t\t\tsought[i]->name);\n+\tfclose(output);\n+}\ndiff --git a/pack.h b/pack.h\nindex 9fc0945ac9..9ae640f417 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -87,6 +87,10 @@ off_t write_pack_header(struct hashfile *f, uint32_t);\n void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n char *index_pack_lockfile(int fd);\n \n+struct ref;\n+\n+void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n+\n /*\n * The \"hdr\" output buffer should be at least this big, which will handle sizes\n * up to 2^67.\n-- \n2.30.0.83.g36fd80b35a\n\n"},{"id":"414197","messageId":"X/39TS5C2TNf2X6t@nand.local","threadId":"54976","inReplyTo":"20210112082159.2277214-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/2] fetch-pack: refactor writing promisor file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-12T19:49:33Z","receivedAt":"2021-01-12T19:50:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 12, 2021 at 09:21:59AM +0100, Christian Couder wrote:\n> Let's replace the 2 different pieces of code that write a\n> promisor file in 'builtin/repack.c' and 'fetch-pack.c'\n> with a new function called 'write_promisor_file()' in\n> 'pack-write.c' and 'pack.h'.\n\nThis is just great. Even though I'm sure that I have read that code in\nbuiltin/repack.c a hundred times before, I for some reason always have\nto remind myself what it is doing.\n\nSo, I'm very happy to see that it is being cleaned up behind a function\nwhose name describes its behavior clearly. And, if that function can be\nused to remove some duplication in a sensible fashion along the way,\nthen all the better.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"414202","messageId":"xmqqwnwh3m8u.fsf@gitster.c.googlers.com","threadId":"54976","inReplyTo":"20210112082159.2277214-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/2] fetch-pack: refactor writing promisor file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-12T20:59:13Z","receivedAt":"2021-01-12T21:41:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n> +{\n> +\tint i;\n> +\tFILE *output = xfopen(promisor_name, \"w\");\n> +\n> +\tfor (i = 0; i < nr_sought; i++)\n> +\t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n> +\t\t\tsought[i]->name);\n> +\tfclose(output);\n> +}\n\nAh, nevermind. This has its output format baked into the code and\nit wants to show object name and which ref it is on, so as long as\nwe are happy with the format (do we read the contents and use it in\nany way, by the way?), it is reasonable to expect any new caller to\nhave \"struct ref\" or an array of it.\n\nThanks.\n"},{"id":"414204","messageId":"xmqq1rep52il.fsf@gitster.c.googlers.com","threadId":"54976","inReplyTo":"20210112082159.2277214-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/2] fetch-pack: refactor writing promisor file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-12T20:22:26Z","receivedAt":"2021-01-12T21:41:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Let's replace the 2 different pieces of code that write a\n> promisor file in 'builtin/repack.c' and 'fetch-pack.c'\n> with a new function called 'write_promisor_file()' in\n> 'pack-write.c' and 'pack.h'.\n>\n> This might also help us in the future, if we want to put\n> back the ref names and associated hashes that were in\n> the promisor files we are repacking in 'builtin/repack.c'\n> as suggested by a NEEDSWORK comment just above the code\n> we are refactoring.\n\nAs soon as you say \"might\", my reading goes \"Meh\", but the real\nissue/question I have about this is \n\n> +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n> +{\n> +\tint i;\n> +\tFILE *output = xfopen(promisor_name, \"w\");\n> +\n> +\tfor (i = 0; i < nr_sought; i++)\n> +\t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n> +\t\t\tsought[i]->name);\n> +\tfclose(output);\n> +}\n\nIf this function is so useful to be factored out, it must have\npotential use cases where callers would want to write out the\npromisor file [*1*]. Is it reasonable to assume that all of these\ncallers have an array of refs, or even _know_ about what \"ref\" is?\nThere still is just one \"real\" caller to this helper after this\npatch, so it is too early to tell.\n\nSeeing that the declaration (below) is made in , I think it\nis fair to assume all the callers would know what \"struct oid\" is,\nbut I do not get the feeling that \"an array of ='struct ref' pointers\"\nis something we can expect callers to have commonly. And expecting\nand/or requiring the potential callers to have its data in an unusual\nshape would be a barrier for the helper's adoption.\n\nLet's not do this change (yet) before we see a new potential caller\nor two and know what kind of API they want this helper to have.\nWithout knowing them, my gut reaction is that it would be more\nwidely usable if it took an array of \"struct object_id\" pointers,\nbut if we make this function to take \"struct object_id **sought\"\nplus \"int nr_sought\", it would mean that the only real caller that\ncurrently exists needs to prepare a separate array out of the array\nof \"struct ref\" poihtners it has. That is way too premature\ngeneralization.\n\n\n> diff --git a/pack.h b/pack.h\n> index 9fc0945ac9..9ae640f417 100644\n> --- a/pack.h\n> +++ b/pack.h\n> @@ -87,6 +87,10 @@ off_t write_pack_header(struct hashfile *f, uint32_t);\n> void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n> char *index_pack_lockfile(int fd);\n> \n> +struct ref;\n> +\n> +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n> +\n> /*\n> * The \"hdr\" output buffer should be at least this big, which will handle sizes\n> * up to 2^67.\n\n\n[Footnote]\n\n*1* The \"we just make sure the file exists by calling this function\nwith no information about any objects\" case I do not count as an\ninteresting caller---it could just have been done with a simple\n\"touch\".\n\n\n"},{"id":"414260","messageId":"X/701Isbo8M9W2VP@coredump.intra.peff.net","threadId":"54976","inReplyTo":"20210112082159.2277214-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/2] fetch-pack: refactor writing promisor file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-13T13:25:40Z","receivedAt":"2021-01-13T13:26:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2021 at 09:21:59AM +0100, Christian Couder wrote:\n\n> Let's replace the 2 different pieces of code that write a\n> promisor file in 'builtin/repack.c' and 'fetch-pack.c'\n> with a new function called 'write_promisor_file()' in\n> 'pack-write.c' and 'pack.h'.\n> \n> This might also help us in the future, if we want to put\n> back the ref names and associated hashes that were in\n> the promisor files we are repacking in 'builtin/repack.c'\n> as suggested by a NEEDSWORK comment just above the code\n> we are refactoring.\n\nI think the interface for the callers is much nicer. Not a new problem,\nbut one thing I did notice in the implementation:\n\n> +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n> +{\n> +\tint i;\n> +\tFILE *output = xfopen(promisor_name, \"w\");\n> +\n> +\tfor (i = 0; i < nr_sought; i++)\n> +\t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n> +\t\t\tsought[i]->name);\n> +\tfclose(output);\n> +}\n\nWe check errors on open via xfopen(), which is good. But we would not\nnotice any problems writing via fprintf or fclose. Is it worth doing\nsomething like:\n\n err = ferror(output);\n err |= fclose(output);\n return err ? -1 : 0;\n\n?\n\n(As an aside, this ferror/fclose dance is awkward enough and has caused\nus enough questions in the past that I wonder if it is worth\nencapsulating into a wrapper).\n\nThe existing callers behave the same way (checking open, but not the\nwrites), so definitely not a regression. But the helper function may\nprovide an opportunity to make things more robust without adding a lot\nof duplicated code.\n\n-Peff\n"},{"id":"414269","messageId":"X/8PtpCRJ4w19zGs@nand.local","threadId":"54976","inReplyTo":"X/701Isbo8M9W2VP@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] fetch-pack: refactor writing promisor file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-13T15:20:22Z","receivedAt":"2021-01-13T15:21:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 13, 2021 at 08:25:40AM -0500, Jeff King wrote:\n> > +void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n> > +{\n> > +\tint i;\n> > +\tFILE *output = xfopen(promisor_name, \"w\");\n> > +\n> > +\tfor (i = 0; i < nr_sought; i++)\n> > +\t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n> > +\t\t\tsought[i]->name);\n> > +\tfclose(output);\n> > +}\n>\n> We check errors on open via xfopen(), which is good. But we would not\n> notice any problems writing via fprintf or fclose. Is it worth doing\n> something like:\n>\n> err = ferror(output);\n> err |= fclose(output);\n> return err ? -1 : 0;\n>\n> ?\n\nI agree below that *not* doing this isn't a regression against the\ncurrent code, since it doesn't check either, but this could be done\nrelatively easily. It is appropriate for both callers of\nwrite_promisor_file() to immediately die() if they get an error, so I\nthink that this is potentially worth doing.\n\n> (As an aside, this ferror/fclose dance is awkward enough and has caused\n> us enough questions in the past that I wonder if it is worth\n> encapsulating into a wrapper).\n\nFrom a quick grep through uses of ferror, there are a reasonable handful\nof spots that I think could be improved if there was a ferror+fclose\nhelper, perhaps: xfclose().\n\n> The existing callers behave the same way (checking open, but not the\n> writes), so definitely not a regression. But the helper function may\n> provide an opportunity to make things more robust without adding a lot\n> of duplicated code.\n\nYep.\n\n> -Peff\n\nThanks,\nTaylor\n"}]}