Re: [PATCH] Keep patch file permissions in mklog

[Tom de Vries <Tom_deVries@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

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


[-- Attachment #2: 0001-Add-inline-option-to-contrib-mklog.patch --]
[-- Type: text/x-patch, Size: 2472 bytes --]

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