[PATCH] git-am: fix "Applying" message when applypatch-hook was run

[PATCH] git-am: fix "Applying" message when applypatch-hook was run

[Simon Ruderich]

---
Hello,

This patch fixes a minor issue with git-am. When the
applypatch-hook modifies the commit message, git-am displays the
original message. This patch updates the message to use the
modified version.

Regards
Simon

 git-am.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index 202130f..0997077 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -795,6 +795,14 @@ To restore the original branch and stop patching run \"\$cmdline --abort\"."
 	then
 		"$GIT_DIR"/hooks/applypatch-msg "$dotest/final-commit" ||
 		stop_here $this
+
+		# applypatch-msg can update the commit message.
+		if test -f "$dotest/final-commit"
+		then
+			FIRSTLINE=$(sed 1q "$dotest/final-commit")
+		else
+			FIRSTLINE=""
+		fi
 	fi
 
 	say "$(eval_gettext "Applying: \$FIRSTLINE")"
-- 
1.8.2

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH] git-am: fix "Applying" message when applypatch-hook was run

[Matthieu Moy]

Simon Ruderich <simon@ruderich.org> writes:

> ---
> Hello,
>
> This patch fixes a minor issue with git-am. When the
> applypatch-hook modifies the commit message, git-am displays the
> original message. This patch updates the message to use the
> modified version.

Please, read SubmittingPatches in the Documentation directory of Git's
source tree. Your text above should be a commit message (hence, no
hello), and should not be below the --- line.

Also, read about signed-off-by in the same document.

> diff --git a/git-am.sh b/git-am.sh
> index 202130f..0997077 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -795,6 +795,14 @@ To restore the original branch and stop patching run \"\$cmdline --abort\"."
>  	then
>  		"$GIT_DIR"/hooks/applypatch-msg "$dotest/final-commit" ||
>  		stop_here $this
> +
> +		# applypatch-msg can update the commit message.
> +		if test -f "$dotest/final-commit"
> +		then
> +			FIRSTLINE=$(sed 1q "$dotest/final-commit")
> +		else
> +			FIRSTLINE=""
> +		fi
>  	fi

This copy/paste a piece of code that is already a few lines above. Is
there any reason not to _move_ the assignment to FIRSTLINE after the "if
test -x "$GIT_DIR"/hooks/applypatch-msg", to avoid duplicating?

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] git-am: fix "Applying" message when applypatch-hook was run

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Please, read SubmittingPatches in the Documentation directory of Git's
> source tree. Your text above should be a commit message (hence, no
> hello), and should not be below the --- line.
>
> Also, read about signed-off-by in the same document.
> ...
> This copy/paste a piece of code that is already a few lines above. Is
> there any reason not to _move_ the assignment to FIRSTLINE after the "if
> test -x "$GIT_DIR"/hooks/applypatch-msg", to avoid duplicating?

More importantly, is this change even desirable?

The original motivation behind the "Applying:" message was to help
the user identify which one of the 100+ patches being fed to the
command, and it was not about showing what we ended up committing.
When you are running the command interactively, we do grab the
edited result since f23272f3fd84 (git-am -i: report rewritten title,
2007-12-04), but I tend to feel that the automated munging done by
applypatch-msg falls into a different category.

Re: [PATCH] git-am: fix "Applying" message when applypatch-hook was run

[Simon Ruderich]

applypatch-hook can modify the commit message. Display the updated
commit message instead of the original one.

Signed-off-by: Simon Ruderich <simon@ruderich.org>
---

On Thu, Mar 21, 2013 at 12:36:00AM +0100, Matthieu Moy wrote:
> Please, read SubmittingPatches in the Documentation directory of Git's
> source tree. Your text above should be a commit message (hence, no
> hello), and should not be below the --- line.
>
> Also, read about signed-off-by in the same document.

Hello Matthieu,

Thank you for the suggestions. I've adapted the patch message and
added the signed-off.

> This copy/paste a piece of code that is already a few lines above. Is
> there any reason not to _move_ the assignment to FIRSTLINE after the "if
> test -x "$GIT_DIR"/hooks/applypatch-msg", to avoid duplicating?

No, there wasn't a reason not to move the code. I just wasn't
sure if it had any side effects. But I rechecked and it should
work fine. Updating version attached.

On Wed, Mar 20, 2013 at 04:52:43PM -0700, Junio C Hamano wrote:
> More importantly, is this change even desirable?
>
> The original motivation behind the "Applying:" message was to help
> the user identify which one of the 100+ patches being fed to the
> command, and it was not about showing what we ended up committing.
> When you are running the command interactively, we do grab the
> edited result since f23272f3fd84 (git-am -i: report rewritten title,
> 2007-12-04), but I tend to feel that the automated munging done by
> applypatch-msg falls into a different category.

When I first used the applypatch-msg hook I was confused because
the messages were different and I thought the hook wasn't
working, hence the patch.

I'm not sure how extensive most applypatch-msg hooks modify the
commit message (in my case just a number prepended), but I think
it's more natural and less confusing to see the message which is
being applied.

If the original behaviour is preferred, a short comment in
githooks(5) should prevent any confusion.

Regards
Simon

 git-am.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 202130f..c092855 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -778,13 +778,6 @@ To restore the original branch and stop patching run \"\$cmdline --abort\"."
 	    action=yes
 	fi
 
-	if test -f "$dotest/final-commit"
-	then
-		FIRSTLINE=$(sed 1q "$dotest/final-commit")
-	else
-		FIRSTLINE=""
-	fi
-
 	if test $action = skip
 	then
 		go_next
@@ -797,6 +790,13 @@ To restore the original branch and stop patching run \"\$cmdline --abort\"."
 		stop_here $this
 	fi
 
+	if test -f "$dotest/final-commit"
+	then
+		FIRSTLINE=$(sed 1q "$dotest/final-commit")
+	else
+		FIRSTLINE=""
+	fi
+
 	say "$(eval_gettext "Applying: \$FIRSTLINE")"
 
 	case "$resolved" in
-- 
1.8.2

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9