diff mbox series

[2/3] sequencer: use run_commit_hook()

Message ID 420ecf442c729878da5219f3c718dd136e4fa2b4.1570732608.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series sequencer: start running post-commit hook again | expand

Commit Message

Linus Arver via GitGitGadget Oct. 10, 2019, 6:36 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

This simplifies the implementation of run_prepare_commit_msg_hook() and
will be used in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c | 22 ----------------------
 commit.h         |  3 ---
 sequencer.c      | 45 ++++++++++++++++++++++++++++++++++-----------
 sequencer.h      |  2 ++
 4 files changed, 36 insertions(+), 36 deletions(-)

Comments

Johannes Schindelin Oct. 10, 2019, 9:15 p.m. UTC | #1 
Hi Phillip,

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This simplifies the implementation of run_prepare_commit_msg_hook() and
> will be used in the next commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  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
> +int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
>  #endif /* SEQUENCER_H */
> --
> gitgitgadget
>
>
Junio C Hamano Oct. 11, 2019, 4:24 a.m. UTC | #2 
Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.
Phillip Wood Oct. 14, 2019, 1:26 p.m. UTC | #3 
Hi Dscho & Junio

On 11/10/2019 05:24, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.
>
Johannes Schindelin Oct. 14, 2019, 10:14 p.m. UTC | #4 
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 <Johannes.Schindelin@gmx.de> 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
diff mbox series

Patch

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
+int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
 #endif /* SEQUENCER_H */