preparing for 2.34.1

preparing for 2.34.1

[Junio C Hamano]

There are a few topics [*] to fix regressions introduced in the
previous cycle, which should be in 2.34.1 and I've merged them to
'master'. I hope we can merge them to 'maint' (which now point at
2.34) and tag 2.34.1 in a few days.

After that, in yet another few days, we will see most of the stalled
topics ejected from the tree, the tip of 'next' rewound, and we will
start the cycle toward 2.35 by starting to take new topics, by the
beginning of the next week at the latest.

Thanks.



* ab/update-submitting-patches (2021-11-13) 1 commit
  (merged to 'next' on 2021-11-19 at b44f4d0eb0)
 + SubmittingPatches: fix Asciidoc syntax in "GitHub CI" section

 Doc fix.


* ev/pull-already-up-to-date-is-noop (2021-11-18) 1 commit
  (merged to 'next' on 2021-11-19 at 2d8f0cd000)
 + pull: should be noop when already-up-to-date

 "git pull" with any strategy when the other side is behind us
 should succeed as it is a no-op, but doesn't.


* hm/paint-hits-in-log-grep (2021-11-19) 1 commit
  (merged to 'next' on 2021-11-19 at e146d25c7c)
 + Revert "grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data"

 "git grep" looking in a blob that has non-UTF8 payload was
 completely broken when linked with certain versions of PCREv2
 library in the latest release.

Re: preparing for 2.34.1

[Carlo Arenas]

On Mon, Nov 22, 2021 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> There are a few topics [*] to fix regressions introduced in the
> previous cycle

There was another regression report[1] that I just fished out of my
spam folder and that I introduced in 3d411afabc (editor: save and
reset terminal after calling EDITOR, 2021-10-05), but that I have not
yet produced a fix for or fully understood.

The gist is that some people seem to be using a hack[2] dscho posted a
few years ago to get info from git programmatically and I just didn't
expect someone would try to invoke the EDITOR unless they had a
terminal, so the fix might be to just add an isatty(0) call, but
reverting that commit would be also an option.

Carlo

[1] https://lore.kernel.org/git/ee302c98-da27-da43-e684-c7ec8b225360@gmx.net/
[2] https://yhbt.net/lore/all/nycvar.QRO.7.76.6.1903221436590.41@tvgsbejvaqbjf.bet/T/

Re: preparing for 2.34.1

[Johannes Schindelin]

Hi Carlo,

On Mon, 22 Nov 2021, Carlo Arenas wrote:

> On Mon, Nov 22, 2021 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > There are a few topics [*] to fix regressions introduced in the
> > previous cycle
>
> There was another regression report[1] that I just fished out of my
> spam folder and that I introduced in 3d411afabc (editor: save and
> reset terminal after calling EDITOR, 2021-10-05), but that I have not
> yet produced a fix for or fully understood.
>
> The gist is that some people seem to be using a hack[2] dscho posted a

You mean

https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903221436590.41@tvgsbejvaqbjf.bet/

right? I.e. figuring out what Git considers its system config location via

	GIT_EDITOR=echo git config --system -e 2>/dev/null

> few years ago to get info from git programmatically and I just didn't
> expect someone would try to invoke the EDITOR unless they had a
> terminal, so the fix might be to just add an isatty(0) call, but
> reverting that commit would be also an option.

The quickest workaround for this is probably to special-case the editor
`echo`:

-- snip --
diff --git a/editor.c b/editor.c
index 674309eed8bd..1b97f7da9920 100644
--- a/editor.c
+++ b/editor.c
@@ -51,12 +51,11 @@ const char *git_sequence_editor(void)
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
-	int term_fail;
-
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");

 	if (strcmp(editor, ":")) {
+		int save_and_restore_term = strcmp(editor, "echo");
 		struct strbuf realpath = STRBUF_INIT;
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
@@ -86,9 +85,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
-		term_fail = save_term(1);
+		if (save_and_restore_term)
+			save_and_restore_term = !save_term(1);
 		if (start_command(&p) < 0) {
-			if (!term_fail)
+			if (save_and_restore_term)
 				restore_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
@@ -97,7 +97,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
-		if (!term_fail)
+		if (!save_and_restore_term)
 			restore_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
-- snap --

However, I could imagine that other scenarios call for an editor that
_also_ does not run in the terminal, and where also no real terminal is
available for saving and restoring.

I was tempted to suggest an `isatty(2)`, but that probably comes with its
own problems, too.

Ciao,
Dscho

>
> Carlo
>
> [1] https://lore.kernel.org/git/ee302c98-da27-da43-e684-c7ec8b225360@gmx.net/
> [2] https://yhbt.net/lore/all/nycvar.QRO.7.76.6.1903221436590.41@tvgsbejvaqbjf.bet/T/
>
>

Re: preparing for 2.34.1

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The quickest workaround for this is probably to special-case the editor
> `echo`:

"GIT_EDITOR=: git cmd" would also be a common trick people would use
to bypass editor and take whatever is given as an initial template.

> However, I could imagine that other scenarios call for an editor that
> _also_ does not run in the terminal, and where also no real terminal is
> available for saving and restoring.
>
> I was tempted to suggest an `isatty(2)`, but that probably comes with its
> own problems, too.

I think isatty(2) is pretty much our synonym to "are we talking to
an end-user sitting in front of the terminal".  Mostly we use it as
a way to control the progress bars, and use of editor on terminal
would be in line with these existing uses.

Re: preparing for 2.34.1

[Johannes Schindelin]

Hi Junio,

On Mon, 22 Nov 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The quickest workaround for this is probably to special-case the editor
> > `echo`:
>
> "GIT_EDITOR=: git cmd" would also be a common trick people would use
> to bypass editor and take whatever is given as an initial template.

GIT_EDITOR=: is not a problem because of
https://github.com/git/git/blob/v2.34.0/editor.c#L59:

	if (strcmp(editor, ":")) {
		[...]
		term_fail = save_term(1);
		if (start_command(&p) < 0) {
			if (!term_fail)
				restore_term();
			[...]
		}

		[...]
		if (!term_fail)
			restore_term();
		[...]
	}

> > However, I could imagine that other scenarios call for an editor that
> > _also_ does not run in the terminal, and where also no real terminal is
> > available for saving and restoring.
> >
> > I was tempted to suggest an `isatty(2)`, but that probably comes with its
> > own problems, too.
>
> I think isatty(2) is pretty much our synonym to "are we talking to
> an end-user sitting in front of the terminal".  Mostly we use it as
> a way to control the progress bars, and use of editor on terminal
> would be in line with these existing uses.

Indeed, I think that isatty(2) is a better indicator than isatty(1). We
sometimes _do_ redirect the output of, say, `git commit`, to capture the
commit hash that was generated. We typically do not redirect stderr,
though, unless calling from an application and capturing everything via
pipes. So isatty(2) strikes me as the best balance we can strike here.

Ciao,
Dscho

RE: preparing for 2.34.1

[rsbecker]

On November 22, 2021 6:30 PM, Johannes Schindelin wrote:
> On Mon, 22 Nov 2021, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > > The quickest workaround for this is probably to special-case the
> > > editor
> > > `echo`:
> >
> > "GIT_EDITOR=: git cmd" would also be a common trick people would use
> > to bypass editor and take whatever is given as an initial template.
> 
> GIT_EDITOR=: is not a problem because of
> https://github.com/git/git/blob/v2.34.0/editor.c#L59:
> 
> 	if (strcmp(editor, ":")) {
> 		[...]
> 		term_fail = save_term(1);
> 		if (start_command(&p) < 0) {
> 			if (!term_fail)
> 				restore_term();
> 			[...]
> 		}
> 
> 		[...]
> 		if (!term_fail)
> 			restore_term();
> 		[...]
> 	}
> 
> > > However, I could imagine that other scenarios call for an editor
> > > that _also_ does not run in the terminal, and where also no real
> > > terminal is available for saving and restoring.
> > >
> > > I was tempted to suggest an `isatty(2)`, but that probably comes
> > > with its own problems, too.
> >
> > I think isatty(2) is pretty much our synonym to "are we talking to an
> > end-user sitting in front of the terminal".  Mostly we use it as a way
> > to control the progress bars, and use of editor on terminal would be
> > in line with these existing uses.
> 
> Indeed, I think that isatty(2) is a better indicator than isatty(1). We
> sometimes _do_ redirect the output of, say, `git commit`, to capture the
> commit hash that was generated. We typically do not redirect stderr,
though,
> unless calling from an application and capturing everything via pipes. So
> isatty(2) strikes me as the best balance we can strike here.

Please be careful of this one. isatty(2) may not be the issue, but the
filedes provided by Jenkins and other CI/CD systems over SSH often
mischaracterises the results from this call. Hacking the file descriptor
(2>&1 etc) prior to going to git is a common thing in scripts. Stdin is
frequently wrong when Jenkins sets up the environment to prompt for an SSH
passphrase - happens in non-Docker nohup situations - where git will end up
thinking it is interactive when it is not - not on NonStop fortunately, but
this happens with a Gentoo Hypervisor and Ubuntu VM. Also, stderr gets
redirected in scripts frequently in my experience - particularly on exotic
operating systems, crossing over from say, legacy NonStop or IBM TSO to
each's POSIX environment. I would expect breakages from this assumption, so
please be cautious.

-Randall

Re: preparing for 2.34.1

[Carlo Arenas]

On Mon, Nov 22, 2021 at 2:28 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> I was tempted to suggest an `isatty(2)`, but that probably comes with its
> own problems, too.

I went with isatty(1) in the suggested minimal bugfix[1] since I think
it is safe to assume that no one that doesn't have stdout connected to
a terminal should even care if the invoking editor dies and messes
with their terminal, and hopefully whoever is hacking git commands
that invoke an editor to read its output, is actually not doing it
through a tty but a pipe.

What problems do you foresee with isatty?, the current code already
checks for errors on accessing the console, but as Phillip
suggested[2] in that thread a signal might be generated for some of
those errors when the terminal is shared with other processes.

Carlo

CC: +Phillip

[1] https://lore.kernel.org/git/20211122222850.674-1-carenas@gmail.com/
[2] https://lore.kernel.org/git/04ab7301-ea34-476c-eae4-4044fef74b91@gmail.com/T/#u

Re: preparing for 2.34.1

[Derrick Stolee]

On 11/22/21 12:37 PM, Junio C Hamano wrote:
> There are a few topics [*] to fix regressions introduced in the
> previous cycle, which should be in 2.34.1 and I've merged them to
> 'master'. I hope we can merge them to 'maint' (which now point at
> 2.34) and tag 2.34.1 in a few days.
> 
> After that, in yet another few days, we will see most of the stalled
> topics ejected from the tree, the tip of 'next' rewound, and we will
> start the cycle toward 2.35 by starting to take new topics, by the
> beginning of the next week at the latest.

There is a patch deep in a thread [1] that is a regression
in 2.34.0 that would be good to include in this release. It
fixes a sparse-checkout bug via a simple revert (plus a new
test).

Sorry that the patch was buried deep and was easy to miss.

[1] https://lore.kernel.org/git/72fffbff-16f7-fa17-b212-67aae9e1b034@gmail.com/

Thanks,
-Stolee

Re: preparing for 2.34.1

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 11/22/21 12:37 PM, Junio C Hamano wrote:
>> There are a few topics [*] to fix regressions introduced in the
>> previous cycle, which should be in 2.34.1 and I've merged them to
>> 'master'. I hope we can merge them to 'maint' (which now point at
>> 2.34) and tag 2.34.1 in a few days.
>> 
>> After that, in yet another few days, we will see most of the stalled
>> topics ejected from the tree, the tip of 'next' rewound, and we will
>> start the cycle toward 2.35 by starting to take new topics, by the
>> beginning of the next week at the latest.
>
> There is a patch deep in a thread [1] that is a regression
> in 2.34.0 that would be good to include in this release. It
> fixes a sparse-checkout bug via a simple revert (plus a new
> test).
>
> Sorry that the patch was buried deep and was easy to miss.
>
> [1] https://lore.kernel.org/git/72fffbff-16f7-fa17-b212-67aae9e1b034@gmail.com/

Instead of these notes, it would be much easier for people to
realize that this (and previous) week is mostly about regression
fixing and it is not welcome to drown the list with new patches, if
you just resent the patch with 

    Subject: [PATCH/regression] fix blah

I would say.

Having said that, thanks for a reminder.  I did read it, but
apparently forgot to include it in the list of things needed for
'master' when I prepared it last night.

preparing for 2.34.1 (2nd round)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

There are a few topics [*] to fix regressions introduced in the
previous cycle, which should be in 2.34.1 and I've merged them to
'master'. I hope we can merge them to 'maint' (which now point at
2.34) and tag 2.34.1 in a day or two.

After that, in yet another few days, we will see most of the stalled
topics ejected from the tree, the tip of 'next' rewound, and we will
start the cycle toward 2.35 by starting to take new topics,
hopefully by the beginning of the next week.


Thanks.


Fixes since v2.34
-----------------

 * "git grep" looking in a blob that has non-UTF8 payload was
   completely broken when linked with certain versions of PCREv2
   library in the latest release.

 * "git pull" with any strategy when the other side is behind us
   should succeed as it is a no-op, but doesn't.
   (merge ea1954af ev/pull-already-up-to-date-is-noop later to maint).

 * An earlier change in 2.34.0 caused JGit application (that abused
   GIT_EDITOR mechanism when invoking "git config") to get stuck with
   a SIGTTOU signal; it has been reverted.
   (merge e3f7e01b50 jc/save-restore-terminal-revert later to maint).

 * An earlier change that broke .gitignore matching has been reverted.
   (merge 33c5d6c845 ds/add-rm-with-sparse-index later to maint).

 * SubmittingPatches document gained a syntactically incorrect mark-up,
   which has been corrected.
   (merge edbd9f37 ab/update-submitting-patches later to maint).