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