| Message ID | 53DF40F7.6050909@mentor.com |
|---|---|
| State | New |
| Headers | show |
> 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
> +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
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.
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.
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