{"thread":{"id":"54989","subject":"[PATCH v2 1/3] fetch-pack: rename helper to create_promisor_file()","startedAt":"2021-01-14T15:51:14Z","lastAt":"2021-01-14T20:12:48Z","messageCount":6,"participants":["Christian Couder","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"414356","messageId":"20210114155016.3005932-2-chriscool@tuxfamily.org","threadId":"54989","inReplyTo":"20210114155016.3005932-1-chriscool@tuxfamily.org","subject":"[PATCH v2 1/3] fetch-pack: rename helper to create_promisor_file()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-14T15:50:14Z","receivedAt":"2021-01-14T15:51:14Z","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.dirty\n\n"},{"id":"414358","messageId":"20210114155016.3005932-3-chriscool@tuxfamily.org","threadId":"54989","inReplyTo":"20210114155016.3005932-1-chriscool@tuxfamily.org","subject":"[PATCH v2 2/3] fetch-pack: refactor writing promisor file","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-14T15:50:15Z","receivedAt":"2021-01-14T15:51:14Z","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.dirty\n\n"},{"id":"414357","messageId":"20210114155016.3005932-1-chriscool@tuxfamily.org","threadId":"54989","inReplyTo":null,"subject":"[PATCH v2 0/3] Refactor writing promisor file","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-14T15:50:13Z","receivedAt":"2021-01-14T15:51:16Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This is a small patch series to refactor the code that actually writes\na promisor file (.promisor) into a write_promisor_file()\nfunction, and then to improve it a bit.\n\nCompared to version 1 of this patch series, the only difference is\nthat patch 3/3 has been added to die() in case of error, instead of\nignoring it, when actually writing the content of the file or closing\nit. Thanks to Peff and Taylor for their suggestions.\n\nDiscussion about V1:\n https://lore.kernel.org/git/20210112082159.2277214-1-chriscool@tuxfamily.org/\n\nChristian Couder (3):\n fetch-pack: rename helper to create_promisor_file()\n fetch-pack: refactor writing promisor file\n pack-write: die on error in write_promisor_file()\n\n builtin/repack.c | 8 +++-----\n fetch-pack.c | 16 +++++-----------\n pack-write.c | 16 ++++++++++++++++\n pack.h | 4 ++++\n 4 files changed, 28 insertions(+), 16 deletions(-)\n\n-- \n2.30.0.83.g36fd80b35a.dirty\n\n"},{"id":"414359","messageId":"20210114155016.3005932-4-chriscool@tuxfamily.org","threadId":"54989","inReplyTo":"20210114155016.3005932-1-chriscool@tuxfamily.org","subject":"[PATCH v2 3/3] pack-write: die on error in write_promisor_file()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-14T15:50:16Z","receivedAt":"2021-01-14T15:51:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"write_promisor_file() already uses xfopen(), so it would die\nif the file cannot be opened for writing. To be consistent\nwith this behavior and not overlook issues, let's also die if\nthere are errors when we are actually writing to the file.\n\nSuggested-by: Jeff King \nSuggested-by: Taylor Blau \nSigned-off-by: Christian Couder \n---\n pack-write.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex db3ff9980f..e9bb3fd949 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -371,11 +371,15 @@ void finish_tmp_packfile(struct strbuf *name_buffer,\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n {\n-\tint i;\n+\tint i, err;\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+\terr = ferror(output);\n+\terr |= fclose(output);\n+\tif (err)\n+\t\tdie(_(\"could not write '%s' promisor file\"), promisor_name);\n }\n-- \n2.30.0.83.g36fd80b35a.dirty\n\n"},{"id":"414365","messageId":"YAB+Hu5/5NHUb/vw@nand.local","threadId":"54989","inReplyTo":"20210114155016.3005932-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 0/3] Refactor writing promisor file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-14T17:23:42Z","receivedAt":"2021-01-14T17:24:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 14, 2021 at 04:50:13PM +0100, Christian Couder wrote:\n> Compared to version 1 of this patch series, the only difference is\n> that patch 3/3 has been added to die() in case of error, instead of\n> ignoring it, when actually writing the content of the file or closing\n> it. Thanks to Peff and Taylor for their suggestions.\n\nThanks, this version looks good to me.\n\nThanks,\nTaylor\n"},{"id":"414383","messageId":"YAClgD7HNglz2LtV@coredump.intra.peff.net","threadId":"54989","inReplyTo":"20210114155016.3005932-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 0/3] Refactor writing promisor file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-14T20:11:44Z","receivedAt":"2021-01-14T20:12:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 14, 2021 at 04:50:13PM +0100, Christian Couder wrote:\n\n> This is a small patch series to refactor the code that actually writes\n> a promisor file (.promisor) into a write_promisor_file()\n> function, and then to improve it a bit.\n> \n> Compared to version 1 of this patch series, the only difference is\n> that patch 3/3 has been added to die() in case of error, instead of\n> ignoring it, when actually writing the content of the file or closing\n> it. Thanks to Peff and Taylor for their suggestions.\n\nThese look great to me. Thanks for adding the extra error handling.\n\n-Peff\n"}]}