[PATCH] Keep patch file permissions in mklog

[PATCH] Keep patch file permissions in mklog

[Tom de Vries]

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

Geoff,

Currently, mklog resets the permissions on a patch file:
...
$ touch tmp.patch
$ ls -la tmp.patch
-rw-rw-r-- 1 vries vries 0 mei 31 09:17 tmp.patch
$ ./contrib/mklog tmp.patch
$ ls -la tmp.patch
-rw------- 1 vries vries 59 mei 31 09:17 tmp.patch
$
...

This patch fixes that:
...
$ rm tmp.patch
$ touch tmp.patch
$ ls -la tmp.patch
-rw-rw-r-- 1 vries vries 0 mei 31 09:41 tmp.patch
$ ./contrib/mklog tmp.patch
$ ls -la tmp.patch
-rw-rw-r-- 1 vries vries 59 mei 31 09:41 tmp.patch
...

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-mklog-permissions-fix.patch --]
[-- Type: text/x-patch, Size: 538 bytes --]

2014-05-31  Tom de Vries  <tom@codesourcery.com>

	* mklog: Keep permissions on patch file.

diff --git a/contrib/mklog b/contrib/mklog
index fb489b0..d6f3556 100755
--- a/contrib/mklog
+++ b/contrib/mklog
@@ -264,7 +264,7 @@ foreach my $clname (keys %cl_entries) {
 }
 
 # Concatenate the ChangeLog template and the original .diff file.
-system ("cat $diff >>$temp && mv $temp $diff") == 0
+system ("cat $diff >>$temp && cat $temp >$diff && rm $temp") == 0
     or die "Could not add the ChangeLog entry to $diff";
 
 exit 0;
-- 
1.9.1

Re: [PATCH] Keep patch file permissions in mklog

[Yury Gribov]

Cc Diego.


-----------------------------------------------
From: Tom de Vries <Tom_deVries@mentor.com>
Sent:  Saturday, May 31, 2014 11:47AM
To: Geoff Keating <geoffk@geoffk.org>
Cc: GCC Patches
Subject: [PATCH] Keep patch file permissions in mklog

On 05/31/2014 11:47 AM, Tom de Vries wrote:
Geoff,

Currently, mklog resets the permissions on a patch file:
...
$ touch tmp.patch
$ ls -la tmp.patch
-rw-rw-r-- 1 vries vries 0 mei 31 09:17 tmp.patch
$ ./contrib/mklog tmp.patch
$ ls -la tmp.patch
-rw------- 1 vries vries 59 mei 31 09:17 tmp.patch
$
...

This patch fixes that:
...
$ rm tmp.patch
$ touch tmp.patch
$ ls -la tmp.patch
-rw-rw-r-- 1 vries vries 0 mei 31 09:41 tmp.patch
$ ./contrib/mklog tmp.patch
$ ls -la tmp.patch
-rw-rw-r-- 1 vries vries 59 mei 31 09:41 tmp.patch
...

OK for trunk?

Thanks,
- Tom

Re: [PATCH] Keep patch file permissions in mklog

[Tom de Vries]

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

On 01-08-14 06:21, Yury Gribov wrote:
> Cc Diego.
>
> -----------------------------------------------
> From: Tom de Vries <Tom_deVries@mentor.com>
> Sent:  Saturday, May 31, 2014 11:47AM
> To: Geoff Keating <geoffk@geoffk.org>
> Cc: GCC Patches
> Subject: [PATCH] Keep patch file permissions in mklog
>
> On 05/31/2014 11:47 AM, Tom de Vries wrote:
> Geoff,
>
> Currently, mklog resets the permissions on a patch file:

Yuri,
Diego,

The permissions problem does not occur anymore with the latests versions of 
mklog, since it doesn't modify the patch file anymore.

This patch adds a script contrib/mklog-in-patch, which uses mklog to generate 
the skeleton log, but generates the log at the start of the patch as mklog did 
before (which is how I like to use it).

OK for trunk?

I can also try to add an --inline option to mklog instead.

Thanks,
- Tom



[-- Attachment #2: 0001-Add-mklog-in-patch.patch --]
[-- Type: text/x-patch, Size: 1868 bytes --]

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

	* mklog-in-patch: New script.

diff --git a/contrib/mklog-in-patch b/contrib/mklog-in-patch
new file mode 100755
index 0000000..6d041eb
--- /dev/null
+++ b/contrib/mklog-in-patch
@@ -0,0 +1,61 @@
+#!/bin/sh
+
+# Copyright (C) 2014 Free Software Foundation, Inc.
+#
+# This file is part of GCC.
+#
+# GCC is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3, or (at your option)
+# any later version.
+#
+# GCC is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GCC; see the file COPYING.  If not, write to
+# the Free Software Foundation, 51 Franklin Street, Fifth Floor,
+# Boston, MA 02110-1301, USA.
+
+
+# Check for errors
+set -e
+
+scriptdir=$(dirname $0)
+prog=$(basename $0)
+
+diff="$1"
+
+if [ "$diff" = "" ]; then
+   usage
+    echo <<EOF
+usage: $prog file.diff
+
+Invokes mklog, and prepends the generated log to file.diff.
+EOF
+elif [ ! -r "$diff" ]; then
+    echo "Could not open file $diff for reading"
+    exit 1
+fi
+
+temp=$(mktemp)
+
+# Generate log in $temp
+if ! $scriptdir/mklog "$diff" > "$temp"; then
+    echo "Could not write log file to $temp"
+    exit 1
+fi
+
+# Append patch to $temp, and move $temp to $patch.  For the latter, we use cat
+# rather than mv to keep $diff permissions the same.
+if ! ( cat $diff >>$temp && cat $temp >$diff ); then
+    echo "Could not add the ChangeLog entry to $diff"
+    exit 1
+fi
+
+if ! rm $temp; then
+    echo "Could not remove $temp"
+    exit 1
+fi

Re: [PATCH] Keep patch file permissions in mklog

[Yury Gribov]

On 08/01/2014 10:52 AM, Tom de Vries wrote:
> This patch adds a script contrib/mklog-in-patch, which uses mklog to
> generate the skeleton log, but generates the log at the start of the
> patch as mklog did before (which is how I like to use it).

Yeah, we had some argument about this but kind of agreed that separate 
log is preferred.

> I can also try to add an --inline option to mklog instead.

I'd prefer this.

-Y

Re: [PATCH] Keep patch file permissions in mklog

[Tom de Vries]

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

On 01-08-14 09:18, Yury Gribov wrote:
> On 08/01/2014 10:52 AM, Tom de Vries wrote:
>> This patch adds a script contrib/mklog-in-patch, which uses mklog to
>> generate the skeleton log, but generates the log at the start of the
>> patch as mklog did before (which is how I like to use it).
>
> Yeah, we had some argument about this but kind of agreed that separate log is
> preferred.
>
>> I can also try to add an --inline option to mklog instead.
>
> I'd prefer this.
>

This patch implements an --inline option to mklog.

I've used the --inline option to prepend the skeleton log to this patch.

OK for trunk?

Thanks,
- Tom


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

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

	* mklog: Add --inline option.

diff --git a/contrib/mklog b/contrib/mklog
index 3d17dc5..ba075cf 100755
--- a/contrib/mklog
+++ b/contrib/mklog
@@ -56,19 +56,27 @@ 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")) {
+	$diff = $ARGV[1];
+	$inline = 1;
+	if ($diff eq "-") {
+	        die "Reading from - and using -i are not compatible";
+	}
+} 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 file.diff is -, read standard input.
 EOF
     exit 1;
+} else {
+	$diff = $ARGV[0];
 }
 
-$diff = $ARGV[0];
 $dir = `dirname $diff`; chop ($dir);
 $basename = `basename $diff`; chop ($basename);
 $hdrline = "$date  $name  <$addr>";
@@ -273,8 +281,32 @@ foreach (@diff_lines) {
 # functions.
 $cl_entries{$clname} .= $change_msg ? "$change_msg\n" : ":\n";
 
+if ($inline) {
+	$tmp = `mktemp`;
+	chomp ($tmp);
+	open (FILE1, '>', $tmp) or die "Could not open temp file";
+} else {
+    *FILE1 = STDOUT;
+}
+
+# Print the log
 foreach my $clname (keys %cl_entries) {
-	print "$clname:\n\n$hdrline\n\n$cl_entries{$clname}\n";
+	print FILE1 "$clname:\n\n$hdrline\n\n$cl_entries{$clname}\n";
+}
+
+
+# Prepend the log to the patch
+if ($inline) {
+	close (FILE1);
+
+	system ("cat $diff >>$tmp") == 0
+		or die "Could not append patch to temp file";
+
+	# We're using cat rather than move, to keep permissions on $diff the same.
+	system ("cat $tmp >$diff") == 0
+		or die "Could not move temp file to patch file";
+
+	unlink ($tmp) == 1 or die "Could not remove temp file";
 }
 
 exit 0;
-- 
1.9.1

Re: [PATCH] Keep patch file permissions in mklog

[Yury Gribov]

Thanks! My 2 (actually 4) cents below.

 > +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {
 > +	$diff = $ARGV[1];

Can we shift here and then just set $diff to $ARGV[0] unconditionally?

 > +	if ($diff eq "-") {
 > +	        die "Reading from - and using -i are not compatible";
 > +	}

Hm, can't we dump ChangeLog to stdout in that case?
The limitation looks rather strange.

 > +	open (FILE1, '>', $tmp) or die "Could not open temp file";

Could we use more descriptive name?

 > +	system ("cat $diff >>$tmp") == 0
 > +		or die "Could not append patch to temp file";
 > ...
 > +	unlink ($tmp) == 1 or die "Could not remove temp file";

The checks look like an overkill given that we don't check for result of 
mktemp...

-Y

Re: [PATCH] Keep patch file permissions in mklog

[Tom de Vries]

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

On 04-08-14 08:45, Yury Gribov wrote:
> Thanks! My 2 (actually 4) cents below.
>

Hi Yuri,

thanks for the review.

>  > +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {
>  > +    $diff = $ARGV[1];
>
> Can we shift here and then just set $diff to $ARGV[0] unconditionally?
>

Done.

>  > +    if ($diff eq "-") {
>  > +            die "Reading from - and using -i are not compatible";
>  > +    }
>
> Hm, can't we dump ChangeLog to stdout in that case?
> The limitation looks rather strange.
>

My original idea here was that --inline means 'in the patch file', which is not 
possible if the patch comes from stdin.

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.

>  > +    open (FILE1, '>', $tmp) or die "Could not open temp file";
>
> Could we use more descriptive name?
>

I've used the slightly more descriptive 'OUTPUTFILE'.

>  > +    system ("cat $diff >>$tmp") == 0
>  > +        or die "Could not append patch to temp file";
>  > ...
>  > +    unlink ($tmp) == 1 or die "Could not remove temp file";
>
> The checks look like an overkill given that we don't check for result of mktemp...
>

I've added a check for the result of mktemp, and removed the unlink result 
check. I've left in the "Could not append patch to temp file" check because the 
patch file might be read-only.

OK for trunk?

Thanks,
- Tom


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

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

	* mklog: Add --inline option.

diff --git a/contrib/mklog b/contrib/mklog
index 3d17dc5..27a0929 100755
--- a/contrib/mklog
+++ b/contrib/mklog
@@ -56,10 +56,14 @@ 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.
@@ -273,8 +277,39 @@ foreach (@diff_lines) {
 # functions.
 $cl_entries{$clname} .= $change_msg ? "$change_msg\n" : ":\n";
 
+if ($inline && $diff ne "-") {
+	$tmp = `mktemp`;
+	if ($? != 0) {
+		die "Could not generate temp file";
+	}
+	chomp ($tmp);
+	open (OUTPUTFILE, '>', $tmp) or die "Could not open temp file $tmp";
+} 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";
+}
+
+# Append the patch to the log
+if ($inline) {
+	foreach (@diff_lines) {
+		print OUTPUTFILE "$_\n";
+	}
+}
+
+# Replace the patch with the temp file
+if ($inline && $diff ne "-") {
+	close (OUTPUTFILE);
+
+	# We're using cat rather than move, to keep permissions on $diff the
+	# same.
+	system ("cat $tmp >$diff") == 0
+		or die "Could not move temp file to patch file";
+
+	unlink ($tmp);
 }
 
 exit 0;
-- 
1.9.1

Re: [PATCH] Keep patch file permissions in mklog

[Yury Gribov]

 > 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?

+if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {

I'd do >= 1 but that's a question of personal preference.

+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.

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

-Y

Re: [PATCH] Keep patch file permissions in mklog

[Segher Boessenkool]

> +if ($inline && $diff ne "-") {
> +	$tmp = `mktemp`;
> +	if ($? != 0) {
> +		die "Could not generate temp file";
> +	}
> 
> IMHO better use consistent style: system() or ticks with $?.

Or let Perl itself create the temporary file:

	open(my $tmp_fh, "+>", undef) or die "cannot create temp file: $!";

or something.  Or use File::Temp if you have to.


Segher

Re: [PATCH] Keep patch file permissions in mklog

[Tom de Vries]

[-- 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

Re: [PATCH] Keep patch file permissions in mklog

[Yury Gribov]

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

Re: [PATCH] Keep patch file permissions in mklog

[Tom de Vries]

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

On 11-08-14 10:18, Yury Gribov wrote:
> On 08/11/2014 11:22 AM, Tom de Vries wrote:
>> 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.
>

Updated according comments and retested.

Thanks again,
- Tom



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

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

	* mklog: Add --inline option.

diff --git a/contrib/mklog b/contrib/mklog
index 3d17dc5..b575b6e 100755
--- a/contrib/mklog
+++ b/contrib/mklog
@@ -26,6 +26,9 @@
 # Author: Diego Novillo <dnovillo@google.com> and
 #         Cary Coutant <ccoutant@google.com>
 
+use File::Temp;
+use File::Copy qw(cp mv);
+
 # Change these settings to reflect your profile.
 $username = $ENV{'USER'};
 $name = `finger $username | grep -o 'Name: .*'`;
@@ -56,14 +59,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 +284,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 "Could not 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

Re: [PATCH] Keep patch file permissions in mklog

[Segher Boessenkool]

On Mon, Aug 11, 2014 at 11:10:59AM +0200, Tom de Vries wrote:
> +use File::Temp;
> +use File::Copy qw(cp mv);

> +	# 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: $!";

That's File::Copy 2.15, not Perl 2.15 (which doesn't exist).  File::Copy 2.15
isn't so terribly old, so you might want to do a version check, e.g. as

use File::Copy 2.15 qw/cp mv/;

(but it's in Perl 5.10 already, so you might not want to bother).

> -if ($#ARGV != 0) {
> +$inline = 0;
> +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq "--inline")) {
> +	shift;
> +	$inline = 1;
> +} elsif ($#ARGV != 0) {

If you want any more command-line options, have a look at Getopt::Long.


Segher

[PATCH][PING] Keep patch file permissions in mklog

[Yury Gribov]

On 08/04/2014 12:14 PM, Tom de Vries wrote:
> On 04-08-14 08:45, Yury Gribov wrote:
>> Thanks! My 2 (actually 4) cents below.
>>
>
> Hi Yuri,
>
> thanks for the review.
>
>>  > +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq
>> "--inline")) {
>>  > +    $diff = $ARGV[1];
>>
>> Can we shift here and then just set $diff to $ARGV[0] unconditionally?
>>
>
> Done.
>
>>  > +    if ($diff eq "-") {
>>  > +            die "Reading from - and using -i are not compatible";
>>  > +    }
>>
>> Hm, can't we dump ChangeLog to stdout in that case?
>> The limitation looks rather strange.
>>
>
> My original idea here was that --inline means 'in the patch file', which
> is not possible if the patch comes from stdin.
>
> 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.
>
>>  > +    open (FILE1, '>', $tmp) or die "Could not open temp file";
>>
>> Could we use more descriptive name?
>>
>
> I've used the slightly more descriptive 'OUTPUTFILE'.
>
>>  > +    system ("cat $diff >>$tmp") == 0
>>  > +        or die "Could not append patch to temp file";
>>  > ...
>>  > +    unlink ($tmp) == 1 or die "Could not remove temp file";
>>
>> The checks look like an overkill given that we don't check for result
>> of mktemp...
>>
>
> I've added a check for the result of mktemp, and removed the unlink
> result check. I've left in the "Could not append patch to temp file"
> check because the patch file might be read-only.
>
> OK for trunk?
>
> Thanks,
> - Tom
>

Pinging the patch for Tom.

Re: [PATCH][PING] Keep patch file permissions in mklog

[Diego Novillo]

On Thu, Sep 18, 2014 at 10:56 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>
> On 08/04/2014 12:14 PM, Tom de Vries wrote:
>>
>> On 04-08-14 08:45, Yury Gribov wrote:
>>>
>>> Thanks! My 2 (actually 4) cents below.
>>>
>>
>> Hi Yuri,
>>
>> thanks for the review.
>>
>>>  > +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq
>>> "--inline")) {
>>>  > +    $diff = $ARGV[1];
>>>
>>> Can we shift here and then just set $diff to $ARGV[0] unconditionally?
>>>
>>
>> Done.
>>
>>>  > +    if ($diff eq "-") {
>>>  > +            die "Reading from - and using -i are not compatible";
>>>  > +    }
>>>
>>> Hm, can't we dump ChangeLog to stdout in that case?
>>> The limitation looks rather strange.
>>>
>>
>> My original idea here was that --inline means 'in the patch file', which
>> is not possible if the patch comes from stdin.
>>
>> 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.
>>
>>>  > +    open (FILE1, '>', $tmp) or die "Could not open temp file";
>>>
>>> Could we use more descriptive name?
>>>
>>
>> I've used the slightly more descriptive 'OUTPUTFILE'.
>>
>>>  > +    system ("cat $diff >>$tmp") == 0
>>>  > +        or die "Could not append patch to temp file";
>>>  > ...
>>>  > +    unlink ($tmp) == 1 or die "Could not remove temp file";
>>>
>>> The checks look like an overkill given that we don't check for result
>>> of mktemp...
>>>
>>
>> I've added a check for the result of mktemp, and removed the unlink
>> result check. I've left in the "Could not append patch to temp file"
>> check because the patch file might be read-only.
>>
>> OK for trunk?
>>
>> Thanks,
>> - Tom
>>
>
> Pinging the patch for Tom.


Apologies for the delay. Could someone post the latest patch. I see
it's gone through a cycle of reviews and changes.


Thanks. Diego.

Re: [PATCH][PING] Keep patch file permissions in mklog

[Tom de Vries]

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

On 18-09-14 19:46, Diego Novillo wrote:
> On Thu, Sep 18, 2014 at 10:56 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>
>> On 08/04/2014 12:14 PM, Tom de Vries wrote:
>>>
>>> On 04-08-14 08:45, Yury Gribov wrote:
>>>>
>>>> Thanks! My 2 (actually 4) cents below.
>>>>
>>>
>>> Hi Yuri,
>>>
>>> thanks for the review.
>>>
>>>>   > +if ($#ARGV == 1 && ("$ARGV[0]" eq "-i" || "$ARGV[0]" eq
>>>> "--inline")) {
>>>>   > +    $diff = $ARGV[1];
>>>>
>>>> Can we shift here and then just set $diff to $ARGV[0] unconditionally?
>>>>
>>>
>>> Done.
>>>
>>>>   > +    if ($diff eq "-") {
>>>>   > +            die "Reading from - and using -i are not compatible";
>>>>   > +    }
>>>>
>>>> Hm, can't we dump ChangeLog to stdout in that case?
>>>> The limitation looks rather strange.
>>>>
>>>
>>> My original idea here was that --inline means 'in the patch file', which
>>> is not possible if the patch comes from stdin.
>>>
>>> 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.
>>>
>>>>   > +    open (FILE1, '>', $tmp) or die "Could not open temp file";
>>>>
>>>> Could we use more descriptive name?
>>>>
>>>
>>> I've used the slightly more descriptive 'OUTPUTFILE'.
>>>
>>>>   > +    system ("cat $diff >>$tmp") == 0
>>>>   > +        or die "Could not append patch to temp file";
>>>>   > ...
>>>>   > +    unlink ($tmp) == 1 or die "Could not remove temp file";
>>>>
>>>> The checks look like an overkill given that we don't check for result
>>>> of mktemp...
>>>>
>>>
>>> I've added a check for the result of mktemp, and removed the unlink
>>> result check. I've left in the "Could not append patch to temp file"
>>> check because the patch file might be read-only.
>>>
>>> OK for trunk?
>>>
>>> Thanks,
>>> - Tom
>>>
>>
>> Pinging the patch for Tom.
>
>
> Apologies for the delay. Could someone post the latest patch. I see
> it's gone through a cycle of reviews and changes.
>
>

Hi Diego,

here it is.

I've followed up on the explanation by Segher about 2.15 File module version and 
fixed the comment.

I've not added the 2.15 file module version check on copy Segher also mentioned, 
since I'm not sure about that one. AFAIU the tradeoff is:
- without the check the mklog still works ok for older versions of perl, apart
   from the permissions (the situation we're in for all versions of perl without
   the patch)
- with the check the script won't work at all for older perl version.
So it's a question of predictability (always do the same or do nothing) vs. 
robustness (do as much as you can given the circumstances). I'm not sure which 
one is better in this case.

Thanks,
- Tom

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

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

	* mklog: Add --inline option.

diff --git a/contrib/mklog b/contrib/mklog
index 3d17dc5..1352e99 100755
--- a/contrib/mklog
+++ b/contrib/mklog
@@ -26,6 +26,9 @@
 # Author: Diego Novillo <dnovillo@google.com> and
 #         Cary Coutant <ccoutant@google.com>
 
+use File::Temp;
+use File::Copy qw(cp mv);
+
 # Change these settings to reflect your profile.
 $username = $ENV{'USER'};
 $name = `finger $username | grep -o 'Name: .*'`;
@@ -56,14 +59,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 +284,39 @@ 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 "Could not create temp file: $!";
+
+	# Copy the permissions to the temp file (in File module version 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

Re: [PATCH][PING] Keep patch file permissions in mklog

[Segher Boessenkool]

On Fri, Sep 19, 2014 at 12:41:53PM +0200, Tom de Vries wrote:
> I've followed up on the explanation by Segher about 2.15 File module 
> version and fixed the comment.
> 
> I've not added the 2.15 file module version check on copy Segher also 
> mentioned, since I'm not sure about that one. AFAIU the tradeoff is:
> - without the check the mklog still works ok for older versions of perl, 
> apart
>   from the permissions (the situation we're in for all versions of perl 
>   without
>   the patch)

Sounds good then.  The module is called File::Copy fwiw, not File.

The patch looks to me like it should work fine.


Segher

Re: [PATCH][PING] Keep patch file permissions in mklog

[Diego Novillo]

On Fri, Sep 19, 2014 at 6:41 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:

> So it's a question of predictability (always do the same or do nothing) vs.
> robustness (do as much as you can given the circumstances). I'm not sure
> which one is better in this case.

I think it's fine the way it is now. Thanks for the patch. Looks fine to me.


Diego.