Re: [PATCH v1 7/8] sequencer: load commit related config

[Phillip Wood <phillip.wood@talktalk.net>]

Thanks for looking at these patches

On 07/11/17 01:02, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Mon, 6 Nov 2017, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Load default values for message cleanup and gpg signing of commits in
>> preparation for committing without forking 'git commit'.
> 
> Nicely explained.
> 
>> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
>> index f8519363a393862b6857acab037e74367c7f2134..68194d3aed950f327a8bc624fa1991478dfea01e 100644
>> --- a/builtin/rebase--helper.c
>> +++ b/builtin/rebase--helper.c
>> @@ -9,6 +9,17 @@ static const char * const builtin_rebase_helper_usage[] = {
>>  	NULL
>>  };
>>  
>> +static int git_rebase_helper_config(const char *k, const char *v, void *cb)
>> +{
>> +	int status;
>> +
>> +	status = git_sequencer_config(k, v, NULL);
>> +	if (status)
>> +		return status;
>> +
>> +	return git_default_config(k, v, NULL);
> 
> It's more a matter of taste than anything else, but this one would be a
> little bit shorter:
> 
> 	return git_sequencer_config(k, v, NULL) ||
> 		git_default_config(k, v, NULL);

I'd do that in python or perl but in C it changes the return value which
may not matter but if the git convention is to return -1 for errors then
this deviates from that.

> A more important question would be whether this `git_default_config()`
> call could be folded into `git_sequencer_config()` right away, so that the
> same pattern does not have to be repeated in rebase--helper as well as in
> revert/cherry-pick.

I kept it separate to be more flexible, imagining that in the future
there maybe other commands that want to call git_sequencer_config()
followed by some_othor_config() before git_default_config(). I don't
have a strong opinion either way.

>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index b9d927eb09c9ed87c84681df1396f4e6d9b13c97..b700dc7f7fd8657ed8cd2450a8537fe98371783f 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -31,6 +31,17 @@ static const char * const cherry_pick_usage[] = {
>>  	NULL
>>  };
>>  
>> +static int git_revert_config(const char *k, const char *v, void *cb)
> 
> Seeing as it is used also by `cmd_cherry_pick()`, and that it is
> file-local anyway, maybe `common_config()` is a better name?

Yes that would be better, the old name came from the file it was in.

> 
> This point is moot if we can call `git_default_config()` in
> `git_sequencer_config()` directly, though.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 3e4c3bbb265db58df22cfcb5a321fb74d822327e..b8cf679751449591d6f97102904e060ebee9d7a1 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -688,6 +688,39 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>>  	return run_command(&cmd);
>>  }
>>  
>> +static enum cleanup_mode default_msg_cleanup = CLEANUP_NONE;
>> +static char *default_gpg_sign;
> 
> I was ready to shout about global state not meshing well with libified
> code, but as long as we're sure that these values are set only while Git
> executes single-threaded, still, it is the correct way to do it: these
> settings reflect the config, and therefore *are* kinda global (at least
> until the day when the submodule fans try to call `git commit` in a
> submodule using the `struct repository` data structure).
> 
> In short: this code is good (and I was just describing a little bit of my
> thinking, to demonstrate that I tried to be a diligent reviewer :-)).
> 
> Thanks,
> Dscho
>