[webkit-reviews] review denied: [Bug 27664] commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog : [Attachment 33465] 2009-07-24 Pierre d'Herbemont <pdherbemont at apple.com>

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Pierre d'Herbemont
<pdherbemont at apple.com>'s request for review:
Bug 27664: 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

Attachment 33465: 2009-07-24  Pierre d'Herbemont  <pdherbemont at apple.com>
https://bugs.webkit.org/attachment.cgi?id=33465&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> +	   * 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.