Bug 27664 – commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Pierre d'Herbemont]

commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog.

It would be especially useful after being reviewed, and when editing the ChangeLogs of an already existing commit.

[Pierre d'Herbemont]

Created attachment 33465 [details]
2009-07-24  Pierre d'Herbemont  <[e-mail]>

Reviewed by NOBODY (OOPS!).

commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog
https://bugs.webkit.org/show_bug.cgi?id=27664

* Scripts/commit-log-editor: Add --allow-commit-log-suppression option.
The user is asked if he wants to suppress previous ChangeLog and regenerate it,
if this option is enabled.
---
 2 files changed, 32 insertions(+), 2 deletions(-)

[David Kilzer (:ddkilzer)]

Comment on attachment 33465 [details]
2009-07-24  Pierre d'Herbemont  <[e-mail]>

> +        * Scripts/commit-log-editor: Add --allow-commit-log-suppression option.
> +        The user is asked if he wants to suppress previous ChangeLog and regenerate it,
> +        if this option is enabled.

I think "--allow-commit-log-suppression" should have a better name.  Maybe --regenerate-log?

> +my $previousLogMessage = "";
[...]
> -    $logContents .= $_;
> +    if(!isGit() || (/^#/)) {
> +        $logContents .= $_;
> +    } else {
> +        $previousLogMessage .= $_;
> +    }
>      $existingLog = isGit() && !(/^#/ || /^\s*$/) unless $existingLog;

The $previousLogMessage contents don't seem to be used anywhere else.  What happens if you want to use the previousLogMessage?  Should $previousLogMessage be assigned (or appended) to $logContents?

> +my $keepExistingLog = 1;
> +if ($allowPreviousCommitLogSuppression && $existingLog) {
> +    print "Existing log message detected, enter 'd' to delete, and regenerate log message from ChangeLogs. Press Return to continue.\n";
> +    my $key = getc(STDIN);
> +    $keepExistingLog = 0 if ($key eq "d");
> +}

My reading of the code indicates that *any* key besides 'd' will cause the existing log to be kept.  I'd change the message to something like:

print "Existing log message detected.  Use 'r' to regenerate from ChangeLogs, or any other key to keep the existing message.\n";

r- to address the above issues.

[Pierre d'Herbemont]

(In reply to comment #2)
> (From update of attachment 33465 [details])
> > +        * Scripts/commit-log-editor: Add --allow-commit-log-suppression option.
> > +        The user is asked if he wants to suppress previous ChangeLog and regenerate it,
> > +        if this option is enabled.
> 
> I think "--allow-commit-log-suppression" should have a better name.  Maybe
> --regenerate-log?

Seems like a better choice.

> 
> > +my $previousLogMessage = "";
> [...]
> > -    $logContents .= $_;
> > +    if(!isGit() || (/^#/)) {
> > +        $logContents .= $_;
> > +    } else {
> > +        $previousLogMessage .= $_;
> > +    }
> >      $existingLog = isGit() && !(/^#/ || /^\s*$/) unless $existingLog;
> 
> The $previousLogMessage contents don't seem to be used anywhere else.  What
> happens if you want to use the previousLogMessage?  Should $previousLogMessage
> be assigned (or appended) to $logContents?

Yes, it's not being used here. It's here just because it's easier to read IMO. At second thought, it mades little sense, and a comment will probably be better.

We could add it back as a comment in the new changelog. I didn't like this very much because it most of the time clutters the view of the modified files etc.

> 
> > +my $keepExistingLog = 1;
> > +if ($allowPreviousCommitLogSuppression && $existingLog) {
> > +    print "Existing log message detected, enter 'd' to delete, and regenerate log message from ChangeLogs. Press Return to continue.\n";
> > +    my $key = getc(STDIN);
> > +    $keepExistingLog = 0 if ($key eq "d");
> > +}
> 
> My reading of the code indicates that *any* key besides 'd' will cause the
> existing log to be kept.  I'd change the message to something like:
> 
> print "Existing log message detected.  Use 'r' to regenerate from ChangeLogs,
> or any other key to keep the existing message.\n";

Actually, it's waiting for a return. I'll switch so that it uses Term::ReadKey package and actually just wait for one key. And change the comment.

Thanks for the review!

Pierre.

[Pierre d'Herbemont]

Created attachment 33585 [details]
2009-07-24  Pierre d'Herbemont  <[e-mail]>

Reviewed by NOBODY (OOPS!).

commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog
https://bugs.webkit.org/show_bug.cgi?id=27664

* Scripts/commit-log-editor: Add --regenerate-log option.
The user is asked if he wants to suppress previous ChangeLog and regenerate it,
if this option is enabled.
---
 2 files changed, 43 insertions(+), 2 deletions(-)

[Pierre d'Herbemont]

I have updated the patch so that:
- Review comment are integrated
- We implement usage() as a subroutine
- Just one key is read
- If there is no ChangeLog file modified we don't ask the user if he wants to get rid of its current log and regenerate the logs

[David Kilzer (:ddkilzer)]

Comment on attachment 33585 [details]
2009-07-24  Pierre d'Herbemont  <[e-mail]>

> +sub usage
> +{
> +    print "Usage: [--help] [--regenerate-log] <log file>\n";
> +    exit 0;
> +}

The usage() method should always exit with a non-zero status.  I suggest "exit 1;".

r=me with the above change.  Thanks!

[Pierre d'Herbemont]

https://trac.webkit.org/changeset/46453