Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Carlo Arenas" <carenas@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Ramsay Jones" <ramsay@ramsayjones.plus.com>
Subject: Re: [PATCH v2 0/4] builtin add -p: hopefully final readkey fixes
Date: Thu, 10 Mar 2022 13:28:05 +0000	[thread overview]
Message-ID: <93d197db-c52c-101b-bdb0-3b4c9b073705@gmail.com> (raw)
In-Reply-To: <xmqqzglyk89e.fsf@gitster.g>

Hi Junio

On 09/03/2022 23:37, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>>> These patches are based on 'pw/single-key-interactive'
>>
>> Is it still true, or does the base only apply to v1?

The base is unchanged but does not seem to match
pw/single-key-interactive. I'm not sure what happened there. They are
based on 300db53b37 ("add -p: disable stdin buffering when
interactive.singlekey is set", 2022-02-16) which is the second parent
of e53fb7aa3f ("Merge branch 'pw/single-key-interactive' into seen",
2022-02-20)

>> $ git checkout --detach pw/single-key-interactive
>> HEAD is now at ac618c418e add -p: disable stdin buffering when interactive.singlekey is set
>> $ git am -s ./+pw4-v2-add-p-single
>> Applying: terminal: use flags for save_term()
>> Applying: terminal: don't assume stdin is /dev/tty
>> Applying: terminal: work around macos poll() bug
>> error: patch failed: compat/terminal.c:397
>> error: compat/terminal.c: patch does not apply
>> Patch failed at 0003 terminal: work around macos poll() bug
>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>> When you have resolved this problem, run "git am --continue".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort".
>> $ exit
> 
> I think I figured it out.  A merge of pw/single-key-interactive into
> a recent tip of 'master' wants the "return 0" in the preimage below
> to be "break" in compat/terminal.c
> 
> 
>> @@ -397,12 +433,7 @@ int read_key_without_echo(struct strbuf *buf)
>>   		 * half a second when we know that the sequence is complete.
>>   		 */
>>   		while (!is_known_escape_sequence(buf->buf)) {
>> -			struct pollfd pfd = { .fd = 0, .events = POLLIN };
>> -
>> -			if (poll(&pfd, 1, 500) < 1)
>> -				break;
>> -
>> -			ch = getchar();
>> +			ch = getchar_with_timeout(500);
>>   			if (ch == EOF)
>>   				return 0;
>>   			strbuf_addch(buf, ch);

That looks good to me. However unfortunately there are some semantic
conflicts as well. The patch below is based on 6b1f77214c ("Merge branch
'pw/add-p-single-key' into seen", 2022-03-09), hopefully Thunderbird wont
mangle it. Whilst preparing the fixup I realized I need to reroll to
fix a closing stdin in patch 2 and resetting the job signals on error in
patch 4. What's the best base to use when rerolling?

Best Wishes

Phillip

---- >8 ----

From: Phillip Wood <phillip.wood@dunelm.org.uk>
Date: Thu, 10 Mar 2022 11:05:26 +0000
Subject: [PATCH] fixup! Merge branch 'pw/add-p-single-key' into seen

---
  compat/terminal.c | 3 ++-
  editor.c          | 2 +-
  2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 3172f4f408..d1ed5c07dc 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -141,7 +141,8 @@ int save_term(enum save_term_flags flags)
         if (term_fd < 0)
                 return -1;
         if ((flags & SAVE_TERM_DUPLEX) && !is_controlling_terminal(term_fd)) {
-               close(term_fd);
+               if (term_fd) /* avoid closing stdin */
+                       close(term_fd);
                 term_fd = -1;
                 return -1;
         }
diff --git a/editor.c b/editor.c
index 6c5c95e6a2..192d6ea75d 100644
--- a/editor.c
+++ b/editor.c
@@ -55,7 +55,7 @@ static int prepare_term(const char *editor)
  
         git_config_get_bool("editor.stty", &need_saverestore);
         if (need_saverestore)
-               return save_term(1);
+               return save_term(SAVE_TERM_DUPLEX);
         return 0;
  }
  
-- 
2.33.0.342.g580a6b0edd
