Bug 171450 – Add a new function for getting the Git hash for a pure git directory.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Marcell]

Add a new function for getting the Git hash for a pure git directory.

[Jason Marcell]

Created attachment 308595 [details]
Patch

[Matthew Hanson]

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.

[Kocsen Chung]

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?

[Jason Marcell]

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.

[Jason Marcell]

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

[Jason Marcell]

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.

[Jason Marcell]

Created attachment 308728 [details]
Patch

[David Kilzer (:ddkilzer)]

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.