[PATCH 2/2] dim: Fix outlook_author variable.

Wed Feb 28 00:39:35 UTC 2018

On Thu, Feb 22, 2018 at 01:17:02PM -0800, Lucas De Marchi wrote:
> On Thu, Feb 22, 2018 at 04:33:46PM +0100, Daniel Vetter wrote:
> > On Wed, Feb 21, 2018 at 03:35:11PM -0800, Rodrigo Vivi wrote:
> > > On Wed, Feb 21, 2018 at 03:12:22PM -0800, Lucas De Marchi wrote:
> > > > On Wed, Feb 21, 2018 at 03:03:44PM -0800, Lucas De Marchi wrote:
> > > > > On Wed, Feb 21, 2018 at 02:02:00PM -0800, Rodrigo Vivi wrote:
> > > > > > On Wed, Feb 21, 2018 at 01:47:58PM -0800, Lucas De Marchi wrote:
> > > > > > > On Mon, Feb 19, 2018 at 04:51:46AM +0100, Daniel Vetter wrote:
> > > > > > > > On Thu, Feb 15, 2018 at 11:14:10AM -0800, Rodrigo Vivi wrote:
> > > > > > > > > The current version author_outlook ends up like
> > > > > > > > > "First, Last," instead of "First Last" as it should be.
> > > > > > > > > 
> > > > > > > > > Honestly I'm so glad that this never worked. So I had a chance
> > > > > > > > > to contact authors before merging the patch and fix the
> > > > > > > > > outlook + patchwork mess.
> > > > > > > > > 
> > > > > > > > > But anyways, if we decide to stay with this logic let's at least
> > > > > > > > > fix it.
> > > > > > > > > 
> > > > > > > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > > > > ---
> > > > > > > > >  dim | 2 +-
> > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/dim b/dim
> > > > > > > > > index 4493b48b1494..ed8b9b03c42a 100755
> > > > > > > > > --- a/dim
> > > > > > > > > +++ b/dim
> > > > > > > > > @@ -732,7 +732,7 @@ function checkpatch_commit_push
> > > > > > > > >  	author=$(git show -s $sha1 --format="format:%an")
> > > > > > > > >  	committer=$(git show -s $sha1 --format="format:%cn")
> > > > > > > > >  	# outlook mangles mails into "Last, First"
> > > > > > > > > -	author_outlook=$(git show -s $sha1 --format="format:%an" | sed -e 's/\([^ ]*\) \(.*\)/\2, \1/')
> > > > > > > > > +	author_outlook=$(git show -s $sha1 --format="format:%an" | sed -e 's/\([^ ]*\) \(.*\)/\2 \1/;s/,//')
> > > > > > > > 
> > > > > > > > The above regex (the original one) moves from "Last, First" to "First
> > > > > > > > Last". 
> > > > > > > 
> > > > > > > It seems it's actually from "First Last" to "Last, First".... but this
> > > > > > > is not what the comment says... it should be converting the other way
> > > > > > > around.
> > > > > > > 
> > > > > > > > Sounds like we need a 2nd path for the other direction? Also
> > > > > > > > simpler to remove the ',' with one regex, like this:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 	author_from_outlook=$(git show -s $sha1 --format="format:%an" | sed -e 's/\([^ ]*\), \(.*\)/\2 \1/')
> > > > > > > 
> > > > > > > You are considering only First and Last without spaces, which isn't
> > > > > > > globally true.
> > > > > > > 
> > > > > > > 	$ echo "De Marchi, Lucas" | sed -e 's/\([^ ]*\), \(.*\)/\2 \1/')
> > > > > > > 	De Lucas Marchi
> > > > > > > 
> > > > > > > not what we want. This yields better results since we don't have to
> > > > > > > deal with the ambiguous space like in the other way around:
> > > > > > > 
> > > > > > > 	author_from_outlook=$(git show -s $sha1 --format="format:%an" | sed -e 's/\(.*\), \(.*\)/\2 \1/')
> > > > > > 
> > > > > > Ok... if I understood correctly so this seems what we want on that author_outlook so we can use
> > > > > > that to check the presense on signed-off-by.. right?
> > > > > 
> > > > > In the case the email was unwillingly converted to "Last, First" by
> > > > > outlook servers, we will have author in the "Last, First" form.
> > > > > I don't think there's a safe conversion from
> > > > > "First Last" -> "Last, First", so what we do is try to convert the other
> > > > > way around and check if the sob matches.
> > > > > 
> > > > > Patches may be in this form (considering you are downloading them from
> > > > > pw):
> > > > > 	From: De Marchi, Lucas <lucas.demarchi at intel.com>
> > > > > 	...
> > > > > 	Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > > 
> > > > > So:
> > > > > author="De Marchi, Lucas"
> > > > > author_first_last="Lucas De Marchi"
> > > > > 
> > > > > And we accept any of the two to sign off
> > > > > 
> > > > > Or if you are getting from mailing list:
> > > > > 	From: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > > 	...
> > > > > 	Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > > 
> > > > > author="Lucas De Marchi"
> > > > > author_first_last="Lucas De Marchi"
> > > > > 
> > > > > And we only accept in the First, Last form, because AFAIK there's no
> > > > > server changing the other way around.
> > > > > 
> > > > > And this is what that line is doing:
> > > > > 
> > > > > 	$ echo "Lucas De Marchi" | sed -e 's/\(.*\), \(.*\)/\2 \1/'
> > > > > 	Lucas De Marchi
> > > > > 
> > > > > 	$ echo "De Marchi, Lucas" | sed -e 's/\(.*\), \(.*\)/\2 \1/'
> > > > > 	Lucas De Marchi
> > > > > 
> > > > > Renaming the variable to be author_first_last I think would make it
> > > > > clearer.
> > > > 
> > > > And now I see we have patches from José:
> > > > José Roberto de Souza <jose.souza at intel.com>
> > > > 
> > > > And replies from him coming from outlook/ews are in the form "Souza,
> > > > José".  So if you are downloading the patch from patchwork, even this
> > > > workaround would not fix it.
> > > 
> > > So, just revert that commit that introduced the outlook_author?
> > > 
> > > > 
> > > > Maybe the only real solution is to fix patchwork.
> > > 
> > > and this...
> > 
> > I think we can try to match for either mail address matching or name
> > matching (for the people with too many email addresses). When patchwork
> > mangles the name, it won't change the address itself, right?
> 
> Right. What about this:
> 
> -----8<-----
> From: Lucas De Marchi <lucas.demarchi at intel.com>
> Subject: Accept sign-off by email or name
> 
> Stop trying to accept the mangled name from outlook/patchwork and
> instead accept the sign-off if email matches.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> 
> diff --git a/dim b/dim
> index ed26033f5aba..0a94b4284d80 100755
> --- a/dim
> +++ b/dim
> @@ -730,12 +730,11 @@ function checkpatch_commit_push
>  
>  	# use real names for people with many different email addresses
>  	author=$(git show -s $sha1 --format="format:%an")
> +	author_email=$(git show -s $sha1 --format="format:%ae")
>  	committer=$(git show -s $sha1 --format="format:%cn")
> -	# outlook mangles mails into "Last, First"
> -	author_outlook=$(git show -s $sha1 --format="format:%an" | sed -e 's/\([^ ]*\) \(.*\)/\2, \1/')
>  
> -	# check for author sign-off
> -	if ! git show -s $sha1 | grep -qi "S.*-by:.*\\($author\\|$author_outlook\\)" ; then
> +	# check for author sign-off: accept either email or name
> +	if ! git show -s $sha1 | grep -qi "S.*-by:.*\\($author\\|<$author_email>\\)\$"; then

To be honest current behavior is saving me. It saved me today again when merging DK's patch.
I prefer we continue checking only for the name and simply remove the atempt of outlook name.
So when merging patch this would block dim and give time to commiter to fix it.

>  		warn_or_fail "$sha1 is lacking author of sign-off"
>  	fi
>  
> ----->8-----
> 
> 
> Lucas De Marchi
> _______________________________________________
> dim-tools mailing list
> dim-tools at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools