[webkit-reviews] review denied: [Bug 16053] prepend git branch with build-webkit and friends : [Attachment 17404] prepend git branch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 19 23:04:34 PST 2007

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Adam Treat
<treat at kde.org>'s request for review:
Bug 16053: prepend git branch with build-webkit and friends

Attachment 17404: prepend git branch

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
>+sub gitBranch()
>+    unless (defined $gitBranch) {
>+	  chomp($gitBranch = `git symbolic-ref HEAD`);
>+	  $gitBranch =~ s/refs\/heads\///;

Please (1) anchor this using "^" and (2) use another character besides '/' to
make the regular expression more readable, e.g.:

$gitBranch =~ s#^refs/heads/##;

You may want to do something like this instead:

$gitBranch = pop @{[split('/', $gitBranch)]}

(Or maybe not if that's not readable by most folks. :)

>+	  $gitBranch =~ s/master//;

Should probably test for equality rather than using a non-anchored regex:

$gitBranch = "" if $gitBranch eq "master";

Otherwise the regex should be anchored with both "^" and "$".

Also, the floating head "(no branch)" case mentioned in Comment #2 and Comment
#3 needs to be addressed.  (You may want to test the exit status of the git
command using exitStatus() from webkitdirs.pm.)

>+sub isGitBranchBuild()
>+    my $branch = gitBranch();
>+    chomp(my $override = `git config branch.$branch.webkitbranchbuild`);
>+    return 1 if $override eq "true";
>+    return 0 if $override eq "false";
>+    unless (defined $isGitBranchBuild) {
>+	  chomp(my $gitBranchBuild = `git config --bool
>+	  $isGitBranchBuild = $gitBranchBuild eq "true";
>+    }

Why is "--bool" used in the second git-config call but not the first?  I think
the first command should also have a "--bool" switch.

r- to fix gitBranch() issues.

More information about the webkit-reviews mailing list