Hi Phillip,
On Mon, 6 Nov 2017, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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);
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.
> 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?
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
