[PATCH] dim: Check whether committer is the author more thoroughly

Jani Nikula jani.nikula at linux.intel.com
Fri Oct 19 08:43:54 UTC 2018


On Thu, 26 Apr 2018, Tvrtko Ursulin <tursulin at ursulin.net> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> For patch authors and committers with multiple email addresses, it is good
> to check all 'From:' lines before deciding to add a Signed-off-by line.
> This prevents adding duplicate S-o-B's in those cases.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  dim | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/dim b/dim
> index 091dff8518ed..b587a4d1bccf 100755
> --- a/dim
> +++ b/dim
> @@ -824,6 +824,22 @@ function dim_push
>  	dim_push_branch $(git_current_branch) "$@"
>  }
>  
> +function is_own_patch
> +{
> +	patch="$1"
> +	committer_email="$2"
> +
> +	grep "From:" $patch | while read patch_from; do
> +		[[ "$patch_from" == *"$committer_email"* ]] && exit 99
> +	done

So in the case of this patch at hand, the grep will match these two
message header lines:

From: Tvrtko Ursulin <tursulin at ursulin.net>
X-Google-Original-From: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>

but, due to the base64 Content-Transfer-Encoding added somewhere along
the mail transfer, it won't match the From: line git has added at the
start of the message body. Which gets added because .gitconfig
user.email does not match the patch author. Which is probably what you
intended to search for.

Basically anywhere dim greps or otherwise manipulates the email message
directly, it's a bug waiting to happen. The current

	patch_from=$(grep "From:" "$patch" | head -1)

mostly works because it takes the first match which is in the message
headers and thus unaffected by Content-Transfer-Encoding... but an RFC
compliant message could still have line folding failing the grep:

From:
 Tvrtko Ursulin <tursulin at ursulin.net>

Email is hard.

On a style note, I prefer *not* using the

	foo || bar

or

	foo && bar

style, instead opting for explicit if-then. The notable exception is "||
true" to avoid the set -e kicking in at places.

BR,
Jani.

> +
> +	if [ $? -eq 99 ]; then
> +		return 0
> +	else
> +		return 1
> +	fi
> +}
> +
>  function apply_patch #patch_file
>  {
>  	local patch message_id committer_email patch_from sob rv
> @@ -833,10 +849,7 @@ function apply_patch #patch_file
>  	message_id=$(message_get_id $patch)
>  	committer_email=$(git_committer_email)
>  
> -	patch_from=$(grep "From:" "$patch" | head -1)
> -	if [[ "$patch_from" != *"$committer_email"* ]] ; then
> -		sob=-s
> -	fi
> +	is_own_patch "$patch" "$committer_email" || sob=-s
>  
>  	git am --scissors -3 $sob "$@" $patch

-- 
Jani Nikula, Intel Open Source Graphics Center


More information about the dim-tools mailing list