Re: [PATCH v4 7/9] sequencer: load commit related config

[Phillip Wood]

On 05/12/17 11:21, Phillip Wood wrote:
> On 04/12/17 18:30, Junio C Hamano wrote:
>> Phillip Wood  writes:
>>
>>> --- 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);
>>> +}
>>> +
>>
>> Sorry for spotting the problem this late, but this code is
>> unfortunate and we will need to revisit it later; we may want to do
>> so sooner rather than later.
> 
> If it needs fixing then doing it before it hits next makes sense.
> 
>> When k,v is a valid configuration that is handled by
>> sequencer_config() successfully, this code still needs to call into
>> default_config() with the same k,v, only to get it ignored.
> 
> I'm a bit confused by this sentence. Do you mean that when k,v is a
> valid configuration that is successfully handled by sequencer_config()
> this code unnecessarily calls default_config() as there is no need to
> call default_config() if k,v have already been handled?
> 
>> The problem lies in the (mis)design of git_sequencer_config().  The
>> function should either allow the caller to notice that (k,v) has
>> already been handled, or be the last one in the callback by making a
>> call to default_config() itself.
> 
> The problem is that git_gpg_config() provides no indication if it has
> handled k,v so there's no way to avoid the call to default_config() in
> that case. builtin/am.c and builtin/commit.c both do something like
> 
> static int git_am_config(const char *k, const char *v, void *cb)
> {
>   int status;
> 
>   status = git_gpg_config(k, v, NULL);
>   if (status)
>   return status;
> 
>   return git_default_config(k, v, NULL);
> }
> 
> 
> Looking more generally at sequencer_config() I wonder if we should be
> providing a warning or an error if the config contains an invalid
> cleanup mode. Also should it be calling git_diff_ui_config() to set
> things up for print_commit_summary()? (I'm not sure if anything in that
> function is affected by diff config settings)

I think the answer maybe yes so that it loads the setting for external
diff commands and textconv filters but I'm not sure, perhaps someone
more familiar with the diff code would know? Also I think we should be
loading the basic diff config for creating the patch when we stop with
conflicts?

If anyone has any thought on this, please share them!

Best Wishes

Phillip
> 
> Let me know what you think. I should have time to update this patch set
> later in the week.
> 
> Best Wishes
> 
> Phillip
> 
>> For the former, because this helper does not have to be called
>> directly as a git_config() callback, but instead it is always meant
>> to be called indirectly from another git_config() callback (like
>> git_rebase_helper_config() here, and common_config() in
>> builtin/revert.c like we see below), it does *not* have to be
>> constrained by the function signature required for it to be a config
>> callback.  It could take a pointer to an int that stores if 'k' was
>> handled inside the function,
>>
>> int git_sequencer_config_helper(char *k, char *v, int *handled);
>>
>> for example.
>>
> 

Re: [PATCH v4 7/9] sequencer: load commit related config

[Phillip Wood]

On 05/12/17 11:21, Phillip Wood wrote:
> On 04/12/17 18:30, Junio C Hamano wrote:
>> Phillip Wood  writes:
>>
>>> --- 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);
>>> +}
>>> +
>>
>> Sorry for spotting the problem this late, but this code is
>> unfortunate and we will need to revisit it later; we may want to do
>> so sooner rather than later.
> 
> If it needs fixing then doing it before it hits next makes sense.
> 
>> When k,v is a valid configuration that is handled by
>> sequencer_config() successfully, this code still needs to call into
>> default_config() with the same k,v, only to get it ignored.
> 
> I'm a bit confused by this sentence. Do you mean that when k,v is a
> valid configuration that is successfully handled by sequencer_config()
> this code unnecessarily calls default_config() as there is no need to
> call default_config() if k,v have already been handled?
> 
>> The problem lies in the (mis)design of git_sequencer_config().  The
>> function should either allow the caller to notice that (k,v) has
>> already been handled, or be the last one in the callback by making a
>> call to default_config() itself.
> 
> The problem is that git_gpg_config() provides no indication if it has
> handled k,v so there's no way to avoid the call to default_config() in
> that case. builtin/am.c and builtin/commit.c both do something like
> 
> static int git_am_config(const char *k, const char *v, void *cb)
> {
>   int status;
> 
>   status = git_gpg_config(k, v, NULL);
>   if (status)
>   return status;
> 
>   return git_default_config(k, v, NULL);
> }
> 

Looking through the callers of gpg_config() it should be fairly easy to
convert it to take an extra parameter in the same way as you suggested
for sequencer_config()

> Looking more generally at sequencer_config() I wonder if we should be
> providing a warning or an error if the config contains an invalid
> cleanup mode. Also should it be calling git_diff_ui_config() to set
> things up for print_commit_summary()? (I'm not sure if anything in that
> function is affected by diff config settings)
> 
> Let me know what you think. I should have time to update this patch set
> later in the week.
> 
> Best Wishes
> 
> Phillip
> 
>> For the former, because this helper does not have to be called
>> directly as a git_config() callback, but instead it is always meant
>> to be called indirectly from another git_config() callback (like
>> git_rebase_helper_config() here, and common_config() in
>> builtin/revert.c like we see below), it does *not* have to be
>> constrained by the function signature required for it to be a config
>> callback.  It could take a pointer to an int that stores if 'k' was
>> handled inside the function,
>>
>> int git_sequencer_config_helper(char *k, char *v, int *handled);
>>
>> for example.
>>
> 

Re: [PATCH v4 7/9] sequencer: load commit related config

[Phillip Wood]

On 04/12/17 18:30, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> --- 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);
>> +}
>> +
> 
> Sorry for spotting the problem this late, but this code is
> unfortunate and we will need to revisit it later; we may want to do
> so sooner rather than later.

If it needs fixing then doing it before it hits next makes sense.

> When k,v is a valid configuration that is handled by
> sequencer_config() successfully, this code still needs to call into
> default_config() with the same k,v, only to get it ignored.

I'm a bit confused by this sentence. Do you mean that when k,v is a
valid configuration that is successfully handled by sequencer_config()
this code unnecessarily calls default_config() as there is no need to
call default_config() if k,v have already been handled?

> The problem lies in the (mis)design of git_sequencer_config().  The
> function should either allow the caller to notice that (k,v) has
> already been handled, or be the last one in the callback by making a
> call to default_config() itself.

The problem is that git_gpg_config() provides no indication if it has
handled k,v so there's no way to avoid the call to default_config() in
that case. builtin/am.c and builtin/commit.c both do something like

static int git_am_config(const char *k, const char *v, void *cb)
{
int status;

status = git_gpg_config(k, v, NULL);
if (status)
return status;

return git_default_config(k, v, NULL);
}


Looking more generally at sequencer_config() I wonder if we should be
providing a warning or an error if the config contains an invalid
cleanup mode. Also should it be calling git_diff_ui_config() to set
things up for print_commit_summary()? (I'm not sure if anything in that
function is affected by diff config settings)

Let me know what you think. I should have time to update this patch set
later in the week.

Best Wishes

Phillip

> For the former, because this helper does not have to be called
> directly as a git_config() callback, but instead it is always meant
> to be called indirectly from another git_config() callback (like
> git_rebase_helper_config() here, and common_config() in
> builtin/revert.c like we see below), it does *not* have to be
> constrained by the function signature required for it to be a config
> callback.  It could take a pointer to an int that stores if 'k' was
> handled inside the function,
> 
> int git_sequencer_config_helper(char *k, char *v, int *handled);
> 
> for example.
> 

Re: [PATCH v4 7/9] sequencer: load commit related config

[Junio C Hamano]

Phillip Wood  writes:

> --- 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);
> +}
> +

Sorry for spotting the problem this late, but this code is
unfortunate and we will need to revisit it later; we may want to do
so sooner rather than later.

When k,v is a valid configuration that is handled by
sequencer_config() successfully, this code still needs to call into
default_config() with the same k,v, only to get it ignored.

The problem lies in the (mis)design of git_sequencer_config().  The
function should either allow the caller to notice that (k,v) has
already been handled, or be the last one in the callback by making a
call to default_config() itself.

For the former, because this helper does not have to be called
directly as a git_config() callback, but instead it is always meant
to be called indirectly from another git_config() callback (like
git_rebase_helper_config() here, and common_config() in
builtin/revert.c like we see below), it does *not* have to be
constrained by the function signature required for it to be a config
callback.  It could take a pointer to an int that stores if 'k' was
handled inside the function,

int git_sequencer_config_helper(char *k, char *v, int *handled);

for example.

Re: [PATCH v4 7/9] sequencer: load commit related config

[Phillip Wood]

On 24/11/17 13:48, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> From: Phillip Wood 
>>
>> Load default values for message cleanup and gpg signing of commits in
>> preparation for committing without forking 'git commit'. Note that we
>> interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE to
>> be consistent with 'git commit'
> 
> Hmph, is that because we never invoke the editor to edit the commit
> log message?  Over there, scissors is demoted to space when the
> editor is not in use, but otherwise this demotion does not occur.

Yes that's right. In fact I'm fairly we always specify an explicit
cleanup mode when calling try_to_commit() so the default is there in
case that changes in the future.

> Just being curious.
> 
>>
>> Signed-off-by: Phillip Wood 
>> ---
>>
>> Notes:
>> changes since v3:
>>  - interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE
>>to match 'git commit'

Re: [PATCH v4 7/9] sequencer: load commit related config

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> Load default values for message cleanup and gpg signing of commits in
> preparation for committing without forking 'git commit'. Note that we
> interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE to
> be consistent with 'git commit'

Hmph, is that because we never invoke the editor to edit the commit
log message?  Over there, scissors is demoted to space when the
editor is not in use, but otherwise this demotion does not occur.

Just being curious.

>
> Signed-off-by: Phillip Wood 
> ---
>
> Notes:
> changes since v3:
>  - interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE
>to match 'git commit'

[PATCH v4 7/9] sequencer: load commit related config

[Phillip Wood]

From: Phillip Wood 

Load default values for message cleanup and gpg signing of commits in
preparation for committing without forking 'git commit'. Note that we
interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE to
be consistent with 'git commit'

Signed-off-by: Phillip Wood 
---

Notes:
changes since v3:
 - interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE
   to match 'git commit'

changes since v1:
 - renamed git_revert_config() to common_config()
 - prefixed cleanup_mode constants to reflect the changes to patch 2
   in this series

 builtin/rebase--helper.c | 13 -
 builtin/revert.c | 15 +--
 sequencer.c  | 34 ++
 sequencer.h  |  1 +
 4 files changed, 60 insertions(+), 3 deletions(-)

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);
+}
+
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
@@ -39,7 +50,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_END()
};
 
-   git_config(git_default_config, NULL);
+   git_config(git_rebase_helper_config, NULL);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
diff --git a/builtin/revert.c b/builtin/revert.c
index 
b9d927eb09c9ed87c84681df1396f4e6d9b13c97..1938825efa6ad20ede5aba57f097863aeb33d1d5
 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -31,6 +31,17 @@ static const char * const cherry_pick_usage[] = {
NULL
 };
 
+static int common_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);
+}
+
 static const char *action_name(const struct replay_opts *opts)
 {
return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
@@ -208,7 +219,7 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
if (isatty(0))
opts.edit = 1;
opts.action = REPLAY_REVERT;
-   git_config(git_default_config, NULL);
+   git_config(common_config, NULL);
res = run_sequencer(argc, argv, );
if (res < 0)
die(_("revert failed"));
@@ -221,7 +232,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
int res;
 
opts.action = REPLAY_PICK;
-   git_config(git_default_config, NULL);
+   git_config(common_config, NULL);
res = run_sequencer(argc, argv, );
if (res < 0)
die(_("cherry-pick failed"));
diff --git a/sequencer.c b/sequencer.c
index 
7400df5522037583108534755af6f542117667c2..40461a41e3798e267813656de6b669c297b521c6
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -688,6 +688,40 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
return run_command();
 }
 
+static enum commit_msg_cleanup_mode default_msg_cleanup =
+   COMMIT_MSG_CLEANUP_NONE;
+static char *default_gpg_sign;
+
+int git_sequencer_config(const char *k, const char *v, void *cb)
+{
+   if (!strcmp(k, "commit.cleanup")) {
+   int status;
+   const char *s;
+
+   status = git_config_string(, k, v);
+   if (status)
+   return status;
+
+   if (!strcmp(s, "verbatim"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
+   else if (!strcmp(s, "whitespace"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
+   else if (!strcmp(s, "strip"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
+   else if (!strcmp(s, "scissors"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
+
+   return status;
+   }
+
+   if (!strcmp(k, "commit.gpgsign")) {
+   default_gpg_sign = git_config_bool(k, v) ? "" : NULL;
+   return 0;
+   }
+
+   return git_gpg_config(k, v, NULL);
+}
+
 static int rest_is_empty(const struct strbuf *sb, int start)
 {
int i, eol;
diff --git a/sequencer.h b/sequencer.h
index 
4f616c61a3f3869daf9f427b978c308d6094a978..77cb174b2aaf3972ebb9e6ec379252be96dedd3d