Re: [PATCH 2/3] sequencer: use run_commit_hook()

2019-10-14 Thread Johannes Schindelin
Hi Phillip,

On Mon, 14 Oct 2019, Phillip Wood wrote:

> Hi Dscho & Junio
>
> On 11/10/2019 05:24, Junio C Hamano wrote:
> > Johannes Schindelin  writes:
> >
> >>>  builtin/commit.c | 22 --
> >>>  commit.h |  3 ---
> >>>  sequencer.c  | 45 ++---
> >>>  sequencer.h  |  2 ++
> >>>  4 files changed, 36 insertions(+), 36 deletions(-)
> >>
> >> Hmm. I would have thought that `commit.c` would be a more logical home
> >> for that function (and that the declaration could remain in `commit.h`)?
> >
> > Good correction.
>
> There are some other public commit related functions in sequencer.c -
> print_commit_summary(), commit_post_rewrite(), rest_is_empty(),
> cleanup_message(), message_is_empty(), template_untouched(),
> update_head_with_reflog() . Would you like to see them moved to commit.c
> (probably as a separate series)?

I don't think that it is necessary to move any of those functions out of
their existing habitat just yet. While I haven't looked more closely
which of these functions are specific to the sequencer and which are
more generic, I would argue that moving any of them is outside of the
goals of your patch series.

Thanks,
Dscho


Re: [PATCH 2/3] sequencer: use run_commit_hook()

2019-10-14 Thread Phillip Wood
Hi Dscho & Junio

On 11/10/2019 05:24, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
>>>  builtin/commit.c | 22 --
>>>  commit.h |  3 ---
>>>  sequencer.c  | 45 ++---
>>>  sequencer.h  |  2 ++
>>>  4 files changed, 36 insertions(+), 36 deletions(-)
>>
>> Hmm. I would have thought that `commit.c` would be a more logical home
>> for that function (and that the declaration could remain in `commit.h`)?
> 
> Good correction.

There are some other public commit related functions in sequencer.c -
print_commit_summary(), commit_post_rewrite(), rest_is_empty(),
cleanup_message(), message_is_empty(), template_untouched(),
update_head_with_reflog() . Would you like to see them moved to commit.c
(probably as a separate series)?

Best Wishes

Phillip

> 
> Thanks.
> 



Re: [PATCH 2/3] sequencer: use run_commit_hook()

2019-10-10 Thread Junio C Hamano
Johannes Schindelin  writes:

>>  builtin/commit.c | 22 --
>>  commit.h |  3 ---
>>  sequencer.c  | 45 ++---
>>  sequencer.h  |  2 ++
>>  4 files changed, 36 insertions(+), 36 deletions(-)
>
> Hmm. I would have thought that `commit.c` would be a more logical home
> for that function (and that the declaration could remain in `commit.h`)?

Good correction.

Thanks.


Re: [PATCH 2/3] sequencer: use run_commit_hook()

2019-10-10 Thread Johannes Schindelin
Hi Phillip,

On Thu, 10 Oct 2019, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood 
>
> This simplifies the implementation of run_prepare_commit_msg_hook() and
> will be used in the next commit.
>
> Signed-off-by: Phillip Wood 
> ---
>  builtin/commit.c | 22 --
>  commit.h |  3 ---
>  sequencer.c  | 45 ++---
>  sequencer.h  |  2 ++
>  4 files changed, 36 insertions(+), 36 deletions(-)

Hmm. I would have thought that `commit.c` would be a more logical home
for that function (and that the declaration could remain in `commit.h`)?

The general thrust is good, of course: `commit.h` is supposedly a
`libgit.a` header, but `builtin/commit.o` is _not_ included in
`libgit.a`...

Thanks,
Dscho

>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 1921401117..d898a57f5d 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1443,28 +1443,6 @@ static int git_commit_config(const char *k, const char 
> *v, void *cb)
>   return git_status_config(k, v, s);
>  }
>
> -int run_commit_hook(int editor_is_used, const char *index_file, const char 
> *name, ...)
> -{
> - struct argv_array hook_env = ARGV_ARRAY_INIT;
> - va_list args;
> - int ret;
> -
> - argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
> -
> - /*
> -  * Let the hook know that no editor will be launched.
> -  */
> - if (!editor_is_used)
> - argv_array_push(&hook_env, "GIT_EDITOR=:");
> -
> - va_start(args, name);
> - ret = run_hook_ve(hook_env.argv,name, args);
> - va_end(args);
> - argv_array_clear(&hook_env);
> -
> - return ret;
> -}
> -
>  int cmd_commit(int argc, const char **argv, const char *prefix)
>  {
>   const char *argv_gc_auto[] = {"gc", "--auto", NULL};
> diff --git a/commit.h b/commit.h
> index f5295ca7f3..37684a35f0 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -389,7 +389,4 @@ void verify_merge_signature(struct commit *commit, int 
> verbose);
>  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
> *unused);
>  int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
> void *unused);
>
> -LAST_ARG_MUST_BE_NULL
> -int run_commit_hook(int editor_is_used, const char *index_file, const char 
> *name, ...);
> -
>  #endif /* COMMIT_H */
> diff --git a/sequencer.c b/sequencer.c
> index 2adcf5a639..3ce578c40b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1123,28 +1123,51 @@ void commit_post_rewrite(struct repository *r,
>   run_rewrite_hook(&old_head->object.oid, new_head);
>  }
>
> +int run_commit_hook(int editor_is_used, const char *index_file,
> + const char *name, ...)
> +{
> + struct argv_array hook_env = ARGV_ARRAY_INIT;
> + va_list args;
> + int ret;
> +
> + argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
> +
> + /*
> +  * Let the hook know that no editor will be launched.
> +  */
> + if (!editor_is_used)
> + argv_array_push(&hook_env, "GIT_EDITOR=:");
> +
> + va_start(args, name);
> + ret = run_hook_ve(hook_env.argv,name, args);
> + va_end(args);
> + argv_array_clear(&hook_env);
> +
> + return ret;
> +}
> +
>  static int run_prepare_commit_msg_hook(struct repository *r,
>  struct strbuf *msg,
>  const char *commit)
>  {
>   struct argv_array hook_env = ARGV_ARRAY_INIT;
> - int ret;
> - const char *name;
> + int ret = 0;
> + const char *name, *arg1 = NULL, *arg2 = NULL;
>
>   name = git_path_commit_editmsg();
>   if (write_message(msg->buf, msg->len, name, 0))
>   return -1;
>
> - argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", r->index_file);
> - argv_array_push(&hook_env, "GIT_EDITOR=:");
> - if (commit)
> - ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
> -   "commit", commit, NULL);
> - else
> - ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
> -   "message", NULL);
> - if (ret)
> + if (commit) {
> + arg1 = "commit";
> + arg2 = commit;
> + } else {
> + arg1 = "message";
> + }
> + if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
> + arg1, arg2, NULL))
>   ret = error(_("'prepare-commit-msg' hook failed"));
> +
>   argv_array_clear(&hook_env);
>
>   return ret;
> diff --git a/sequencer.h b/sequencer.h
> index ac66892d71..b0419d6ddb 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -201,4 +201,6 @@ int write_basic_state(struct replay_opts *opts, const 
> char *head_name,
>  void sequencer_post_commit_cleanup(struct repository *r);
>  int sequencer_get_last_command(struct repository* r,
>  enum replay_action *action);
> +LAST_ARG_MUST_BE_NULL
> +i