[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