Re: [PATCH v4 02/11] commit-graph: return with errors during write

[Derrick Stolee <stolee@gmail.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