Hello Daniel,
Dear all,
thanks for your detailed review, Daniel. Finally got some more leisure
time to care about it.
I deceided to concentrate on the first part to handle property-modified
files plus your overall comments.
Left the Windows support aside as this has to be fine-tuned a lot, as I
learned a lot from your comments, thanks.
On 2020-09-12 05:14, Daniel Shahaf wrote:
> Nathan Hartman wrote on Fri, Sep 11, 2020 at 10:38:24 -0400:
>> Subject: [PATCH] Check also property-modified files in hook script
>> check-mime-type.pl
>>
>> * contrib/hook-scripts/check-mime-type.pl:
>> (proplist) add property-modified files for checks
>
> There are several changes not mentioned in the log message:
>
> - Script name fix in comments
>
> - Move 'use' before 'BEGIN'
> (incidentally, I'm not actually sure that's a correct change, given
> the BEGIN block implies perls as old as 5.4 are supported.)
>
> - Add docstrings
>
>> +++ b/contrib/hook-scripts/check-mime-type.pl
>> @@ -1,18 +1,19 @@
>> #!/usr/bin/env perl
>>
>> #
>> ====================================================================
>> -# commit-mime-type-check.pl: check that every added file has the
>> -# svn:mime-type property set and every added file with a mime-type
>> -# matching text/* also has svn:eol-style set.
>> +# check-mime-type.pl: check that every added or property-modified
>> file
>> +# has the svn:mime-type property set and every added or
>> property-modified
>> +# file with a mime-type matching text/* also has svn:eol-style set.
>
> I don't think it's acceptable for a script called "check-mime-type.pl"
> to start enforcing svn:eol-style as well. This violates the principles
> of least surprise and of "Do one thing and do it well". Also, the
> choice of text/* is a bit arbitrary, and would seem to have both false
> positives and false negatives: e.g., application/xml and text/x-diff.
>
> I'm not even sure it's acceptable for this script to start enforcing
> svn:mime-type's presence upon propmods, either. It's not exactly
> a natural extension that existing users would expect.
First I am not the original author of that hook script, just using it
with those modifications with Subversion 1.6 up to 1.14.0 on
Debian/Ubuntu and Windows.
Some people (like me) want to enforce "svn:mime-type" for some
repositories and using that script is optional to admins.
I agree with the original author to check mime-type plus EoL in a single
script, as EoL setting can be important to text files.
Enforcing a policy on creation but not on changes is inconsistent,
that's the reason for this fix.
But I do understand your worries and made the functionalities optional,
setting the default exactly as it acts right now.
Completely removing the EoL check would change its fucntionality after
16 years.
New version of the patch, log and script itself attached. Will change
the Pull Request according to upcoming comments.
Additionally it could be renamed, e.g. to
"check-mime-type-and-text-eol.pl"?
>> #
>> ====================================================================
>> # Copyright (c) 2000-2004 CollabNet. All rights reserved.
>> +# Copyright (c) 2014-2020 Apache Software Foundation (ASF).
>
> How did you come up with these numbers? The script received commits in
> 2013 and 2015, so the end point should be 2013. (Also, the next line
> talks of a "COPYING" file which doesn't exist, but that's a preëxisting
> problem.)
Those lines are inherited from commit-access-control.pl, when it was
copied as a base for check-mime-type.pl.
Copyright lines and license were not updated by CollabNet or ASF despite
new commits since 2004.
I used 2014 as a start as this was the time this patch was posted on the
ASF mailing list.
Maybe coyprigght and license should be adapted to the current version of
commit-access-control.pl either with or without copyright lines.
According to the overall Subversion history it may should be changed to:
# Copyright (c) 2000-2009 CollabNet. All rights reserved.
# Copyright (c) 2010-2020 Apache Software Foundation (ASF).
>> @@ -100,19 +101,20 @@ my $tmp_dir = '/tmp';
>> -# Figure out what files have added using svnlook.
>> -my @files_added;
>> +# Figure out what files have been added/property-modified using
>> svnlook.
>> +my @paths_to_check;
>> +my $props_changed_re = qr/^(?:A |[U_ ]U) (.*[^\/])$/;
>> foreach my $line (&read_from_process($svnlook, 'changed', $repos,
>> '-t', $txn))
>> {
>> - # Add only files that were added to @files_added
>> - if ($line =~ /^A. (.*[^\/])$/)
>> + # Add only files that were added/property-modified to
>> @paths_to_check
>> + if ($line =~ /$props_changed_re/)
>
> Why did you change /^A./ to /^A /?
The RegEx was taken from Leo Davis' mail, but I agree it should be /^A./
for added files and also /^.U/ for property-modified files.
>> Subject: [PATCH] Enhanced platform support of hook script
>> check-mime-type.pl
>>
>> * contrib/hook-scripts/check-mime-type.pl:
>> (svnlook) Cross Platform: sane executable determination
>
>> +++ b/contrib/hook-scripts/check-mime-type.pl
>> @@ -28,6 +28,15 @@
>>
>> use strict;
>> use Carp;
>> +use Config;
>> +use Cwd;
>
> «use Cwd qw(getcwd);» would be better.
>
>> +use Env;
>
> Why?
>
> This syntax (according to «perldoc Env») "ties all existing environment
> variables ("keys %ENV") to scalars". This seems like a dangerous thing
> to do; in fact, it negates the «use strict» a few lines above.
>
> I suspect this line should be deleted.
>
>> +use File::Which qw(which);
>> +use File::Temp qw(tempdir tempfile);
>> +
>> +my $old_dir;
>> +my $os_windows = $^O eq 'MSWin32';
>> +my $os_dos = $^O eq 'dos';
>>
>> # Turn on warnings the best way depending on the Perl version.
>> BEGIN {
>> @@ -35,6 +44,12 @@ BEGIN {
>> { require warnings; import warnings; }
>> else
>> { $^W = 1; }
>> +
>> + $old_dir = getcwd;
>> +}
>> +
>> +END {
>> + chdir($old_dir);
>> }
>
> Why is this necessary?
On Windows a sub script called by the precommit hook can change the
current directory whcih also affects the precommit hook, which had led
to weird results for the following sub scripts by the precommit hook.
This should have been explained in a comment.
>> @@ -42,31 +57,58 @@ BEGIN {
>> # Configuration section.
>>
>> # Svnlook path.
>> -my $svnlook = "/usr/bin/svnlook";
>> +my $svnlook;
>>
>> # Since the path to svnlook depends upon the local installation
>> # preferences, check that the required program exists to insure that
>> # the administrator has set up the script properly.
>> {
>> - my $ok = 1;
>> - foreach my $program ($svnlook)
>> + my $svnlook_exe = 'svnlook' . $Config{_exe};
>> + my @svnlook_paths;
>> +
>> + if ($os_windows || $os_dos)
>> + {
>> + # On Windows/DOS the environment is available
>> + my $svn_bindir = $ENV{'SVN_BINDIR'};
>
> Shouldn't the condition be «if (defined $ENV{SVN_BINDIR})»?
>
>> + if (defined $svn_bindir and length $svn_bindir)
>> + {
>> + $svnlook_paths[++$#svnlook_paths] =
>> "$svn_bindir/$svnlook_exe";
>
> Why does this line use «$arrayvar[++$#arrayvar] = $foo» rather than
> «push @arrayvar, $foo», which is equivalent and idiomatic?
Just because I'm not a Perl expert.
That's why I postponed a patch for Windows support.
>> + }
>> +
>> + $svnlook = which($svnlook_exe);
>> + if (defined $svnlook and length $svnlook)
>> + {
>> + $svnlook_paths[++$#svnlook_paths] = $svnlook;
>> + }
>
> Why do we bother to have $svnlook tested for existence and
> executability? Don't we trust File::Which::which() to work as
> advertised?
>
>> + undef $svnlook;
>> + }
>> +
>> + # On Unix the environment is empty, define fallback with absolute
>> path
>> + $svnlook_paths[++$#svnlook_paths] = "/usr/bin/$svnlook_exe";
>> + foreach my $program (@svnlook_paths)
>> {
>> if (-e $program)
>> {
>> unless (-x $program)
>> {
>> - warn "$0: required program `$program' is not
>> executable, ",
>> - "edit $0.\n";
>> - $ok = 0;
>> + warn "$0: program `$program' is not executable.\n";
>> + next;
>> }
>> + $svnlook = $program;
>> + last;
>> }
>> else
>> {
>> - warn "$0: required program `$program' does not exist, edit
>> $0.\n";
>> - $ok = 0;
>> + warn "$0: program `$program' does not exist.\n";
>> + next;
>> }
>> }
>
> What's the point of issuing a warning when there are some more
> iterations yet to go?
>
> If I'm reading this correctly, this entire block should be reduced to:
>
> # Use «||» rather than «//» since the BEGIN block implies versions
> of Perl that lack «//» are supported.
> my $svnlook = +(defined($ENV{SVN_BINDIR}) ?
> "$ENV{SVN_BINDIR}/$svnlook_exe" : undef) || which("svnlook") ||
> "/usr/bin/svnlook";
> die … unless -x $svnlook and (-f _ or -l _);
>
> I changed -e to (-f or -l) since -x implies -e.
>
> I also disabled the fallbacks when the envvar is set but doesn't point
> to a usable executable. I'm not married to these semantics, but I
> think
> these semantics may be expected _given the particular variable name_:
> SVN_BINDIR is the name of the configure AC_SUBST() variable that's
> cooked into various scripts installed by install-tools.
>
>> - exit 1 unless $ok;
>> + unless (defined $svnlook and length $svnlook)
>> + {
>> + warn "$0: required program `$svnlook_exe' does not exist or is
>> not executable, maybe have to edit $0.\n";
>> + exit 1;
>> + }
>> }
>>
>>
>> ######################################################################
>> @@ -95,9 +137,9 @@ sub ACCESS_READ_WRITE () { 'read-write' }
>>
>> ######################################################################
>> # Harvest data using svnlook.
>>
>> -# Change into /tmp so that svnlook diff can create its .svnlook
>> +# Change into temp dir so that svnlook diff can create its .svnlook
>
> When this script was first added, svnlook would create
> a $TMPDIR/svnlook.$unique_integer directory (without a leading dot, and
> in $TMPDIR rather than in cwd). svnlook doesn't do that nowadays, so
> I see no reason to chdir at all — …
>
>> # directory.
>> -my $tmp_dir = '/tmp';
>> +my $tmp_dir = tempdir( TMPDIR => 1, CLEANUP => 1 );
>
> … in which case, this line — though otherwise correct — should simply
> be
> deleted altogether, and the File::Temp and Cwd uses with it.
>
>> @@ -220,19 +262,66 @@ sub safe_read_from_pipe
>> + while ($command = shift)
>> + {
>> + if ("-m" eq $command)
>
> - This is a general-purpose "run a child process" function, so it can't
> special-case "-m".
>
> - If it could, it would have to check for all four variants: «-m foo»,
> «-mfoo», «--message=foo», and «--message foo».
>
> - It would also have to look for the «--» end-of-options guard, since
> «-m» may occur after one. (This can actually happen in the second
> callsite of read_from_process().)
>
> - None of the callers pass -m in the first place, so this is dead code.
>
>> + {
>> + $comment = shift;
>> + my ($handle, $tmpfile) = tempfile( DIR => $tmp_dir);
>> + print $handle $comment;
>> + close($handle);
>> +
>> + push(@commandline, "--file");
>> + push(@commandline, $tmpfile);
>
> There may have been a "--" end-of-options guard earlier on the command
> line.
>
>> + }
>> + else
>> + {
>> + # Munge the command to protect it from the command line
>> + $command =~ s/\"/\\\"/g;
>> + if ($command =~ m"\s") { $command = "\"$command\""; }
>> + if ($command eq "") { $command = "\"\""; }
>
> I'm pretty sure this escaping is incomplete. Use a library function
> rather than roll your own escaping.
>
>> + if ($command =~ m"\n")
>> + {
>> + warn "$0: carriage return detected in command - may
>> not work\n";
>
> If it "may not work", then die() rather than warn().
>
>> + }
>> + push(@commandline, $command);
> ⋮
>> + open(SAFE_READ, "@commandline |")
>
>> They can be applied in sequence:
>> svn-patch-01-c73cb4415d.txt
>> svn-patch-02-68cc555740.txt
>>
>> In principle I think the changes are useful, though might change
>> behavior in a surprising way for those who are using the hook as it
>> stands now.
>>
>
> Usefulness aside, I think the first patch is unacceptable because of
> backwards compatibility considerations and the second patch would not
> be
> acceptable unless revised.
>
>> Would it make sense to copy check-mime-type.pl, calling it, say,
>> check-mime-type-2.pl, and apply the changes to that file, keeping the
>> original as it is now?
>
> For the first patch, moving the new behaviours to a new script would
> address the backwards compatibility point, as would making the new
> behaviours optional and opt-in. However, neither would address the
> other review points.
>
> The second patch doesn't appear to raise any backwards compatibility
> concerns.
>
> Cheers,
> Daniel
Kind regards
Maddes
Received on 2020-12-21 16:22:41 CET