threads / patch / 33242

patchgit-am: fix "Applying" message when applypatch-hook was run

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

## tl;dr

4 messages between Mar 20, 2013 and Mar 21, 2013. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Simon Ruderich· Mar 20, 2013, 23:18 UTC · lore

--- 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(+)
Show changes to git-am.sh +8 −0
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
Matthieu Moy· Mar 20, 2013, 23:36 UTC · re: Simon Ruderich · lore

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

Simon Ruderich <simon@ruderich.org> writes:
Show 7 quoted lines
> ---
> 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.
Show 17 quoted lines
> 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/
Junio C Hamano· Mar 20, 2013, 23:52 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 9 quoted lines
> 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.

Simon Ruderich· Mar 21, 2013, 02:40 UTC · re: Junio C Hamano · lore

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

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:
Show 5 quoted lines
> 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:
Show 9 quoted lines
> 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(-)
Show changes to git-am.sh +7 −7
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

← back to recent threads