Re: [PATCH v3 1/5] git-p4: rewrite prompt to be Windows compatible

From: Ben Keene <seraphire@gmail.com>
Date: Feb 10, 2020, 15:49 UTC
Message-ID: <2e0d4859-a2d7-9882-4781-7ae67ec608f5@gmail.com>
In-Reply-To: <xmqqwo8zecro.fsf@gitster-ct.c.googlers.com>

On 2/6/2020 2:28 PM, Junio C Hamano wrote:

Show 18 quoted lines

> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> The existing function prompt(prompt_text) does not work correctly when
>> run on Windows 10 bash terminal when launched from the sourcetree
>> GUI application. The stdout is not flushed properly so the prompt text
>> is not displayed to the user until the next flush of stdout, which is
>> quite confusing.
> Is that the bug in raw_input(prompt_text) used in the source, or is
> that the bug in your environment (whatever "the sourcetree GUI
> application" is)?  I cannot quite tell if this is butchering code
> that is perfectly working well for other people just to cope with a
> broken invoker that is what really needs fixing, or if it is working
> around a bug in raw_input().  If the former, the change is not what
> we want, and if the latter, the change should go to Python upstream,
> so either way, I am not sure if we want this patch without further
> information.

Sourcetree is a graphical UI for GIT maintained by Atlassian. It invokes the git-bash.exe in the git for windows installation as the shell interface. In this context, the raw_input() function failed to work for me. I do not know if I am unique in using git-p4.py on a windows platform, but from what I could find online, raw_input() does not flush stdout. (See https://bugs.python.org/issue526382) From what I saw in response to this problem, everyone suggested reading from stdin instead of using raw_input. Though I can understand the hesitation for this sort of change.

Show 36 quoted lines

>
> Anybody on a similar platform have opinion on this?  I am OK as long
> as this change does *not* break the program in an environment that
> it is working fine, but that is not even clear.
>
>> Change this method by:
>> * Adding flush to stderr, stdout, and stdin
>> * Use readline from sys.stdin instead of raw_input.
>>
>> The existing strip().lower() are retained.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> ---
>>   git-p4.py | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 40d9e7c594..7d8a5ee788 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -175,7 +175,11 @@ def prompt(prompt_text):
>>       """
>>       choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
>>       while True:
>> -        response = raw_input(prompt_text).strip().lower()
>> +        sys.stderr.flush()
>> +        sys.stdout.write(prompt_text)
>> +        sys.stdout.flush()
> raw_input() is getting replace with input() in another series to
> bring us to Python3 compatible world, but because you are getting
> rid of its use, as long as the resulting code works with both
> Python2 and Python3, we are happy ;-)
>
>> +        sys.stdin.flush()
> What does it even mean to flush the input stream here?  At least in
> C, it is meaningless and causes an undefined behaviour.

The intent, was to clear out the input buffer before reading the input, however, in a non-interactive mode, this would be undesirable.

I will remove the stdin.flush().

Show 6 quoted lines

>
>> +        response=sys.stdin.readline().strip().lower()
>>           if not response:
>>               continue
>>           response = response[0]
> Thanks.