[PATCH 1 of 3 V2] convert: Add support to common commandline to access stdin of the process
Daniel Atallah
daniel.atallah at gmail.com
Thu Mar 24 15:49:04 CDT 2011
On Thu, Mar 24, 2011 at 13:56, Patrick Mézard <pmezard at gmail.com> wrote:
> Le 24/03/11 16:38, Daniel Atallah a écrit :
>> On Thu, Mar 24, 2011 at 11:09, Mads Kiilerich <mads at kiilerich.com> wrote:
>>> On 03/24/2011 03:48 PM, daniel.atallah at gmail.com wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Daniel Atallah<daniel.atallah at gmail.com>
>>>> # Date 1300906529 14400
>>>> # Branch stable
>>>> # Node ID 0fb617760b931cd25537e3362af52b2cfbd1a329
>>>> # Parent 913c2c66a555934cab9bcdac3412703256f9cdf2
>>>> convert: Add support to common commandline to access stdin of the process
>>>>
>>>> diff --git a/hgext/convert/common.py b/hgext/convert/common.py
>>>> --- a/hgext/convert/common.py
>>>> +++ b/hgext/convert/common.py
>>>> @@ -233,6 +233,7 @@
>>>> def __init__(self, ui, command):
>>>> self.ui = ui
>>>> self.command = command
>>>> + self.redirectstdin = True
>>>
>>> Is it really a good idea to have this as a state variable? Shouldn't it just
>>> be a parameter to _dorun and _cmdline?
>>
>> I actually originally implemented it that way, but reverted to this
>> state variable for a couple reasons:
>>
>> I didn't want to change the _cmdline() function signature (this itself
>> may not be important because it isn't used anywhere outside
>> common.py).
>>
>> More importantly, _cmdline() is used by limit_arglist() (and
>> subsequently xargs(), which is used externally)
>>
>> I can certainly make the changes and change the xargs() signature too
>> if that is desirable.
>
> We might want to pass redirectstdin to xargs() and others in the future but that's not necessary right now. In our case, using it is pretty clear: if you call _run(), there is no stdin to handle, and if you _run2() and do not use the returned stdin, then why do you call _run2() in the first place? So, what about something in-between?
>
> diff -r 4781291f9803 hgext/convert/common.py
> --- a/hgext/convert/common.py Thu Mar 24 10:33:18 2011 -0400
> +++ b/hgext/convert/common.py Thu Mar 24 18:53:28 2011 +0100
> @@ -247,7 +247,6 @@
> def __init__(self, ui, command):
> self.ui = ui
> self.command = command
> - self.redirectstdin = True
>
> def prerun(self):
> pass
> @@ -255,7 +254,7 @@
> def postrun(self):
> pass
>
> - def _cmdline(self, cmd, *args, **kwargs):
> + def _cmdline(self, cmd, closestdin, *args, **kwargs):
> cmdline = [self.command, cmd] + list(args)
> for k, v in kwargs.iteritems():
> if len(k) == 1:
> @@ -272,19 +271,19 @@
> cmdline = [util.shellquote(arg) for arg in cmdline]
> if not self.ui.debugflag:
> cmdline += ['2>', util.nulldev]
> - if self.redirectstdin:
> + if closestdin:
> cmdline += ['<', util.nulldev]
> cmdline = ' '.join(cmdline)
> return cmdline
>
> def _run(self, cmd, *args, **kwargs):
> - return self._dorun(util.popen, cmd, *args, **kwargs)
> + return self._dorun(util.popen, cmd, True, *args, **kwargs)
>
> def _run2(self, cmd, *args, **kwargs):
> - return self._dorun(util.popen2, cmd, *args, **kwargs)
> + return self._dorun(util.popen2, cmd, False, *args, **kwargs)
>
> - def _dorun(self, openfunc, cmd, *args, **kwargs):
> - cmdline = self._cmdline(cmd, *args, **kwargs)
> + def _dorun(self, openfunc, cmd, closestdin, *args, **kwargs):
> + cmdline = self._cmdline(cmd, closestdin, *args, **kwargs)
> self.ui.debug('running: %s\n' % (cmdline,))
> self.prerun()
> try:
> @@ -345,7 +344,8 @@
> return self._argmax
>
> def limit_arglist(self, arglist, cmd, *args, **kwargs):
> - limit = self.getargmax() - len(self._cmdline(cmd, *args, **kwargs))
> + cmdlen = len(self._cmdline(cmd, True, *args, **kwargs))
> + limit = self.getargmax() - cmdlen
> bytes = 0
> fl = []
> for fn in arglist:
>
> I haven't tested the patch but it gives you the idea.
>
> I am adding one last remark to the second patch and I think the third repost will be the good.
This seems reasonable to me.
I'll do the tweaks to this and the other patch and submit a V3 soon.
Thanks,
-D
More information about the Mercurial-devel
mailing list