webkit-patch failed to detect git repository

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Bill Ming]

Following error is thrown even user.name is set.

Failed to determine ChangeLog name.
Either:
  set CHANGE_LOG_NAME in your environment
  OR pass --name= on the command line
  OR set REAL_NAME in your environment  OR git users can set 'git config user.name'

This is because in VCSUtils.pm, isGitDirectory giving a wrong path to git.exe
Having single quote on *nix systems are fine but on Windows it only accepts double quote. Therefore, using double quote should work on all platforms.

[Bill Ming]

Created attachment 307146 [details]
Patch

[Daniel Bates]

Comment on attachment 307146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307146&action=review

Thank you for filing this bug and posting a patch. There are more places that we need to modify. You may find it useful to look at the patch on bug #169003,, which added the -C option to various code paths. Please grep through the code as well to find any other occurrences that have been added since the patch for bug #169003.

> Tools/ChangeLog:3
> +        This change makes isGitDirectory() work on Windows.

This line should be the Bugzilla bug title verbatim. The description of the change goes below the "Reviewed by" line.

[Bill Ming]

Created attachment 307162 [details]
Patch

[Daniel Bates]

Comment on attachment 307162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307162&action=review

The change looks good. The ChangeLog is out-of-date and I noticed some minor stylistic issues with it. Please update it. You do not need to post this patch for review again (though feel free to ask for one if you want one or make changes other than fixing the ChangeLog entry). When you update the ChangeLog entry change the  "Reviewed by NOBODY (OOPS!)" line to "Reviewed by Daniel Bates." and upload the patch without a review flag and with the cq flag set to ?.

> Tools/ChangeLog:3
> +        webkit-patch failed to detect git repository.

Minor: Remove the period at the end of this line so that it matches the Bugzilla title verbatim.

> Tools/ChangeLog:8
> +        When -C is given to git, using double quotes for the path.

Please add an empty line below this line.

> Tools/ChangeLog:10
> +        * Scripts/VCSUtils.pm:
> +        (isGitDirectory):

This file/function listing is out-of-date. Please generate this ChangeLog entry again using prepare-ChangeLog.

[Daniel Bates]

Comment on attachment 307162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307162&action=review

>> Tools/ChangeLog:8
>> +        When -C is given to git, using double quotes for the path.
> 
> Please add an empty line below this line.

Please elaborate on why we need to use double quotes for the path. From my understanding the Window prompt (cmd.exe) only supports quoting parameters using double quotes. That is, it does not support quoting parameters using single quotes.

[Daniel Bates]

(In reply to Daniel Bates from comment #5)
> [...] From my
> understanding the Window prompt (cmd.exe) only supports quoting parameters
> using double quotes.

You actually mentioned this in comment #0. For convenience, I suggest that you mention it as well in your ChangeLog description.

[Bill Ming]

Created attachment 307165 [details]
Patch

[Bill Ming]

Thanks for the review!

[Daniel Bates]

Comment on attachment 307165 [details]
Patch

Thank you for updating the patch, Bill.

r=me

[WebKit Commit Bot]

Comment on attachment 307165 [details]
Patch

Clearing flags on attachment: 307165

Committed r215381: <http://trac.webkit.org/changeset/215381>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.