On 5/13/2019 7:04 AM, Derrick Stolee wrote:
> On 5/12/2019 11:13 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
And I see from your other feedback (upon re-reading) that you prefer
translating a negative error value from the library into a "1" here
for the builtin.
As I prepare my next version, I'll have write_commit_graph() return -1
for all errors and have graph_write() translate that to a 1. But I'll
wait to see if you want more specific error codes from write_commit_graph().
-Stolee
