patchconnect: handle putty/plink also in GIT_SSH_COMMAND

From: Segev Finer <segev208@gmail.com>

Git for Windows has special support for the popular SSH client PuTTY: when using PuTTY's non-interactive version ("plink.exe"), we use the -P option to specify the port rather than OpenSSH's -p option. TortoiseGit ships with its own, forked version of plink.exe, that adds support for the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the user overrides the SSH command via the environment variable GIT_SSH (which allows specifying the name of the executable), not GIT_SSH_COMMAND (which allows specifying a full command, including additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink, such as setting a private key, they are required to either use a shell script named plink or tortoiseplink or duplicate the logic that is already in Git for passing the correct style of command line arguments, which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v1
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v1

	Original Pull Request:
	https://github.com/git-for-windows/git/pull/1006

 connect.c        | 23 ++++++++++++++++-------
 t/t5601-clone.sh | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

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

I suspect that this will break when GIT_SSH_COMMAND, which is meant to be processed by the shell, hence the user can write anything, begins with a one-shot environment variable assignment, e.g.

	[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink

One possible unintended side effect of this patch is when VAL1 ends with /plink (or /tortoiseplink) and the command is not either of these, in which case the "tortoiseplink" and "putty" variables will tweak the command line for an SSH implementation that does not want such a tweak to be made. There may be other unintended fallouts.

Sorry, no concrete suggestion to get this to work comes to my mind offhand.

Perhaps give an explicit way to force "tortoiseplink" and "putty" variables without looking at and guessing from the pathname, so that the solution does not have to split and peek the command line?

 connect.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

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

Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND) interface takes any shell-interpretable commands, the first "token" you see is not limited to a one-shot environment assignment. It could even be "cmd1 && cmd2 && ..." or even:

	if test -f ${TPLINK:=//path/to/tortoiseplink}
	then
		exec "$TPLINK" "$@"
	elif test -f ${PLINK:=//path/to/plink}
		exec "$PLINK" "$@"
	else
		echo >&2 no usable ssh on this host
	fi

Worse, the above may be in "myssh" script found on user's PATH and then core.sshcommand may be set to

	TPLINK=//my/path/to/tortoiseplink PLINK=//my/path/to/plink myssh

in the configuration the user uses on multiple hosts, some have tortoiseplink installed and some do not. The idea the user who set it up may be to use whatever available depending on the host.

The posted patch would be confused that we are using tortoiseplink but the "myssh" script would correctly notice that on a particular host it is unavailable and choose to use plink instead.

A user with such an elaborate set-up with multiple hosts with different configurations would likely to want to say "Just give me a regular SSH command line, and in my 'myssh' script I'll futz with -p/-P differences and such, as I'm writing a small script to cope with Tortoiseplink and Puttyplink anyway". With a separate, explicit configuration variable to tell Git what variant of SSH you have, even such a user can be served (she would just set ssh.variant to "vanilla" or whatever that is not "tortoiseplink" or "plink").

.

Hi Junio,

On Sun, 8 Jan 2017, Junio C Hamano wrote:

Sure, it could be all of that. It could even blast through the environment limits in some environments on some platforms, but not on others. It could automatically transfer bitcoins whenever a connection to github.com is attempted. It could start the camera and build a time-lapse of "every time I pushed or fetched a repository", as an art project. It could shut down the computer.

It could do all of that.

In practice, however, what users realistically do is to use GIT_SSH_COMMAND whenever they need to override the ssh command with options.

This is the common case, and that is what we must make less cumbersome to use.

If you feel strongly about your contrived examples possibly being affected by this patch, we could easily make this conditional on

1) no '&&' or '||' being found on the command-line, and
2) argv[0] not containing an '='

Another approach would be to verify that argv[i] starts with '-' for non-zero i.

But do we really need to do that?

Let's have a very brief look at the amount of work required to come up with a false positive (I am not so much concerned about any power user writing elaborate shell expressions that may call plink.exe: those power users will simply have to continue to use their own -P => -p and -batch hacks):

The logic kicks in if the split command-line's first component has a basename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The only way this logic can kick in by mistake is if the first component is *not* the command to call *and* if the first component *still* has a basename of `plink` or `tortoiseplink`.

That means that the user has to specify something like

	HAHAHA_IT_IS_NOT=/plink.exe ssh

as GIT_SSH_COMMAND.

Now, let's take a *very* brief look at the question how likely it is to have a basename of `plink` or `tortoiseplink`. Remember, there are two ways that the basename can be a specific term: either the original string is already equal to that term, or it ends in a slash followed by the term. That is, either the first component refers to plink.exe/tortoiseplink.exe, i.e. it would be a *true* positive. Or the first component would end in "/plink" or "/tortoiseplink" (possibly with the `.exe` suffix) *and not be a command*. But how likely is it to specify a non-command (such as a per-process variable assignment) that specifically mentions plink or tortoiseplink?

Is it even realistic to expect users to specify values for GIT_SSH_COMMAND that contain "plink" as a substring when they do *not* want to call plink at all?

After thinking a bit longer than I had originally planned about it, my answer is: no. No, I do not think it is realistic. I fully expect *no* user to have a GIT_SSH_COMMAND containing even a substring "plink" *anywhere*, unless they do, in fact, want to call plink or tortoiseplink.

My main aim with Git for Windows is to improve the user experience, and the patch in question does do that. Of course, I also try to avoid breaking existing setups while improving the user experience, and I believe that it is safe to assume that the patch in question in all likelihood does not break any existing setup.

Ciao, Dscho

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

No. An explicit way to override an incorrect guess is sufficient and necessary. The above two-item list you gave will be just part of the machinery to make an incorrect guess. The auto-detection in the posted patch should cover many users' use case and I do not think it needs to be extended further to make it more brittle, as by definition its guess cannot be perfect. Just keep it simple and give a separate escape hatch.

My second message was to clarify that "VAR1=VAL2 command" is NOT a contrived example, and this response indicates that I somehow failed to convey that to you. The "if tortoiseplink exists (and the end user can override the location with an environment), use that, and if PuTTY plink exists (ditto), use that instead" in a "myssh" script, and use it as core.sshcommand with the environment to override my custom installation location to these two programs, would be what I would do when I get two Windows machines, with these variants of SSH on each. So take the second message as a bug report against the version of Git for Windows you ship with the patch in question.

The auto-detection may work for many people and that is a great thing. I failed to say that in my message, as I thought that was obvious. But it is important to plan to cope with the case where it does not work. The usual practice around here is to say "the it may not necessarily work for everybody, so lets be prepared to add an explicit override if it turns out to be necessary".

The second message, which you are responding to, was meant to be a bug report from the future, telling us that an override is needed, showing that we do not have to wait for a bug report to act on.

Hi Junio,

On Mon, 9 Jan 2017, Junio C Hamano wrote:

Exactly.

Indeed. The quite contrived example was about a script that chooses between plink and tortoiseplink (and fails to call anything else). And it failed to convince me.

But since you seem to convinced that a future bug report like this may happen (I am not, and it contradicts my conviction that one should cross a bridge only when reaching it, but whatever), how about this, on top:

-- snipsnap --

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

I do not understand why you keep arguing.

With No-Exactly exchange, I thought that we established that

 * The original auto-detection for GIT_SSH is battle tested and has
   worked for us very well.  It may probably have covered 99.9% of
   the population, and we haven't heard complaints from the
   remaining 0.1%.

 * The added support for GIT_SSH_COMMAND, because the mechanism
   gives more lattitude to end-users to be creative, will have lower
   chance of correctly deciding between ssh, tortoiseplink and
   plink, but it would still be high enough, say 99%, correct
   detection rate.

 * It is foolish to add more code to refine the auto-detection to
   raise 99% to 99.5%.  We know that the approach fundamentally
   cannot make the detection rate reach 100%.

The last one can be paraphrased as "perfect is the enemy of good".

But that does not mean that it is OK to leave the system unusable for 1% of the users. If the auto-detection code does not work correctly for a tiny minority of users, it is still OK, as long as we give them an escape hatch to allow them to say "You may not be able to guess among ssh, tortoiseplink and plink, or you may even guess incorrectly, so I'll tell you. I am using plink". 99% of users do not have to know about that escape hatch, but 1% of users will be helped.

IOW, "give an escape hatch to override auto-detected tortoiseplink and plink variables" suggestion should be taken as an "in addition to" suggestion (as opposed to an "instead of" suggestion). I was not clear in my very first message, and I thought in my second and my third message I clarified it as such, but probably I wasn't explicit enough.

In any case, "do this only if the first one token on the command line does not have '='" we see below is flawed for two reasons and I think it would not be a workable counter-proposal (besides, it goes against the "perfect is the enemy of good" mantra).

For one thing, the command line may not be "VAR=VAL cmd", but just "cmd" that names the path to tortoiseplink, but the leading directory path that houses tortoiseplink may have a '=' in it, in which case we would detect it correctly if we didn't have such a hack, but with the hack we would punt. More importantly, even when "VAR=VAL cmd" form was caught with the change, it may punt and stop guessing, but punting alone does not help users by letting them say "I know you cannot auto-detect, so let me help you---what I have is PuTTY plink". If it always assumes that the user uses the plain vanilla ssh, then the change merely changed what incorrect guess the auto-detection makes from "tortoiseplink" to "vanilla ssh" for a user who uses the PuTTY variant of plink.

Hi Junio,

On Mon, 9 Jan 2017, Junio C Hamano wrote:

While you may not have been explicit enough, I was not smart enough.

That escape-hatch exists *already*. And it is exactly the thing that you mentioned earlier as a potential source of mis-identification.

Let's assume that you want to use a script for the GIT_SSH_COMMAND that eventually uses PuTTY, and you call this script "junio-is-a-superstar.sh". All plausible so far ;-)

Now, with the patch in question (without the follow-up, which I would like to ask you to ignore, just like you did so far), Git would not figure out that your script calls PuTTY eventually. The work-around? Easy:

	DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh

In other words: the thing that we thought may be a problem is actually a feature.

Likewise, if your GIT_SSH_COMMAND looks like this:

	THIS_IS_NOT_ACTUALLY_PUTTY=/my/window/needs/putty my-ssh.sh

... you can easily change Git's mind by prefixing

	DUMMY=blubber

If you want to use a script that decides itself whether to call OpenSSH or PuTTY, Git should "stay dumb" about it, as it will not be able to tell beforehand whether a port argument should be passed via `-p` or `-P` anyway.

Of course, given our discussion I think all of this should be documented in the commit message as well as in the man page.

Do you agree this is a good direction to take?

Ciao, Johannes

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

Think about how you would explain that to an end-user in our document? You'll need to explain how exactly the auto-detection works, so that the user can "exploit" the loophole to do that. And what maintenance burden does it add when auto-detection is updated?

I think I know you well enough that you know well enough that it is too ugly to live, and I suspect that the above is a tongue-in-cheek "arguing for the sake of argument" and would not need a serious response, but just in case...

Yes. Here is what comes on an obvious clean-up patch (which will be sent as a follow-up to this message).

-- >8 --
Subject: [PATCH] connect: core.sshvariant to correct misidentification

While the logic we have been using to guess which variant of SSH implementation is in use by looking at the name of the program has been robust enough for GIT_SSH (which does not go through the shell, hence it can only spell the name the SSH program and nothing else), extending it to GIT_SSH_COMMAND and core.sshcommand opens it for larger chance of misidentification, because these can specify arbitrary shell command, and a simple "the first word on the line must be the command name" heuristic may not even look at the command name at all.

As any effort to improve the heuristic will give us only diminishing returns, and especially given that most of the users are expected to have a command line for which the heuristic would work correctly, give an explicit escape hatch to override a misidentification, just in case one happens.

It is arguably bad to add this new variable to the core.* set, in the sense that you'll never see this variable if you do not interact with other repositories over ssh, but core.sshcommand is already there for some reason, so let's match it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 13 +++++++++++++
 connect.c                | 26 ++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

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

... and this is the "obvious clean-up patch" to apply directly on top of Segev's GIT_SSH_COMMAND support, which comes before the previous one.

-- >8 --
Subject: [PATCH] connect: rename tortoiseplink and putty variables

One of these two may have originally been named after "what exact SSH implementation do we have" so that we can tweak the command line options, but these days "putty=1" no longer means "We are using the plink SSH implementation that comes with PuTTY". It is set when we guess that either PuTTY plink or Tortoiseplink is in use.

Rename them after what effect is desired. The current "putty" option is about using "-P <port>" when OpenSSH would use "-p <port>", so rename it to port_option whose value is either 'p' or 'P". The other one is about passing an extra command line option "-batch", so rename it needs_batch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

Apart from an overly-long line, this patch looks good. It made my life a little harder, as I had to rebase Segev's ssh.variant (why this should be a core.* variable is not clear to me) patch and it caused conflicts. I resolved those conflicts and made both patches part of this patch series.

Will contribute v2 as soon as the test suite passes, Johannes

On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:

I have been watching this discussion from the sidelines, and I agree that this direction is a big improvement.

IIRC, the "const" in git_config_get_string_const is only about avoiding an annoying cast. The result is still allocated and needs freed. Since you are not keeping the value after the function returns, I think you could just use git_config_get_value().

(Grepping around, I see a few other places that seem to make the same mistake. I think this is a confusing interface that should probably be fixed).

-Peff

Jeff King <peff@peff.net> writes:

It is too late for today's pushout (I have this topic near the tip of 'pu', so it is possible to tweak and redo only 'pu' branch, but I generally hate tweaking things after 15:00 my time), but I'll fix that before the topic hits 'jch' (which is a bit more than 'next' and that is what I use for everyday work).

Thanks.  

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

Fine, you do not like it. Saying so (instead of asking me questions) would have been helpful.

It was not tongue-in-cheek, I was being serious.

I'd much rather prefer https://github.com/git-for-windows/git/pull/1030 than your patch.

Ciao, Johannes

We already handle PuTTY's plink and TortoiseGit's tortoiseplink in GIT_SSH by automatically using the -P option to specify ports, and in tortoiseplink's case by passing the --batch option.

For users who need to pass additional command-line options to plink, this poses a problem: the only way to do that is to use GIT_SSH_COMMAND, but Git does not handle that specifically, so those users have to manually parse the command-line options passed via GIT_SSH_COMMAND and replace -p (if present) by -P, and add --batch in the case of tortoiseplink.

This is error-prone and a bad user experience.

To fix this, the changes proposed in this patch series introduce handling this by splitting the GIT_SSH_COMMAND value and treating the first parameter with the same grace as GIT_SSH. To counter any possible misdetection, the user can also specify explicitly via GIT_SSH_VARIANT or ssh.variant which SSH variant they are using.

This is v2 of the patch, now turned patch series. Relative to v1, it integrates Junio's cleanup patch and Segev's follow-up Pull Request that introduces the GIT_SSH_VARIANT and ssh.variant settings to override Git's autodetection manually.

Junio C Hamano (1):
  connect: rename tortoiseplink and putty variables

Segev Finer (2):
  connect: handle putty/plink also in GIT_SSH_COMMAND
  connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

 Documentation/config.txt |  7 +++++
 Documentation/git.txt    |  7 +++++
 connect.c                | 66 +++++++++++++++++++++++++++++++++++-------------
 t/t5601-clone.sh         | 41 ++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 17 deletions(-)

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v2
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v2

Interdiff vs v1:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index af2ae4cc02..f2c210f0a0 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1949,6 +1949,13 @@ Environment variable settings always override any matches.  The URLs that are
  matched against are those given directly to Git commands.  This means any URLs
  visited as a result of a redirection do not participate in matching.
  
 +ssh.variant::
 +	Override the autodetection of plink/tortoiseplink in the SSH
 +	command that 'git fetch' and 'git push' use. It can be set to
 +	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
 +	value will be treated as normal ssh. This is useful in case
 +	that Git gets this wrong.
 +
  i18n.commitEncoding::
  	Character encoding the commit messages are stored in; Git itself
  	does not care per se, but this information is necessary e.g. when
 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index 4f208fab92..c322558aa7 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
  personal `.ssh/config` file.  Please consult your ssh documentation
  for further details.
  
 +`GIT_SSH_VARIANT`::
 +	If this environment variable is set, it overrides the autodetection
 +	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
 +	push' use. It can be set to either `ssh`, `plink`, `putty` or
 +	`tortoiseplink`. Any other value will be treated as normal ssh. This
 +	is useful in case that Git gets this wrong.
 +
  `GIT_ASKPASS`::
  	If this environment variable is set, then Git commands which need to
  	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
 diff --git a/connect.c b/connect.c
 index c81f77001b..7b4437578b 100644
 --- a/connect.c
 +++ b/connect.c
 @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
  	return NULL;
  }
  
 +static int handle_ssh_variant(int *port_option, int *needs_batch)
 +{
 +	const char *variant;
 +
 +	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
 +		git_config_get_string_const("ssh.variant", &variant))
 +		return 0;
 +
 +	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
 +		*port_option = 'P';
 +	else if (!strcmp(variant, "tortoiseplink")) {
 +		*port_option = 'P';
 +		*needs_batch = 1;
 +	}
 +
 +	return 1;
 +}
 +
  /*
   * This returns a dummy child_process if the transport protocol does not
   * need fork(2), or a struct child_process object if it does.  Once done,
 @@ -769,7 +787,8 @@ struct child_process *git_connect(int fd[2], const char *url,
  		conn->in = conn->out = -1;
  		if (protocol == PROTO_SSH) {
  			const char *ssh;
 -			int putty = 0, tortoiseplink = 0;
 +			int needs_batch = 0;
 +			int port_option = 'p';
  			char *ssh_host = hostandport;
  			const char *port = NULL;
  			char *ssh_argv0 = NULL;
 @@ -816,28 +835,32 @@ struct child_process *git_connect(int fd[2], const char *url,
  				ssh_argv0 = xstrdup(ssh);
  			}
  
 -			if (ssh_argv0) {
 +			if (!handle_ssh_variant(&port_option, &needs_batch) &&
 +			    ssh_argv0) {
  				const char *base = basename(ssh_argv0);
  
 -				tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
 -					!strcasecmp(base, "tortoiseplink.exe");
 -				putty = tortoiseplink ||
 -					!strcasecmp(base, "plink") ||
 -					!strcasecmp(base, "plink.exe");
 -
 -				free(ssh_argv0);
 +				if (!strcasecmp(base, "tortoiseplink") ||
 +				    !strcasecmp(base, "tortoiseplink.exe")) {
 +					port_option = 'P';
 +					needs_batch = 1;
 +				} else if (!strcasecmp(base, "plink") ||
 +					   !strcasecmp(base, "plink.exe")) {
 +					port_option = 'P';
 +				}
  			}
  
 +			free(ssh_argv0);
 +
  			argv_array_push(&conn->args, ssh);
  			if (flags & CONNECT_IPV4)
  				argv_array_push(&conn->args, "-4");
  			else if (flags & CONNECT_IPV6)
  				argv_array_push(&conn->args, "-6");
 -			if (tortoiseplink)
 +			if (needs_batch)
  				argv_array_push(&conn->args, "-batch");
  			if (port) {
 -				/* P is for PuTTY, p is for OpenSSH */
 -				argv_array_push(&conn->args, putty ? "-P" : "-p");
 +				argv_array_pushf(&conn->args,
 +						 "-%c", port_option);
  				argv_array_push(&conn->args, port);
  			}
  			argv_array_push(&conn->args, ssh_host);
 diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
 index 9335e10c2a..b52b8acf98 100755
 --- a/t/t5601-clone.sh
 +++ b/t/t5601-clone.sh
 @@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
  	expect_ssh "-v -P 123" myhost src
  '
  
 +test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
 +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 +	GIT_SSH_VARIANT=ssh \
 +	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
 +	expect_ssh "-p 123" myhost src
 +'
 +
 +test_expect_success 'ssh.variant overrides plink detection' '
 +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 +	git -c ssh.variant=ssh \
 +		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
 +	expect_ssh "-p 123" myhost src
 +'
 +
 +test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
 +	GIT_SSH_VARIANT=plink \
 +	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
 +	expect_ssh "-P 123" myhost src
 +'
 +
 +test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
 +	GIT_SSH_VARIANT=tortoiseplink \
 +	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
 +	expect_ssh "-batch -P 123" myhost src
 +'
 +
  # Reset the GIT_SSH environment variable for clone tests.
  setup_ssh_wrapper
  

From: Segev Finer <segev208@gmail.com>

Git for Windows has special support for the popular SSH client PuTTY: when using PuTTY's non-interactive version ("plink.exe"), we use the -P option to specify the port rather than OpenSSH's -p option. TortoiseGit ships with its own, forked version of plink.exe, that adds support for the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the user overrides the SSH command via the environment variable GIT_SSH (which allows specifying the name of the executable), not GIT_SSH_COMMAND (which allows specifying a full command, including additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink, such as setting a private key, they are required to either use a shell script named plink or tortoiseplink or duplicate the logic that is already in Git for passing the correct style of command line arguments, which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c        | 23 ++++++++++++++++-------
 t/t5601-clone.sh | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

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

One of these two may have originally been named after "what exact SSH implementation do we have" so that we can tweak the command line options, but these days "putty=1" no longer means "We are using the plink SSH implementation that comes with PuTTY". It is set when we guess that either PuTTY plink or Tortoiseplink is in use.

Rename them after what effect is desired. The current "putty" option is about using "-P <port>" when OpenSSH would use "-p <port>", so rename it to port_option whose value is either 'p' or 'P". The other one is about passing an extra command line option "-batch", so rename it needs_batch.

[jes: wrapped overly-long line]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

From: Segev Finer <segev208@gmail.com>

This environment variable and configuration value allow to override the autodetection of plink/tortoiseplink in case that Git gets it wrong.

[jes: wrapped overly-long lines, changed get_ssh_variant() to handle_ssh_variant() to accomodate the change from the putty/tortoiseplink variables to port_option/needs_batch.]

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  7 +++++++
 Documentation/git.txt    |  7 +++++++
 connect.c                | 24 ++++++++++++++++++++++--
 t/t5601-clone.sh         | 26 ++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)

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

I do like the fact that this now sits under ssh.* hierarchy (not core.*).

It should mention core.sshCommand, GIT_SSH_COMMAND, etc., because reading this alone would mislead users to set only this one and expect that their plink will be used without setting core.sshCommand etc.

It also should say that GIT_SSH_VARIANT environment variable will override this variable.

Similarly this should mention GIT_SSH_COMMAND at least. It is crazy to set something that will cause misdetection to core.sshCommand and use this environment variable to fix it (instead of using ssh.variant), so I think it is OK not to mention core.sshCommand (but it would not hurt to do so).

Between handle and get I do not think there is strong reason to favor one over the other. Unlike the one I sent yesterday, this is not overriding but is getting, so we should keep the original name Segev gave it, which is shorter, I would think, rather than renaming it to "handle".

The string that came from the configuration must be freed, no? That is what I recall in Peff's comment yesterday.

I like the placement of this "if the user explicitly told us" much better.

My patch yesterday was deliberately being lazy, because the next thought that will come to us after comparing the two is "this way, we avoid having to do the auto-detection altogether, which is way better than wasting the effort to auto-detect, only to override".

And that reasoning will lead to a realization "there is no reason to even do the split_cmdline() if the user explicitly told us". While that reasoning is correct and we _should_ further refactor, I didn't want to spend braincycles on that myself.

Hi Junio,

On Thu, 26 Jan 2017, Junio C Hamano wrote:

That is correct. "handle" and "get" are two very beautiful words, and none of them deserves to take a back seat behind the other.

In this case, "get" is inappropriate, though, as the function does not return the ssh variant, nor does it assign the ssh variant to any variable to which any of its argument points.

Will try to find the time to address the other issues soon, Johannes

Just to save us extra round-trip.

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

The leak needs plugging in some way.  

Because this thing is merely an escape hatch that somebody who needs it needs to use it always (as opposed to one-shot basis), we do not need to support the environment variable and go with only the configuration, which may make it easier to plug the leak.

This _should_ above is a lot weaker than _must_.  

IOW, I think it is acceptable to always split GIT_SSH_COMMAND into tokens before we realize that the user used the escape hatch and the splitting was a wasted effort. This is exactly because this thing is an escape hatch that is expected to be rarely used. Of course, if the "wasted effort" can be eliminated without sacrificing the simplicity of the code, that is fine as well.

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

Simplicity is retained. Battle-readiness was sacrificed on the way: the new code is not tested well enough, and `next` will not help one bit.

Ciao, Johannes

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

Let me make it clear that there is no burning desire to sacrifice battle-readiness in the above. If we expect that auto-detection would be minority, then it makes sense to get the configured value first and then spend cycles to split and guess only when detection is needed.

In this case, because we expect that auto-detection will be used most of the time, it is good enough to always split first, get the configured value, and spend cycles to guess, or for that matter it is perfectly fine to always split and guess first and then override with the configured value.

If your attempt to optimize for a wrong case ended up causing new unnecessary bugs, don't blame me.

We already handle PuTTY's plink and TortoiseGit's tortoiseplink in GIT_SSH by automatically using the -P option to specify ports, and in tortoiseplink's case by passing the --batch option.

For users who need to pass additional command-line options to plink, this poses a problem: the only way to do that is to use GIT_SSH_COMMAND, but Git does not handle that specifically, so those users have to manually parse the command-line options passed via GIT_SSH_COMMAND and replace -p (if present) by -P, and add --batch in the case of tortoiseplink.

This is error-prone and a bad user experience.

To fix this, the changes proposed in this patch series introduce handling this by splitting the GIT_SSH_COMMAND value and treating the first parameter with the same grace as GIT_SSH. To counter any possible misdetection, the user can also specify explicitly via GIT_SSH_VARIANT or ssh.variant which SSH variant they are using.

Changes relative to v2:

- touched up the documentation for ssh.variant to make it even easier to
  understand

- free()d the config variable

- completely refactored the code to fulfil Junio's burning desire to
  avoid split_cmdline() when unnecessary

It is quite preposterous to call this an "iteration" of the patch series, because the code is so different now. I say this because I want to caution that this code has not been tested as thoroughly, by far, as the first iteration.

The primary purpose of code review is correctness, everything else is either a consequence of it, or a means to make reviewing easier.

That means that I highly encourage those who pushed for these extensive changes that make the patch series a lot less robust to balance things out by at least *rudimentary* testing.

Johannes Schindelin (1):
  git_connect(): factor out SSH variant handling

Junio C Hamano (1):
  connect: rename tortoiseplink and putty variables

Segev Finer (2):
  connect: handle putty/plink also in GIT_SSH_COMMAND
  connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

 Documentation/config.txt | 11 +++++++
 Documentation/git.txt    |  6 ++++
 connect.c                | 75 ++++++++++++++++++++++++++++++++++++------------
 t/t5601-clone.sh         | 41 ++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 19 deletions(-)

base-commit: 8f60064c1f538f06e1c579cbd9840b86b10bcd3d
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v3
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v3

Interdiff vs v2:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index f2c210f0a0..b88df57ab6 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands.  This means any URLs
  visited as a result of a redirection do not participate in matching.
  
  ssh.variant::
 -	Override the autodetection of plink/tortoiseplink in the SSH
 -	command that 'git fetch' and 'git push' use. It can be set to
 -	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
 -	value will be treated as normal ssh. This is useful in case
 -	that Git gets this wrong.
 +	Depending on the value of the environment variables `GIT_SSH` or
 +	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
 +	auto-detects whether to adjust its command-line parameters for use
 +	with plink or tortoiseplink, as opposed to the default (OpenSSH).
 ++
 +The config variable `ssh.variant` can be set to override this auto-detection;
 +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
 +will be treated as normal ssh. This setting can be overridden via the
 +environment variable `GIT_SSH_VARIANT`.
  
  i18n.commitEncoding::
  	Character encoding the commit messages are stored in; Git itself
 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index c322558aa7..a0c6728d1a 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -1021,11 +1021,10 @@ personal `.ssh/config` file.  Please consult your ssh documentation
  for further details.
  
  `GIT_SSH_VARIANT`::
 -	If this environment variable is set, it overrides the autodetection
 -	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
 -	push' use. It can be set to either `ssh`, `plink`, `putty` or
 -	`tortoiseplink`. Any other value will be treated as normal ssh. This
 -	is useful in case that Git gets this wrong.
 +	If this environment variable is set, it overrides Git's autodetection
 +	whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
 +	plink or tortoiseplink. This variable overrides the config setting
 +	`ssh.variant` that serves the same purpose.
  
  `GIT_ASKPASS`::
  	If this environment variable is set, then Git commands which need to
 diff --git a/connect.c b/connect.c
 index 7b4437578b..7f1f802396 100644
 --- a/connect.c
 +++ b/connect.c
 @@ -691,20 +691,45 @@ static const char *get_ssh_command(void)
  	return NULL;
  }
  
 -static int handle_ssh_variant(int *port_option, int *needs_batch)
 +static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 +			      int *port_option, int *needs_batch)
  {
 -	const char *variant;
 +	const char *variant = getenv("GIT_SSH_VARIANT");
 +	char *p = NULL;
 +
 +	if (variant)
 +		; /* okay, fall through */
 +	else if (!git_config_get_string("ssh.variant", &p))
 +		variant = p;
 +	else if (!is_cmdline) {
 +		p = xstrdup(ssh_command);
 +		variant = basename(p);
 +	} else {
 +		const char **ssh_argv;
  
 -	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
 -		git_config_get_string_const("ssh.variant", &variant))
 -		return 0;
 +		p = xstrdup(ssh_command);
 +		if (split_cmdline(p, &ssh_argv)) {
 +			variant = basename((char *)ssh_argv[0]);
 +			/*
 +			 * At this point, variant points into the buffer
 +			 * referenced by p, hence we do not need ssh_argv
 +			 * any longer.
 +			 */
 +			free(ssh_argv);
 +		} else
 +			return 0;
 +	}
  
 -	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
 +	if (!strcasecmp(variant, "plink") ||
 +	    !strcasecmp(variant, "plink.exe") ||
 +	    !strcasecmp(variant, "putty"))
  		*port_option = 'P';
 -	else if (!strcmp(variant, "tortoiseplink")) {
 +	else if (!strcasecmp(variant, "tortoiseplink") ||
 +		 !strcasecmp(variant, "tortoiseplink.exe")) {
  		*port_option = 'P';
  		*needs_batch = 1;
  	}
 +	free(p);
  
  	return 1;
  }
 @@ -791,7 +816,6 @@ struct child_process *git_connect(int fd[2], const char *url,
  			int port_option = 'p';
  			char *ssh_host = hostandport;
  			const char *port = NULL;
 -			char *ssh_argv0 = NULL;
  			transport_check_allowed("ssh");
  			get_host_and_port(&ssh_host, &port);
  
 @@ -812,15 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,
  			}
  
  			ssh = get_ssh_command();
 -			if (ssh) {
 -				char *split_ssh = xstrdup(ssh);
 -				const char **ssh_argv;
 -
 -				if (split_cmdline(split_ssh, &ssh_argv))
 -					ssh_argv0 = xstrdup(ssh_argv[0]);
 -				free(split_ssh);
 -				free((void *)ssh_argv);
 -			} else {
 +			if (ssh)
 +				handle_ssh_variant(ssh, 1, &port_option,
 +						   &needs_batch);
 +			else {
  				/*
  				 * GIT_SSH is the no-shell version of
  				 * GIT_SSH_COMMAND (and must remain so for
 @@ -831,26 +850,12 @@ struct child_process *git_connect(int fd[2], const char *url,
  				ssh = getenv("GIT_SSH");
  				if (!ssh)
  					ssh = "ssh";
 -
 -				ssh_argv0 = xstrdup(ssh);
 +				else
 +					handle_ssh_variant(ssh, 0,
 +							   &port_option,
 +							   &needs_batch);
  			}
  
 -			if (!handle_ssh_variant(&port_option, &needs_batch) &&
 -			    ssh_argv0) {
 -				const char *base = basename(ssh_argv0);
 -
 -				if (!strcasecmp(base, "tortoiseplink") ||
 -				    !strcasecmp(base, "tortoiseplink.exe")) {
 -					port_option = 'P';
 -					needs_batch = 1;
 -				} else if (!strcasecmp(base, "plink") ||
 -					   !strcasecmp(base, "plink.exe")) {
 -					port_option = 'P';
 -				}
 -			}
 -
 -			free(ssh_argv0);
 -
  			argv_array_push(&conn->args, ssh);
  			if (flags & CONNECT_IPV4)
  				argv_array_push(&conn->args, "-4");

From: Segev Finer <segev208@gmail.com>

Git for Windows has special support for the popular SSH client PuTTY: when using PuTTY's non-interactive version ("plink.exe"), we use the -P option to specify the port rather than OpenSSH's -p option. TortoiseGit ships with its own, forked version of plink.exe, that adds support for the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the user overrides the SSH command via the environment variable GIT_SSH (which allows specifying the name of the executable), not GIT_SSH_COMMAND (which allows specifying a full command, including additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink, such as setting a private key, they are required to either use a shell script named plink or tortoiseplink or duplicate the logic that is already in Git for passing the correct style of command line arguments, which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c        | 23 ++++++++++++++++-------
 t/t5601-clone.sh | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

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

One of these two may have originally been named after "what exact SSH implementation do we have" so that we can tweak the command line options, but these days "putty=1" no longer means "We are using the plink SSH implementation that comes with PuTTY". It is set when we guess that either PuTTY plink or Tortoiseplink is in use.

Rename them after what effect is desired. The current "putty" option is about using "-P <port>" when OpenSSH would use "-p <port>", so rename it to port_option whose value is either 'p' or 'P". The other one is about passing an extra command line option "-batch", so rename it needs_batch.

[jes: wrapped overly-long line]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

We handle plink and tortoiseplink as OpenSSH replacements, by passing the correct command-line options when detecting that they are used.

To let users override that auto-detection (in case Git gets it wrong), we need to introduce new code to that end.

In preparation for this code, let's factor out the SSH variant handling into its own function, handle_ssh_variant().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c | 72 ++++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 26 deletions(-)

From: Segev Finer <segev208@gmail.com>

This environment variable and configuration value allow to override the autodetection of plink/tortoiseplink in case that Git gets it wrong.

[jes: wrapped overly-long lines, factored out and changed get_ssh_variant() to handle_ssh_variant() to accomodate the change from the putty/tortoiseplink variables to port_option/needs_batch, adjusted the documentation, free()d value obtained from the config.]

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt | 11 +++++++++++
 Documentation/git.txt    |  6 ++++++
 connect.c                | 11 ++++++++---
 t/t5601-clone.sh         | 26 ++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 3 deletions(-)

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

This means that "putty" that appear as the first word of the command line, not the value configured for ssh.variant or GIT_SSH_VARIANT, will also trigger the option for "plink", no?

Worse, "plink.exe" configured as the value of "ssh.variant", is anything other than 'ssh', 'plink', 'putty', or 'tortoiseplink', but it is not treated as normal ssh, contrary to the documentation.

I was hoping that the "rewrite that retains simplicity" was also correct, but let's not go in this direction. The more lines you touch in a hurry, the worse the code will get, and that is to be expected.

I'd suggest taking the documentation updates from this series, and then minimally plug the leak introduced by the previous round, perhaps like the attached SQUASH. As I said, GIT_SSH_VARIANT and ssh.variant are expected to be rare cases, and the case in which they are set does not have to be optimized if it makes the necessary change smaller and more likely to be correct.

I think restructuring along the line of 3/4 of this round is very sensible in the longer term (if anything, handle_ssh_variant() now really handles ssh variants in all cases, which makes it worthy of its name, as opposed to the one in the previous round that only reacts to the overrides). But it seems that it will take longer to get such a rewrite right, and my priority is seeing this topic to add autodetection to GIT_SSH_COMMAND and other smaller topics in the upcoming -rc0 in a serviceable and correct shape.

The restructuring done by 3/4 can come later after the dust settles, if somebody cares deeply enough about performance in the rare cases.

 Documentation/config.txt | 14 +++++++++-----
 Documentation/git.txt    |  9 ++++-----
 connect.c                |  9 +++++----
 3 files changed, 18 insertions(+), 14 deletions(-)

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

Having said all that, because I like the patch 3/4 so much, here is another way to fix this, and I think (of course I am biased, but...) the result is easier to grok. Not only it makes it clear that the namespace for the actual command names on the command line and the namespace for the supported values of the configuration are distinct, it makes it clear that we do not do anything extra when the user explicitly tells us which variant to use.

This is designed to be squashed into [4/4] of this latest round (as opposed to the one in the message I am responding to, which is to be squashed into the previous round).

 connect.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

That leaves the "putty" case in handle_ssh_variant(), does it not? Was it not your specific objection that that is the case?

No worries, I will let this simmer for a while. Your fixup has a lot of duplicated code (so much for maintainability as an important goal... ;-)) and I will have to think about it. My immediate thinking is to *not* duplicate code, strip the .exe extension directly after calling basename() in the cases where we handle commands, and handle "putty" in the other cases by replacing it with the string "plink".

But as I said, this will simmer for a while.

Ciao, Johannes

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

Yup, you can remove that while you reroll.

You need to realize that the namespaces of the configuration and the command names are distinct. There is no code duplication.

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

Since your 2/4 did away with the "plink" and "tortoiseplink" values in favor of "port_option" and "batch_option", there is a duplication of logic which I tried to undo in v3 and which you reintroduce in your fixup.

Maybe that refactoring was not so smart to begin with, and we should have instead modified the code to use an enum instead and keep the original conditionals instead of setting a port_option and a batch_option explicitly.

Ciao, Johannes

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

To explain this a bit, there is no reason why allowed values for SSH_VARIANT must be "putty" and "tortoiseplink". An alternative design could be "port_option=-p,needs_batch=yes" and it may be more logical and futureproof if a variant of tortoiseplink decides to use "-p" instead of "-P" and still require "-batch".

Prematurely attempting to share code, only because the current vocabularies for two distinct concepts happen to overlap, is not de-duplicating the code for maintainability. It is adding unnecessary work other people need to do in the future when they want to extend the system.

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

Except, of course, that your hypothetical port_option and needs_batch settings would be handled at a different point altogether.

I sense very strongly that this discussion has taken a very emotional turn, which is detrimental to the quality. So let's take a break here.

Ciao, Johannes

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

You are utterly wrong here.

The primary purpose of code review is to spot and correct the problems the submitter has missed. The problems can span from stupid bugs that affect correctness to maintainability, to design mistakes, to clarity of explanation for both end users and future developers.

Among them, correctness problems are, as long as the problem to be solved is specified clearly enough, the easiest to spot by the submitter before the patch is sent out. The submitter is focused on solving one problem, and if the updated code does not even work as the submitter advertises it would, that can be caught by the submitter before the patch is even sent out.

Of course, humans are not perfect, and catching correctness problems is important, but that is not the only (let alone primary) thing.

When a submitter has been focusing on solving one problem, it is easy to lose the larger picture and to end up adding something that may be "correct" (in the sense of "works as advertised by the submitter") but does not fit well with the rest of the system, or covers some use cases but misses other important and related use cases. If the "does not fit well" surfaces to the end user level, that would become a design problem. If it affects the future developers, that would become a maintainability problem.

Compared to the correctness issue, these are much harder to spot by the submitter alone, who focused so intensely to get his code "correct". The review process is of greater value to spot these issues.

I've already read and commented on the series; as I said, I think the rewrite in 3/4 makes the resulting code much easier to read, and with the fix-up I just sent out, I think the end result is correct, works as advertised, the way it is advertised is clear to end users, and is maintainable.

But I do share your uneasiness about the "new-ness" of the code.

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

We will never agree on this.

From my perspective, design, explanation and maintainability are a consequence of making it easy for reviewers to spot where the code is incorrect.

And correctness is not covered by "the submitter tested this". Correctness includes all the corner cases, where the "many eyes make bugs shallow" really shines.

I'd rather have reviewers find bugs than users.

I will *never* be a fan of a review process that pushes correctness to a back seat (yes, it is much harder than spotting typos or lines longer than 80 columns per row, but the ultimate goal is to deliver value to the end user, not to make life easy for the maintainer).

Ciao, Johannes

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

That's too bad.

I'd rather have submitters find bugs than reviewers.

Did I ever say correctness is pushed to a back seat?

I said that it is easier to spot correctness issues for you as a submitter than other kinds of issues without outside help (and implied that if you are a diligent contributor, you should aim for, and you should be able to achieve, a patch series where correctness issues do not need to be pointed out). But other higher level issues are harder for any submitter to spot (regardless of experience and competence of the submitter), because one gets so married to one's own code, design and worldview. And that is why "review is primarily to spot bugs" can never be a correct viewpoint.

A reviewee needs to be prepared to accept review comments on higher level issues, even more readily than comments on correctness issues, because it is too easy to be constrained by early decisions one has already made while preparing a patch series and become blind to bigger picture after staring one's own new code for number of hours. Higher level issues can be more easily spotted by reviewers, whose eyes are still fresh to the series.