diff mbox

[PING] Keep patch file permissions in mklog

Message ID 541C0871.4020407@mentor.com
State New
Headers show

Commit Message

Tom de Vries Sept. 19, 2014, 10:41 a.m. UTC 
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

Comments

Segher Boessenkool Sept. 19, 2014, 12:35 p.m. UTC | #1 
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
Diego Novillo Sept. 19, 2014, 9:47 p.m. UTC | #2 
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.
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..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