From: Derrick Stolee <stolee@gmail.com> To: Junio C Hamano <gitster@pobox.com>, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> Cc: git@vger.kernel.org, sandals@crustytoothpaste.net, avarab@gmail.com, peff@peff.net, Derrick Stolee <dstolee@microsoft.com> Subject: Re: [PATCH v4 02/11] commit-graph: return with errors during write Date: Mon, 13 May 2019 07:04:39 -0400 Message-ID: <17829620-1084-74e5-54ad-aa95990f4dbd@gmail.com> (raw) In-Reply-To: <xmqq36ljdp2s.fsf@gitster-ct.c.googlers.com> On 5/12/2019 11:13 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c >> @@ -188,14 +187,14 @@ static int graph_write(int argc, const char **argv) >> UNLEAK(buf); >> } >> >> - write_commit_graph(opts.obj_dir, >> - pack_indexes, >> - commit_hex, >> - opts.append, >> - 1); >> + result = write_commit_graph(opts.obj_dir, >> + pack_indexes, >> + commit_hex, >> + opts.append, >> + 1); >> >> UNLEAK(lines); >> - return 0; >> + return result; >> } > > What were the error values this function used to return? I am > wondering if the callers of this function are prepraed to see the > returned values from write_commit_graph() this function stores in > 'result' (which presumably are small negative value like our usual > internal API convention)? The only caller is cmd_commit_graph() and it is in this snippet: if (argc > 0) { if (!strcmp(argv[0], "read")) return graph_read(argc, argv); if (!strcmp(argv[0], "verify")) return graph_verify(argc, argv); if (!strcmp(argv[0], "write")) return graph_write(argc, argv); } So these return values are passed directly to the result of the builtin. If that is against convention (passing an error code from the library to the result of the builtin) then I can modify. > OK. The callers of write_commit_graph_reachable() can be careful > about its return values to the same degree as the callers of > write_commit_graph(). > > These functions perhaps deserves > /* > * returns X when .... > */ > in front (or in *.h)? Can do, in commit-graph.h. >> +int write_commit_graph(const char *obj_dir, >> + struct string_list *pack_indexes, >> + struct string_list *commit_hex, >> + int append, int report_progress) >> { >> struct packed_oid_list oids; >> struct packed_commit_list commits; >> struct hashfile *f; >> uint32_t i, count_distinct = 0; >> - char *graph_name; >> + char *graph_name = NULL; >> struct lock_file lk = LOCK_INIT; >> uint32_t chunk_ids[5]; >> uint64_t chunk_offsets[5]; >> @@ -883,15 +886,17 @@ void write_commit_graph(const char *obj_dir, >> uint64_t progress_cnt = 0; >> struct strbuf progress_title = STRBUF_INIT; >> unsigned long approx_nr_objects; >> + int res = 0; >> >> if (!commit_graph_compatible(the_repository)) >> - return; >> + return 0; > > OK. I tend to find "return 0" easier to read/follow than "return > res" here. Yes, this choice was deliberate as there is no cleanup to do if we return this early. Also note that we don't "fail" because we did exactly as much work as we expect in this scenario. I'll be careful to point this out when I add a comment to the header file. >> oids.nr = 0; >> approx_nr_objects = approximate_object_count(); >> oids.alloc = approx_nr_objects / 32; >> oids.progress = NULL; >> oids.progress_done = 0; >> + commits.list = NULL; >> >> if (append) { >> prepare_commit_graph_one(the_repository, obj_dir); >> @@ -932,10 +937,16 @@ void write_commit_graph(const char *obj_dir, >> strbuf_setlen(&packname, dirlen); >> strbuf_addstr(&packname, pack_indexes->items[i].string); >> p = add_packed_git(packname.buf, packname.len, 1); >> - if (!p) >> - die(_("error adding pack %s"), packname.buf); >> - if (open_pack_index(p)) >> - die(_("error opening index for %s"), packname.buf); >> + if (!p) { >> + error(_("error adding pack %s"), packname.buf); >> + res = 1; >> + goto cleanup; >> + } >> + if (open_pack_index(p)) { >> + error(_("error opening index for %s"), packname.buf); >> + res = 1; >> + goto cleanup; >> + } > > Hmph, is this signal an error by returning a positive "1"? That's a > bit unusual. Your hint above of "passing a negative value by convention" did make me think I must be doing something wrong. >> @@ -1006,8 +1017,11 @@ void write_commit_graph(const char *obj_dir, >> } >> stop_progress(&progress); >> >> - if (count_distinct >= GRAPH_EDGE_LAST_MASK) >> - die(_("the commit graph format cannot write %d commits"), count_distinct); >> + if (count_distinct >= GRAPH_EDGE_LAST_MASK) { >> + error(_("the commit graph format cannot write %d commits"), count_distinct); >> + res = 1; >> + goto cleanup; >> + } >> >> commits.nr = 0; >> commits.alloc = count_distinct; >> @@ -1039,16 +1053,21 @@ void write_commit_graph(const char *obj_dir, >> num_chunks = num_extra_edges ? 4 : 3; >> stop_progress(&progress); >> >> - if (commits.nr >= GRAPH_EDGE_LAST_MASK) >> - die(_("too many commits to write graph")); >> + if (commits.nr >= GRAPH_EDGE_LAST_MASK) { >> + error(_("too many commits to write graph")); >> + res = 1; >> + goto cleanup; >> + } >> >> compute_generation_numbers(&commits, report_progress); >> >> graph_name = get_commit_graph_filename(obj_dir); >> if (safe_create_leading_directories(graph_name)) { >> UNLEAK(graph_name); >> - die_errno(_("unable to create leading directories of %s"), >> - graph_name); >> + error(_("unable to create leading directories of %s"), >> + graph_name); >> + res = errno; >> + goto cleanup; >> } > > Hmph. Do we know errno==0 means no error everywhere? Do we know > errno==1 is not used by anybody as a meaningful value? > > What I am getting at is if a hardcoded "1" we saw above as "error > exists but we are not telling the caller what kind of system-level > error led to it by returning errno" (and a hardcoded "0" as "there > is no error") are consistent with this use of "res" where "the > callers are allowed to learn what system-level error led to this > error return from this function by sending the return value of this > function to strerror() or comparing with EWHATEVER". I do not think > this is a good design. That's a good point. In a new design, would you like me to (1) ignore errno here and use a constant value for "write_commit_graph() failed at some point" or to (2) split the possible _reasons_ for the failure into different constants? I believe the use of error() should prevent the need for the second option. The first option would only change this 'res = errno' into 'res = 1'. Thanks, -Stolee
next prev parent reply other threads:[~2019-05-13 11:04 UTC|newest] Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-23 21:59 [PATCH 0/6] Create commit-graph file format v2 Derrick Stolee via GitGitGadget 2019-01-23 21:59 ` [PATCH 1/6] commit-graph: return with errors during write Derrick Stolee via GitGitGadget 2019-01-23 21:59 ` [PATCH 2/6] commit-graph: collapse parameters into flags Derrick Stolee via GitGitGadget 2019-01-23 21:59 ` [PATCH 3/6] commit-graph: create new version flags Derrick Stolee via GitGitGadget 2019-01-23 21:59 ` [PATCH 4/6] commit-graph: add --version=<n> option Derrick Stolee via GitGitGadget 2019-01-24 9:31 ` Ævar Arnfjörð Bjarmason 2019-01-23 21:59 ` [PATCH 5/6] commit-graph: implement file format version 2 Derrick Stolee via GitGitGadget 2019-01-23 23:56 ` Jonathan Tan 2019-01-24 9:40 ` Ævar Arnfjörð Bjarmason 2019-01-24 14:34 ` Derrick Stolee 2019-03-21 9:21 ` Ævar Arnfjörð Bjarmason 2019-01-23 21:59 ` [PATCH 6/6] commit-graph: test verifying a corrupt v2 header Derrick Stolee via GitGitGadget 2019-01-23 23:59 ` Jonathan Tan 2019-01-24 23:05 ` [PATCH 0/6] Create commit-graph file format v2 Junio C Hamano 2019-01-24 23:39 ` Junio C Hamano 2019-01-25 13:54 ` Derrick Stolee 2019-04-24 19:58 ` [PATCH v2 0/5] " Derrick Stolee via GitGitGadget 2019-04-24 19:58 ` [PATCH v2 1/5] commit-graph: return with errors during write Derrick Stolee via GitGitGadget 2019-04-24 19:58 ` [PATCH v2 2/5] commit-graph: collapse parameters into flags Derrick Stolee via GitGitGadget 2019-04-25 5:21 ` Junio C Hamano 2019-04-24 19:58 ` [PATCH v2 3/5] commit-graph: create new version flags Derrick Stolee via GitGitGadget 2019-04-25 5:29 ` Junio C Hamano 2019-04-25 11:09 ` Derrick Stolee 2019-04-25 21:31 ` Ævar Arnfjörð Bjarmason 2019-04-26 2:20 ` Junio C Hamano 2019-04-24 19:58 ` [PATCH v2 4/5] commit-graph: add --version=<n> option Derrick Stolee via GitGitGadget 2019-04-24 19:58 ` [PATCH v2 5/5] commit-graph: implement file format version 2 Derrick Stolee via GitGitGadget 2019-04-25 22:09 ` [PATCH v2 0/5] Create commit-graph file format v2 Ævar Arnfjörð Bjarmason 2019-04-26 2:28 ` Junio C Hamano 2019-04-26 8:33 ` Ævar Arnfjörð Bjarmason 2019-04-26 12:06 ` Derrick Stolee 2019-04-26 13:55 ` Ævar Arnfjörð Bjarmason 2019-04-27 12:57 ` Ævar Arnfjörð Bjarmason 2019-05-01 13:11 ` [PATCH v3 0/6] " Derrick Stolee via GitGitGadget 2019-05-01 13:11 ` [PATCH v3 1/6] commit-graph: return with errors during write Derrick Stolee via GitGitGadget 2019-05-01 14:46 ` Ævar Arnfjörð Bjarmason 2019-05-01 13:11 ` [PATCH v3 2/6] commit-graph: collapse parameters into flags Derrick Stolee via GitGitGadget 2019-05-01 13:11 ` [PATCH v3 3/6] commit-graph: create new version parameter Derrick Stolee via GitGitGadget 2019-05-01 13:11 ` [PATCH v3 4/6] commit-graph: add --version=<n> option Derrick Stolee via GitGitGadget 2019-05-01 13:11 ` [PATCH v3 5/6] commit-graph: implement file format version 2 Derrick Stolee via GitGitGadget 2019-05-01 19:12 ` Ævar Arnfjörð Bjarmason 2019-05-01 19:56 ` Derrick Stolee 2019-05-01 13:11 ` [PATCH v3 6/6] commit-graph: remove Future Work section Derrick Stolee via GitGitGadget 2019-05-01 14:58 ` Ævar Arnfjörð Bjarmason 2019-05-01 19:59 ` Derrick Stolee 2019-05-01 20:25 ` [PATCH v3 0/6] Create commit-graph file format v2 Ævar Arnfjörð Bjarmason 2019-05-02 13:26 ` Derrick Stolee 2019-05-02 18:02 ` Ævar Arnfjörð Bjarmason 2019-05-03 12:47 ` Derrick Stolee 2019-05-03 13:41 ` Ævar Arnfjörð Bjarmason 2019-05-06 8:27 ` Christian Couder 2019-05-06 13:47 ` Derrick Stolee 2019-05-03 14:16 ` SZEDER Gábor 2019-05-03 15:11 ` Derrick Stolee 2019-05-09 14:22 ` [PATCH v4 00/11] Commit-graph write refactor (was: Create commit-graph file format v2) Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 01/11] commit-graph: fix the_repository reference Derrick Stolee via GitGitGadget 2019-05-13 2:56 ` Junio C Hamano 2019-05-09 14:22 ` [PATCH v4 02/11] commit-graph: return with errors during write Derrick Stolee via GitGitGadget 2019-05-13 3:13 ` Junio C Hamano 2019-05-13 11:04 ` Derrick Stolee [this message] 2019-05-13 11:22 ` Derrick Stolee 2019-05-09 14:22 ` [PATCH v4 03/11] commit-graph: collapse parameters into flags Derrick Stolee via GitGitGadget 2019-05-13 3:44 ` Junio C Hamano 2019-05-13 11:07 ` Derrick Stolee 2019-05-09 14:22 ` [PATCH v4 04/11] commit-graph: remove Future Work section Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 05/11] commit-graph: create write_commit_graph_context Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 07/11] commit-graph: extract fill_oids_from_commit_hex() Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 06/11] commit-graph: extract fill_oids_from_packs() Derrick Stolee via GitGitGadget 2019-05-13 5:05 ` Junio C Hamano 2019-05-09 14:22 ` [PATCH v4 08/11] commit-graph: extract fill_oids_from_all_packs() Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 09/11] commit-graph: extract count_distinct_commits() Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 10/11] commit-graph: extract copy_oids_to_commits() Derrick Stolee via GitGitGadget 2019-05-09 14:22 ` [PATCH v4 11/11] commit-graph: extract write_commit_graph_file() Derrick Stolee via GitGitGadget 2019-05-13 5:09 ` Junio C Hamano 2019-05-09 17:58 ` [PATCH v4 00/11] Commit-graph write refactor (was: Create commit-graph file format v2) Josh Steadmon 2019-06-12 13:29 ` [PATCH v5 " Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 01/11] commit-graph: fix the_repository reference Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 02/11] commit-graph: return with errors during write Derrick Stolee via GitGitGadget 2019-06-29 17:23 ` SZEDER Gábor 2019-07-01 12:19 ` Derrick Stolee 2019-06-12 13:29 ` [PATCH v5 03/11] commit-graph: collapse parameters into flags Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 04/11] commit-graph: remove Future Work section Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 05/11] commit-graph: create write_commit_graph_context Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 06/11] commit-graph: extract fill_oids_from_packs() Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 08/11] commit-graph: extract fill_oids_from_all_packs() Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 07/11] commit-graph: extract fill_oids_from_commit_hex() Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 09/11] commit-graph: extract count_distinct_commits() Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 10/11] commit-graph: extract copy_oids_to_commits() Derrick Stolee via GitGitGadget 2019-06-12 13:29 ` [PATCH v5 11/11] commit-graph: extract write_commit_graph_file() Derrick Stolee via GitGitGadget
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=17829620-1084-74e5-54ad-aa95990f4dbd@gmail.com \ --to=stolee@gmail.com \ --cc=avarab@gmail.com \ --cc=dstolee@microsoft.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=gitster@pobox.com \ --cc=peff@peff.net \ --cc=sandals@crustytoothpaste.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
git@vger.kernel.org list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ git@vger.kernel.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git