Bug 28168 – commit-log-editor does not support all the email config that prepare-Changelog supports

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Pierre d'Herbemont]

commit-log-editor does not support all the email config that prepare-Changelog supports.

This is problematic as an extra unneeded "Patch by XXX on DD-DD-DD" might be added even though committer and author are the same person.

[Pierre d'Herbemont]

Created attachment 34527 [details]
commit-log-editor does not support all the email config that prepare-Changelog supports

https://bugs.webkit.org/show_bug.cgi?id=28168

Reviewed by NOBODY (OOPS!).

* Scripts/VCSUtils.pm: Move gitConfig() to VCSUtils.pm.
* Scripts/prepare-ChangeLog: ditto.
* Scripts/commit-log-editor: Use CHANGE_LOG_EMAIL_ADDRESS
and gitConfig("user.email").
---
 4 files changed, 30 insertions(+), 16 deletions(-)

[Mark Rowe (bdash)]

Comment on attachment 34527 [details]
commit-log-editor does not support all the email config that prepare-Changelog supports

> +sub gitConfig($)
> +{
> +    return unless $isGit;
> +    
> +    my ($config) = @_;
> +    
> +    my $result = `git config $config`;
> +    if (($? >> 8) != 0) {
> +        $result = `git repo-config $config`;
> +    }
> +    chomp $result;
> +    return $result;
> +}

In theory "$? >> 8" would be more portable as exitStatus($?).

>  1;
> diff --git a/WebKitTools/Scripts/commit-log-editor b/WebKitTools/Scripts/commit-log-editor
> index b783ccb..fe138ae 100755
> --- a/WebKitTools/Scripts/commit-log-editor
> +++ b/WebKitTools/Scripts/commit-log-editor
> @@ -171,7 +171,10 @@ for my $changeLog (@changeLogs) {
>  
>              # Attempt to insert the "patch by" line, after the first blank line.
>              if ($previousLineWasBlank && $hasAuthorInfoToWrite && $lineCount > 0) {
> -                my $authorAndCommitterAreSamePerson = $email eq $ENV{'EMAIL_ADDRESS'};
> +                my $committerEmail = $ENV{CHANGE_LOG_EMAIL_ADDRESS}
> +                                    || $ENV{EMAIL_ADDRESS}
> +                                    || gitConfig("user.email");
> +                my $authorAndCommitterAreSamePerson = $email eq $committerEmail;

What will happen if the email address is not set via any of these means?  Will it spew a perl warning and continue, or will it display a useful message like prepare-ChangeLog does?  Should we consider removing the duplication of the retrieving of these values by pushing them down in to a function?

r=me

[Pierre d'Herbemont]

(In reply to comment #2)
> (From update of attachment 34527 [details])
> > +sub gitConfig($)
> > +{
> > +    return unless $isGit;
> > +    
> > +    my ($config) = @_;
> > +    
> > +    my $result = `git config $config`;
> > +    if (($? >> 8) != 0) {
> > +        $result = `git repo-config $config`;
> > +    }
> > +    chomp $result;
> > +    return $result;
> > +}
> 
> In theory "$? >> 8" would be more portable as exitStatus($?).

Ok. (This was copy and paste)

> >  1;
> > diff --git a/WebKitTools/Scripts/commit-log-editor b/WebKitTools/Scripts/commit-log-editor
> > index b783ccb..fe138ae 100755
> > --- a/WebKitTools/Scripts/commit-log-editor
> > +++ b/WebKitTools/Scripts/commit-log-editor
> > @@ -171,7 +171,10 @@ for my $changeLog (@changeLogs) {
> >  
> >              # Attempt to insert the "patch by" line, after the first blank line.
> >              if ($previousLineWasBlank && $hasAuthorInfoToWrite && $lineCount > 0) {
> > -                my $authorAndCommitterAreSamePerson = $email eq $ENV{'EMAIL_ADDRESS'};
> > +                my $committerEmail = $ENV{CHANGE_LOG_EMAIL_ADDRESS}
> > +                                    || $ENV{EMAIL_ADDRESS}
> > +                                    || gitConfig("user.email");
> > +                my $authorAndCommitterAreSamePerson = $email eq $committerEmail;
> 
> What will happen if the email address is not set via any of these means?  Will
> it spew a perl warning and continue, or will it display a useful message like
> prepare-ChangeLog does?  Should we consider removing the duplication of the
> retrieving of these values by pushing them down in to a function?

Nice idea. However I failed to see the proper place to factorize such a fonction. I'll resend a patch with a factorization.

[Pierre d'Herbemont]

(In reply to comment #2)
> (From update of attachment 34527 [details])
> > +sub gitConfig($)
> > +{
> > +    return unless $isGit;
> > +    
> > +    my ($config) = @_;
> > +    
> > +    my $result = `git config $config`;
> > +    if (($? >> 8) != 0) {
> > +        $result = `git repo-config $config`;
> > +    }
> > +    chomp $result;
> > +    return $result;
> > +}
> 
> In theory "$? >> 8" would be more portable as exitStatus($?).

In fact exitStatus() is not available from here. I am not sure if we want to do a circular reference loop between webkitdir.pm and VCSUtil.pm. I'll leave it here, as this was old code.

[Pierre d'Herbemont]

Created attachment 34528 [details]
commit-log-editor does not support all the email config that prepare-Changelog supports

https://bugs.webkit.org/show_bug.cgi?id=28168

Reviewed by NOBODY (OOPS!).

* Scripts/VCSUtils.pm: Move gitConfig() to VCSUtils.pm.
* Scripts/webkitdirs.pm: Move changeLogEmailAddress() to webkitdirs.pm.
* Scripts/prepare-ChangeLog: ditto.
* Scripts/commit-log-editor: Use changeLogEmailAddress().
---
 5 files changed, 56 insertions(+), 37 deletions(-)

[Pierre d'Herbemont]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 34527 [details] [details])
> > > +sub gitConfig($)
> > > +{
> > > +    return unless $isGit;
> > > +    
> > > +    my ($config) = @_;
> > > +    
> > > +    my $result = `git config $config`;
> > > +    if (($? >> 8) != 0) {
> > > +        $result = `git repo-config $config`;
> > > +    }
> > > +    chomp $result;
> > > +    return $result;
> > > +}
> > 
> > In theory "$? >> 8" would be more portable as exitStatus($?).
> 
> In fact exitStatus() is not available from here. I am not sure if we want to do
> a circular reference loop between webkitdir.pm and VCSUtil.pm. I'll leave it
> here, as this was old code.

I just added webkitdir.pm to prepare-ChangeLog to fix this.

[Eric Seidel (no email)]

Comment on attachment 34528 [details]
commit-log-editor does not support all the email config that prepare-Changelog supports

Should this be an empty return as such?
 224     return unless $isGit;
not '' ?

Style:
229     if (exitStatus($?) != 0) {
 230         $result = `git repo-config $config`;
 231     }

Shouldn't you move the user.name stuff into webkitdirs.pm too?

Seems we should be splitting some of this stuff out into separate modules from webkitdirs.pm anyway...

And geeeez we need to move commit-log-editor to python. :(

r- for the style nits.  Consider breaking these out of this huge (poorly named) module.

[Mark Rowe (bdash)]

Created attachment 42692 [details]
Tidied patch

[Mark Rowe (bdash)]

Landed in r50619.