diff mbox

Keep patch file permissions in mklog

Message ID 53E86F4B.2020107@mentor.com
State New
Headers show

Commit Message

Tom de Vries Aug. 11, 2014, 7:22 a.m. UTC 
On 04-08-14 13:50, Yury Gribov wrote:
>  > thanks for the review.
>
> Np, I'm personally happy to see that script is useful.
>
>  > I've now interpreted it such that --inline prints to stdout what it
>  > would print to the patch file otherwise, that is, both log and patch.
>  > Printing just the log to stdout can be already be achieved by not using
>  > --inline.
>
> Could you add a note in the help message?
>

Done.

> +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {
>
> I'd do >= 1 but that's a question of personal preference.
>

What is the purpose of that proposed change ?

> +if ($inline && $diff ne "-") {
> +    $tmp = `mktemp`;
> +    if ($? != 0) {
> +        die "Could not generate temp file";
> +    }
>
> IMHO better use consistent style: system() or ticks with $?.
> Or even better, encapsulate environment calls in a subfunction which would check
> return code and return stdout.
>

I've realised that using cat to keep permissions on the patch file might not be 
a good idea: if it's interrupted, the original patch might be truncated. So I 
use mv at the end to atomically move the new contents to the patch file. And I 
implement keeping the permission by first copying the patch file to the temp 
file. That means that it's necessary to first truncate the temp file before 
writing to it. I've implemented the truncate using open.

I've tried using a temp file with just a file handle, as Segher suggested, (and 
use perl truncate to do the truncation), but I didn't get that working. So I'm 
now using mktemp from File::Temp.

With these modification, I've eliminated system() calls and backticks from the 
patch.

> BTW you may want to wait for Diego's and Trevor's comments (Diego is the
> maintainer and approver for this code).
>

ok.

Thanks,
- Tom

Comments

Yury Gribov Aug. 11, 2014, 8:18 a.m. UTC | #1 
On 08/11/2014 11:22 AM, Tom de Vries wrote:
>> +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {
>>
>> I'd do >= 1 but that's a question of personal preference.
>>
>
> What is the purpose of that proposed change ?

Kind of laying foundation for adding more flags in the future. But as I 
said, this could be done as need for new flags arises.

> I'm now using mktemp from File::Temp.

The script now relies on external Perl modules. This is probably fine 
because AFAIK File::Temp and File::Copy are available included in all 
distributions.

+use File::Copy "cp";
+use File::Copy "mv";

You could trade one line with qw(cp mv)...

+	$tmp = mktemp("tmp.XXXXXXXX") or die "cannot create temp file: $!";

Perhaps "Could not" like in other error messages?

+	cp "$diff", "$tmp" or die "Could not copy patch file to temp file: $!";

Script don't quote stuff in other places so quoting here is useless.

-Y
diff mbox

Patch

2014-08-11  Tom de Vries  <tom@codesourcery.com>

	* mklog: Add --inline option.

diff --git a/contrib/mklog b/contrib/mklog
index 3d17dc5..d294417 100755
--- a/contrib/mklog
+++ b/contrib/mklog
@@ -26,6 +26,10 @@ 
 # Author: Diego Novillo <dnovillo@google.com> and
 #         Cary Coutant <ccoutant@google.com>
 
+use File::Temp;
+use File::Copy "cp";
+use File::Copy "mv";
+
 # Change these settings to reflect your profile.
 $username = $ENV{'USER'};
 $name = `finger $username | grep -o 'Name: .*'`;
@@ -56,14 +60,22 @@  if (-d "$gcc_root/.git") {
 # Program starts here. You should not need to edit anything below this
 # line.
 #-----------------------------------------------------------------------------
-if ($#ARGV != 0) {
+$inline = 0;
+if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {
+	shift;
+	$inline = 1;
+} elsif ($#ARGV != 0) {
     $prog = `basename $0`; chop ($prog);
     print <<EOF;
-usage: $prog file.diff
+usage: $prog [ -i | --inline ] file.diff
 
 Generate ChangeLog template for file.diff.
 It assumes that patch has been created with -up or -cp.
+When -i is used, the ChangeLog template is followed by the contents of
+file.diff.
 When file.diff is -, read standard input.
+When -i is used and file.diff is not -, it writes to file.diff, otherwise it
+writes to stdout.
 EOF
     exit 1;
 }
@@ -273,8 +285,38 @@  foreach (@diff_lines) {
 # functions.
 $cl_entries{$clname} .= $change_msg ? "$change_msg\n" : ":\n";
 
+if ($inline && $diff ne "-") {
+	# Get a temp filename, rather than an open filehandle, because we use
+	# the open to truncate.
+	$tmp = mktemp("tmp.XXXXXXXX") or die "cannot create temp file: $!";
+
+	# Copy the permissions to the temp file (in perl 2.15 and later).
+	cp "$diff", "$tmp" or die "Could not copy patch file to temp file: $!";
+
+	# Open the temp file, clearing contents.
+	open (OUTPUTFILE, '>', $tmp) or die "Could not open temp file: $!";
+} else {
+	*OUTPUTFILE = STDOUT;
+}
+
+# Print the log
 foreach my $clname (keys %cl_entries) {
-	print "$clname:\n\n$hdrline\n\n$cl_entries{$clname}\n";
+	print OUTPUTFILE "$clname:\n\n$hdrline\n\n$cl_entries{$clname}\n";
+}
+
+if ($inline) {
+	# Append the patch to the log
+	foreach (@diff_lines) {
+		print OUTPUTFILE "$_\n";
+	}
+}
+
+if ($inline && $diff ne "-") {
+	# Close $tmp
+	close(OUTPUTFILE);
+
+	# Write new contents to $diff atomically
+	mv $tmp, $diff or die "Could not move temp file to patch file: $!";
 }
 
 exit 0;
-- 
1.9.1