Re: [PATCH v4 03/11] commit-graph: collapse parameters into flags

[Derrick Stolee <stolee@gmail.com>]

On 5/12/2019 11:44 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The write_commit_graph() and write_commit_graph_reachable() methods
>> currently take two boolean parameters: 'append' and 'report_progress'.
>> We will soon expand the possible options to send to these methods, so
>> instead of complicating the parameter list, first simplify it.
> 
> I think this change to introduce "flags" and pack these two into a
> single parameter, even if there is no plan to add code that starts
> using third and subsequent bits immediately.
> 
> We are no longer adding anything beyond PROGRESS and APPEND in this
> series, no?

In this series, we are no longer expanding the options. I will add
a flag when I update the incremental file format series. I can modify
the message to no longer hint at an immediate addition.

>>
>> Collapse these parameters into a 'flags' parameter, and adjust the
>> callers to provide flags as necessary.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  builtin/commit-graph.c | 8 +++++---
>>  builtin/commit.c       | 2 +-
>>  builtin/gc.c           | 4 ++--
>>  commit-graph.c         | 9 +++++----
>>  commit-graph.h         | 8 +++++---
>>  5 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
>> index 2e86251f02..828b1a713f 100644
>> --- a/builtin/commit-graph.c
>> +++ b/builtin/commit-graph.c
>> @@ -142,6 +142,7 @@ static int graph_write(int argc, const char **argv)
>>  	struct string_list *commit_hex = NULL;
>>  	struct string_list lines;
>>  	int result;
>> +	int flags = COMMIT_GRAPH_PROGRESS;
> 
> Make it a habit to use "unsigned" not a signed type, when you pack a
> collection of bits into a flag word, unless you are treating the MSB
> specially, e.g. checking to see if it is negative is cheaper than
> masking with MSB to see if it is set.

Ah sorry. I missed this one after changing the parameter in your earlier
feedback.
 
>> ...
>>  	result = write_commit_graph(opts.obj_dir,
>>  				    pack_indexes,
>>  				    commit_hex,
>> -				    opts.append,
>> -				    1);
>> +				    flags);
>> ...
>> -int write_commit_graph_reachable(const char *obj_dir, int append,
>> -				 int report_progress)
>> +int write_commit_graph_reachable(const char *obj_dir, unsigned int flags)
>> ...
>>  int write_commit_graph(const char *obj_dir,
>>  		       struct string_list *pack_indexes,
>>  		       struct string_list *commit_hex,
>> -		       int append, int report_progress)
>> +		       unsigned int flags)
> 
> OK, so the receivers of the flags word know the collection is
> unsigned; it's just the user of the API in graph_write() that gets
> the signedness wrong.  OK, easy enough to correct, I guess.
>