Bug 171450 - Add a new function for getting the Git hash for a pure git directory.
Summary: Add a new function for getting the Git hash for a pure git directory.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-28 14:28 PDT by Jason Marcell
Modified: 2017-05-01 16:15 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.71 KB, patch)
2017-04-28 14:29 PDT, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (1.62 KB, patch)
2017-05-01 10:05 PDT, Jason Marcell
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Marcell 2017-04-28 14:28:10 PDT
Add a new function for getting the Git hash for a pure git directory.
Comment 1 Jason Marcell 2017-04-28 14:29:40 PDT
Created attachment 308595 [details]
Patch
Comment 2 Matthew Hanson 2017-04-28 14:38:39 PDT
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 3 Kocsen Chung 2017-04-28 14:47:27 PDT
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 4 Jason Marcell 2017-04-28 15:19:21 PDT
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 5 Jason Marcell 2017-05-01 09:52:07 PDT
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 6 Jason Marcell 2017-05-01 09:59:45 PDT
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.
Comment 7 Jason Marcell 2017-05-01 10:05:20 PDT
Created attachment 308728 [details]
Patch
Comment 8 David Kilzer (:ddkilzer) 2017-05-01 12:11:08 PDT
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.