On Thu, Sep 18, 2014 at 10:56 AM, Yury Gribov <y.gri...@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.
