From: Tom de Vries <Tom_deVries@mentor.com>
To: Yury Gribov <y.gribov@samsung.com>
Cc: Diego Novillo <dnovillo@google.com>,
Geoff Keating <geoffk@geoffk.org>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Trevor Saunders <tsaunders@mozilla.com>
Subject: Re: [PATCH] Keep patch file permissions in mklog
Date: Mon, 04 Aug 2014 08:15:00 -0000 [thread overview]
Message-ID: <53DF40F7.6050909@mentor.com> (raw)
In-Reply-To: <53DF2BF4.8090806@samsung.com>
[-- 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
next prev parent reply other threads:[~2014-08-04 8:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-31 7:47 Tom de Vries
2014-08-01 4:21 ` Yury Gribov
2014-08-01 6:52 ` Tom de Vries
2014-08-01 7:18 ` Yury Gribov
2014-08-02 19:23 ` Tom de Vries
2014-08-04 6:45 ` Yury Gribov
2014-08-04 8:15 ` Tom de Vries [this message]
2014-08-04 11:50 ` Yury Gribov
2014-08-04 13:37 ` Segher Boessenkool
2014-08-11 7:23 ` Tom de Vries
2014-08-11 8:18 ` Yury Gribov
2014-08-11 9:11 ` Tom de Vries
2014-08-11 17:29 ` Segher Boessenkool
2014-09-18 14:56 ` [PATCH][PING] " Yury Gribov
2014-09-18 17:46 ` Diego Novillo
2014-09-19 10:42 ` Tom de Vries
2014-09-19 12:35 ` Segher Boessenkool
2014-09-19 21:47 ` Diego Novillo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53DF40F7.6050909@mentor.com \
--to=tom_devries@mentor.com \
--cc=dnovillo@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=geoffk@geoffk.org \
--cc=tsaunders@mozilla.com \
--cc=y.gribov@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).