git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
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

  reply index

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 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 07/11] commit-graph: extract fill_oids_from_commit_hex() Derrick Stolee via GitGitGadget
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 publically 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)

Archives are clonable:
	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

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.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git