Skip to content

sequencer: start running post-commit hook again #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions builtin/commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -1443,28 +1443,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
return git_status_config(k, v, s);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Phillip,

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

> From: Phillip Wood <[email protected]>
>
> This simplifies the implementation of run_prepare_commit_msg_hook() and
> will be used in the next commit.
>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
>  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 c=
har *name, ...)
> -{
> -	struct argv_array hook_env =3D ARGV_ARRAY_INIT;
> -	va_list args;
> -	int ret;
> -
> -	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=3D%s", index_file);
> -
> -	/*
> -	 * Let the hook know that no editor will be launched.
> -	 */
> -	if (!editor_is_used)
> -		argv_array_push(&hook_env, "GIT_EDITOR=3D:");
> -
> -	va_start(args, name);
> -	ret =3D 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[] =3D {"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, i=
nt 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 c=
har *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 =3D ARGV_ARRAY_INIT;
> +	va_list args;
> +	int ret;
> +
> +	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=3D%s", index_file);
> +
> +	/*
> +	 * Let the hook know that no editor will be launched.
> +	 */
> +	if (!editor_is_used)
> +		argv_array_push(&hook_env, "GIT_EDITOR=3D:");
> +
> +	va_start(args, name);
> +	ret =3D 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 =3D ARGV_ARRAY_INIT;
> -	int ret;
> -	const char *name;
> +	int ret =3D 0;
> +	const char *name, *arg1 =3D NULL, *arg2 =3D NULL;
>
>  	name =3D git_path_commit_editmsg();
>  	if (write_message(msg->buf, msg->len, name, 0))
>  		return -1;
>
> -	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=3D%s", r->index_file);
> -	argv_array_push(&hook_env, "GIT_EDITOR=3D:");
> -	if (commit)
> -		ret =3D run_hook_le(hook_env.argv, "prepare-commit-msg", name,
> -				  "commit", commit, NULL);
> -	else
> -		ret =3D run_hook_le(hook_env.argv, "prepare-commit-msg", name,
> -				  "message", NULL);
> -	if (ret)
> +	if (commit) {
> +		arg1 =3D "commit";
> +		arg2 =3D commit;
> +	} else {
> +		arg1 =3D "message";
> +	}
> +	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
> +			    arg1, arg2, NULL))
>  		ret =3D 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, cons=
t 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 c=
har *name, ...);
>  #endif /* SEQUENCER_H */
> --
> gitgitgadget
>
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Dscho & Junio

On 11/10/2019 05:24, Junio C Hamano wrote:
> Johannes Schindelin <[email protected]> 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.
> 

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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 <[email protected]> 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 hom=
e
> >> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Phillip,

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

> From: Phillip Wood <[email protected]>
>
> Prior to commit 356ee4659b ("sequencer: try to commit without forking
> 'git commit'", 2017-11-24) the sequencer would always run the
> post-commit hook after each pick or revert as it forked `git commit` to
> create the commit. The conversion to committing without forking `git
> commit` omitted to call the post-commit hook after creating the commit.
>
> Signed-off-by: Phillip Wood <[email protected]>

Makes sense.

> ---
>  builtin/commit.c              |  2 +-
>  sequencer.c                   |  5 +++++
>  sequencer.h                   |  1 +
>  t/t3404-rebase-interactive.sh | 17 +++++++++++++++++
>  4 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d898a57f5d..adb8c89c60 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const =
char *prefix)
>
>  	repo_rerere(the_repository, 0);
>  	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
> -	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
> +	run_post_commit_hook(use_editor, get_index_file());

Does it really make sense to abstract the hook name away? It adds a lot
of churn for just two callers...

>  	if (amend && !no_post_rewrite) {
>  		commit_post_rewrite(the_repository, current_head, &oid);
>  	}
> diff --git a/sequencer.c b/sequencer.c
> index 3ce578c40b..b4947f6969 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1173,6 +1173,10 @@ static int run_prepare_commit_msg_hook(struct rep=
ository *r,
>  	return ret;
>  }
>
> +void run_post_commit_hook(int editor_is_used, const char *index_file) {
> +	run_commit_hook(editor_is_used, index_file, "post-commit", NULL);
> +}
> +

If we must have a separate `run_post_commit_hook()`, then it should be
an `inline` function, defined in the header. Or even a macro to begin
with.

>  static const char implicit_ident_advice_noconfig[] =3D
>  N_("Your name and email address were configured automatically based\n"
>  "on your username and hostname. Please check that they are accurate.\n"
> @@ -1427,6 +1431,7 @@ static int try_to_commit(struct repository *r,
>  		goto out;
>  	}
>
> +	run_post_commit_hook(0, r->index_file);

So this is the _actual_ change of this patch.

>  	if (flags & AMEND_MSG)
>  		commit_post_rewrite(r, current_head, oid);
>
> diff --git a/sequencer.h b/sequencer.h
> index b0419d6ddb..e3e73c5635 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -203,4 +203,5 @@ 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 c=
har *name, ...);
> +void run_post_commit_hook(int editor_is_used, const char *index_file);
>  #endif /* SEQUENCER_H */
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.=
sh
> index d2f1d5bd23..d9217235b6 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when aut=
hor contains single quote' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'post-commit hook is called' '
> +	test_when_finished "rm -f .git/hooks/post-commit commits" &&
> +	mkdir -p .git/hooks &&
> +	write_script .git/hooks/post-commit <<-\EOS &&
> +	git rev-parse HEAD >>commits

Should `commits` be initialized before this script is written, e.g.
using

	>commits &&

?

> +	EOS
> +	set_fake_editor &&

The `set_fake_editor` function sets a global environment variable, and
therefore needs to be run in a subshell. Therefore, this line (as well
as the next one) need to be enclosed in `( ... )`.

> +	FAKE_LINES=3D"edit 4 1 reword 2 fixup 3" git rebase -i A E &&
> +	echo x>file3 &&

We usually leave no space after the `>`, but we _do_ leave a space
_before_ the `>`.

> +	git add file3 &&
> +	FAKE_COMMIT_MESSAGE=3Dedited git rebase --continue &&
> +	# rev-list does not support -g --reverse
> +	git rev-list --no-walk=3Dunsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} =
\
> +		HEAD@{1} HEAD >expected &&

Wouldn't this be better as:

	git rev-parse HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} HEAD@{1} HEAD \
		>expect &&

> +	test_cmp expected commits

We usually use the name `expect` instead of `expected` in the test
suite.

Thanks,
Dscho

> +'
> +
>  test_done
> --
> gitgitgadget
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> writes:

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d898a57f5d..adb8c89c60 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>
>>  	repo_rerere(the_repository, 0);
>>  	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>> -	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
>> +	run_post_commit_hook(use_editor, get_index_file());
>
> Does it really make sense to abstract the hook name away? It adds a lot
> of churn for just two callers...

After looking at the three patches, I do not think so.

>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index d2f1d5bd23..d9217235b6 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when author contains single quote' '
>>  	test_cmp expected actual
>>  '
>>
>> +test_expect_success 'post-commit hook is called' '
>> +	test_when_finished "rm -f .git/hooks/post-commit commits" &&
>> +	mkdir -p .git/hooks &&
>> +	write_script .git/hooks/post-commit <<-\EOS &&
>> +	git rev-parse HEAD >>commits
>
> Should `commits` be initialized before this script is written, e.g.
> using
>
> 	>commits &&

Yes.

>> +	git rev-list --no-walk=unsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} \
>> +		HEAD@{1} HEAD >expected &&
>
> Wouldn't this be better as:
>
> 	git rev-parse HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} HEAD@{1} HEAD \
> 		>expect &&
>

Yes.

>> +	test_cmp expected commits
>
> We usually use the name `expect` instead of `expected` in the test
> suite.

And the actual output file is called 'actual'.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Dscho

On 10/10/2019 22:31, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 10 Oct 2019, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <[email protected]>
>>
>> Prior to commit 356ee4659b ("sequencer: try to commit without forking
>> 'git commit'", 2017-11-24) the sequencer would always run the
>> post-commit hook after each pick or revert as it forked `git commit` to
>> create the commit. The conversion to committing without forking `git
>> commit` omitted to call the post-commit hook after creating the commit.
>>
>> Signed-off-by: Phillip Wood <[email protected]>
> 
> Makes sense.
> 
>> ---
>>   builtin/commit.c              |  2 +-
>>   sequencer.c                   |  5 +++++
>>   sequencer.h                   |  1 +
>>   t/t3404-rebase-interactive.sh | 17 +++++++++++++++++
>>   4 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d898a57f5d..adb8c89c60 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>
>>   	repo_rerere(the_repository, 0);
>>   	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>> -	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
>> +	run_post_commit_hook(use_editor, get_index_file());
> 
> Does it really make sense to abstract the hook name away? It adds a lot
> of churn for just two callers...

I'll drop the new function in the reroll

>>   	if (amend && !no_post_rewrite) {
>>   		commit_post_rewrite(the_repository, current_head, &oid);
>>   	}
>> diff --git a/sequencer.c b/sequencer.c
>> index 3ce578c40b..b4947f6969 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1173,6 +1173,10 @@ static int run_prepare_commit_msg_hook(struct repository *r,
>>   	return ret;
>>   }
>>
>> +void run_post_commit_hook(int editor_is_used, const char *index_file) {
>> +	run_commit_hook(editor_is_used, index_file, "post-commit", NULL);
>> +}
>> +
> 
> If we must have a separate `run_post_commit_hook()`, then it should be
> an `inline` function, defined in the header. Or even a macro to begin
> with.
> 
>>   static const char implicit_ident_advice_noconfig[] =
>>   N_("Your name and email address were configured automatically based\n"
>>   "on your username and hostname. Please check that they are accurate.\n"
>> @@ -1427,6 +1431,7 @@ static int try_to_commit(struct repository *r,
>>   		goto out;
>>   	}
>>
>> +	run_post_commit_hook(0, r->index_file);
> 
> So this is the _actual_ change of this patch.
> 
>>   	if (flags & AMEND_MSG)
>>   		commit_post_rewrite(r, current_head, oid);
>>
>> diff --git a/sequencer.h b/sequencer.h
>> index b0419d6ddb..e3e73c5635 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -203,4 +203,5 @@ 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, ...);
>> +void run_post_commit_hook(int editor_is_used, const char *index_file);
>>   #endif /* SEQUENCER_H */
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index d2f1d5bd23..d9217235b6 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when author contains single quote' '
>>   	test_cmp expected actual
>>   '
>>
>> +test_expect_success 'post-commit hook is called' '
>> +	test_when_finished "rm -f .git/hooks/post-commit commits" &&
>> +	mkdir -p .git/hooks &&
>> +	write_script .git/hooks/post-commit <<-\EOS &&
>> +	git rev-parse HEAD >>commits
> 
> Should `commits` be initialized before this script is written, e.g.
> using
> 
> 	>commits &&

Good point, especially if it is renamed to actual as Junio suggests

>> +	EOS
>> +	set_fake_editor &&
> 
> The `set_fake_editor` function sets a global environment variable, and
> therefore needs to be run in a subshell. Therefore, this line (as well
> as the next one) need to be enclosed in `( ... )`.

There are ~80 instances of 
set_fake_editor/test_set_editor/set_cat_todo_editor in that file that 
are not in subshells. I've converted them in a preparatory patch (that 
was fun), removing about 20 that can now safely rely on EDITOR=: 
(hopefully that will ameliorate the performance hit of ~60 extra 
subshells a little)

>> +	FAKE_LINES="edit 4 1 reword 2 fixup 3" git rebase -i A E &&
>> +	echo x>file3 &&
> 
> We usually leave no space after the `>`, but we _do_ leave a space
> _before_ the `>`.
> 
>> +	git add file3 &&
>> +	FAKE_COMMIT_MESSAGE=edited git rebase --continue &&
>> +	# rev-list does not support -g --reverse
>> +	git rev-list --no-walk=unsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} \
>> +		HEAD@{1} HEAD >expected &&
> 
> Wouldn't this be better as:
> 
> 	git rev-parse HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} HEAD@{1} HEAD \
> 		>expect &&

Good point

>> +	test_cmp expected commits
> 
> We usually use the name `expect` instead of `expected` in the test
> suite.

OK

Thanks for looking at this series

Phillip

> Thanks,
> Dscho
> 
>> +'
>> +
>>   test_done
>> --
>> gitgitgadget
>>

}

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};
Expand Down
24 changes: 24 additions & 0 deletions commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "advice.h"
#include "refs.h"
#include "commit-reach.h"
#include "run-command.h"

static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);

Expand Down Expand Up @@ -1581,3 +1582,26 @@ size_t ignore_non_trailer(const char *buf, size_t len)
}
return boc ? len - boc : len - cutoff;
}

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;
}
24 changes: 11 additions & 13 deletions sequencer.c
Original file line number Diff line number Diff line change
Expand Up @@ -1127,25 +1127,22 @@ 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;
}
Expand Down Expand Up @@ -1404,6 +1401,7 @@ static int try_to_commit(struct repository *r,
goto out;
}

run_commit_hook(0, r->index_file, "post-commit", NULL);
if (flags & AMEND_MSG)
commit_post_rewrite(r, current_head, oid);

Expand Down
3 changes: 1 addition & 2 deletions sequencer.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,10 @@ void print_commit_summary(struct repository *repo,

int read_author_script(const char *path, char **name, char **email, char **date,
int allow_missing);
#endif

void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
int write_basic_state(struct replay_opts *opts, const char *head_name,
struct commit *onto, const char *orig_head);
void sequencer_post_commit_cleanup(struct repository *r);
int sequencer_get_last_command(struct repository* r,
enum replay_action *action);
#endif /* SEQUENCER_H */
28 changes: 28 additions & 0 deletions t/lib-rebase.sh
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,31 @@ make_empty () {
git commit --allow-empty -m "$1" &&
git tag "$1"
}

# Call this (inside test_expect_success) at the end of a test file to
# check that no tests have changed editor related environment
# variables or config settings
test_editor_unchanged () {
# We're only interested in exported variables hence 'sh -c'
sh -c 'cat >actual <<-EOF
EDITOR=$EDITOR
FAKE_COMMIT_AMEND=$FAKE_COMMIT_AMEND
FAKE_COMMIT_MESSAGE=$FAKE_COMMIT_MESSAGE
FAKE_LINES=$FAKE_LINES
GIT_EDITOR=$GIT_EDITOR
GIT_SEQUENCE_EDITOR=$GIT_SEQUENCE_EDITOR
core.editor=$(git config core.editor)
sequence.editor=$(git config sequence.editor)
EOF'
cat >expect <<-\EOF
EDITOR=:
FAKE_COMMIT_AMEND=
FAKE_COMMIT_MESSAGE=
FAKE_LINES=
GIT_EDITOR=
GIT_SEQUENCE_EDITOR=
core.editor=
sequence.editor=
EOF
test_cmp expect actual
}
Loading