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.
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(-)
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
(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.
(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.
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(-)
(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.
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.
Created attachment 42692 [details] Tidied patch
Landed in r50619.