| Summary: | Add a new function for getting the Git hash for a pure git directory. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jason Marcell <jmarcell> | ||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bshafiei, buildbot, dbates, ddkilzer, jmarcell, kocsen_chung, lforschler, matthew_hanson | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Jason Marcell
2017-04-28 14:28:10 PDT
Created attachment 308595 [details]
Patch
Comment on attachment 308595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review Perhaps we should warn the user if the directory is not a Git directory, since that is likely indicative of a mistake on the part of the caller. This is a great addition; The semicolon needs to be addressed and I'd like to see a warning when this function is called on non-Git repos. > Tools/Scripts/VCSUtils.pm:451 > + my $revision; I know that the variable name $revision is consistent with the other functions of this ilk, but you might consider calling it $commit or $hash to match the name of your function. > Tools/Scripts/VCSUtils.pm:454 > + warn "$directory is a pure git repo."; Is this debugging code? > Tools/Scripts/VCSUtils.pm:458 > + chomp($revision) Missing semicolon. Comment on attachment 308595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >> Tools/Scripts/VCSUtils.pm:454 >> + warn "$directory is a pure git repo."; > > Is this debugging code? I assume we also don't mind that git-svn repos will execute this code path. > Tools/Scripts/VCSUtils.pm:456 > + $command = "LC_ALL=C $command" if !isWindows(); Curious what is this for? Comment on attachment 308595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >> Tools/Scripts/VCSUtils.pm:451 >> + my $revision; > > I know that the variable name $revision is consistent with the other functions of this ilk, but you might consider calling it $commit or $hash to match the name of your function. Yep. I guess I'll go with $commit unless anyone objects. >>> Tools/Scripts/VCSUtils.pm:454 >>> + warn "$directory is a pure git repo."; >> >> Is this debugging code? > > I assume we also don't mind that git-svn repos will execute this code path. This is debugging code (which I will remove before landing). This function will work fine for a git-svn repo, however, we wouldn't/shouldn't use it because there is an already-existing function called `svnRevisionForDirectory` (which, not coincidentally, this function was heavily based on). >> Tools/Scripts/VCSUtils.pm:456 >> + $command = "LC_ALL=C $command" if !isWindows(); > > Curious what is this for? I'm not sure. It's some copy pasta from `svnRevisionForDirectory`. You're right to question it. We should figure out if we even need it here before landing. >> Tools/Scripts/VCSUtils.pm:458 >> + chomp($revision) > > Missing semicolon. Will fix. Comment on attachment 308595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >>> Tools/Scripts/VCSUtils.pm:456 >>> + $command = "LC_ALL=C $command" if !isWindows(); >> >> Curious what is this for? > > I'm not sure. It's some copy pasta from `svnRevisionForDirectory`. You're right to question it. We should figure out if we even need it here before landing. I've done some research. Why do this? From here: https://unix.stackexchange.com/a/87763 We're setting the locale to something generic in this case because the returned data is not intended for human consumption and, thus, does not need locale specific interpretation. > Any time where you process input data or output data that is not intended from/for a human. If you're talking to a user, you may want to use their convention and language, but for instance, if you generate some numbers to feed some other application that expects English style decimal points, or English month names, you'll want to set LC_ALL=C Why are we excluding Windows? https://bugs.webkit.org/show_bug.cgi?id=93339 Comment on attachment 308595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >>> Tools/Scripts/VCSUtils.pm:451 >>> + my $revision; >> >> I know that the variable name $revision is consistent with the other functions of this ilk, but you might consider calling it $commit or $hash to match the name of your function. > > Yep. I guess I'll go with $commit unless anyone objects. Actually, I decided to go with 'hash' because of the name I already chose for the function. Created attachment 308728 [details]
Patch
Comment on attachment 308728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308728&action=review r=me > Tools/Scripts/VCSUtils.pm:96 > &svnRevisionForDirectory > + &gitHashForDirectory > &svnStatus Please alphabetize within the list. |