[PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH

[Jonathan Nieder]

Previously: [1].

This version should be essentially identical to v2.  Changes:
- patch 1 is new and should fix the test failure on Windows
- patch 2 is new, discussed at [2]
- patch 5 split off from patch 6 as suggested at [3]
- patch 6 commit message got two new notes to address the worries
  from [3]

Thanks for the helpful reviews, and sorry to take so long to get this
out.  Thoughts of all kinds welcome, as always.

Sincerely,
Jonathan Nieder (8):
  ssh test: make copy_ssh_wrapper_as clean up after itself
  connect: move no_fork fallback to git_tcp_connect
  connect: split git:// setup into a separate function
  connect: split ssh command line options into separate function
  connect: split ssh option computation to its own function
  ssh: 'auto' variant to select between 'ssh' and 'simple'
  ssh: 'simple' variant does not support -4/-6
  ssh: 'simple' variant does not support --port

 Documentation/config.txt |  24 ++--
 connect.c| 322 +--
 t/t5601-clone.sh |  69 ++
 t/t5603-clone-dirname.sh |   2 +
 4 files changed, 265 insertions(+), 152 deletions(-)

[1] 
https://public-inbox.org/git/20171023231625.6mhcyqti7vdg6...@aiede.mtv.corp.google.com/
[2] 
https://public-inbox.org/git/20171115202516.hduhzsgeoff5a...@aiede.mtv.corp.google.com/
[3] https://public-inbox.org/git/xmqq60b59toe@gitster.mtv.corp.google.com/

[PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Jonathan Nieder]

Simplify by not allowing the copied ssh wrapper to persist between
tests.  This way, tests can be safely reordered, added, and removed
with less fear of hidden side effects.

This also avoids having to call setup_ssh_wrapper to restore the value
of GIT_SSH after this battery of tests, since it means each test will
restore it individually.

Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash
will overwrite that when redirecting via `>uplink`.  A proposed test
wrote a script to 'uplink' after a previous test created uplink.exe
using copy_ssh_wrapper_as, so the script written with '>uplink' had
the wrong filename and failed.

Reported-by: Johannes Schindelin 
Signed-off-by: Jonathan Nieder 
---
Thanks to Dscho for tracking this subtle issue down.

 t/t5601-clone.sh | 28 
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 86811a0c35..9d007c0f8d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
test_cmp fetch.expected fetch.actual
 '
 
-setup_ssh_wrapper () {
-   test_expect_success 'setup ssh wrapper' '
-   cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
-   "$TRASH_DIRECTORY/ssh$X" &&
-   GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
-   export GIT_SSH &&
-   export TRASH_DIRECTORY &&
-   >"$TRASH_DIRECTORY"/ssh-output
-   '
-}
+test_expect_success 'set up ssh wrapper' '
+   cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
+   "$TRASH_DIRECTORY/ssh$X" &&
+   GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
+   export GIT_SSH &&
+   export TRASH_DIRECTORY &&
+   >"$TRASH_DIRECTORY"/ssh-output
+'
 
 copy_ssh_wrapper_as () {
cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
+   test_when_finished "rm -f ${1%$X}$X" &&
GIT_SSH="${1%$X}$X" &&
-   export GIT_SSH
+   test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""
 }
 
 expect_ssh () {
@@ -344,8 +343,6 @@ expect_ssh () {
(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
 }
 
-setup_ssh_wrapper
-
 test_expect_success 'clone myhost:src uses ssh' '
git clone myhost:src ssh-clone &&
expect_ssh myhost src
@@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink 
detection' '
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/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' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
GIT_SSH_VARIANT=tortoiseplink \
git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
expect_ssh "-batch -P 123" myhost src
@@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '
git clone "[myhost:123]:src" sq-failure
 '
 
-# Reset the GIT_SSH environment variable for clone tests.
-setup_ssh_wrapper
-
 counter=0
 # $1 url
 # $2 none|host
-- 
2.15.0.448.gf294e3d99a

[PATCH 3/8] connect: split git:// setup into a separate function

[Jonathan Nieder]

The git_connect function is growing long.  Split the
PROTO_GIT-specific portion to a separate function to make it easier to
read.

No functional change intended.

Signed-off-by: Jonathan Nieder 
Reviewed-by: Stefan Beller 
---
As before.

 connect.c | 103 +++---
 1 file changed, 59 insertions(+), 44 deletions(-)

diff --git a/connect.c b/connect.c
index aa994d1518..9425229206 100644
--- a/connect.c
+++ b/connect.c
@@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char 
*ssh_command,
return ssh_variant;
 }
 
+/*
+ * Open a connection using Git's native protocol.
+ *
+ * The caller is responsible for freeing hostandport, but this function may
+ * modify it (for example, to truncate it to remove the port part).
+ */
+static struct child_process *git_connect_git(int fd[2], char *hostandport,
+const char *path, const char *prog,
+int flags)
+{
+   struct child_process *conn;
+   struct strbuf request = STRBUF_INIT;
+   /*
+* Set up virtual host information based on where we will
+* connect, unless the user has overridden us in
+* the environment.
+*/
+   char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+   if (target_host)
+   target_host = xstrdup(target_host);
+   else
+   target_host = xstrdup(hostandport);
+
+   transport_check_allowed("git");
+
+   /* These underlying connection commands die() if they
+* cannot connect.
+*/
+   if (git_use_proxy(hostandport))
+   conn = git_proxy_connect(fd, hostandport);
+   else
+   conn = git_tcp_connect(fd, hostandport, flags);
+   /*
+* Separate original protocol components prog and path
+* from extended host header with a NUL byte.
+*
+* Note: Do not add any other headers here!  Doing so
+* will cause older git-daemon servers to crash.
+*/
+   strbuf_addf(&request,
+   "%s %s%chost=%s%c",
+   prog, path, 0,
+   target_host, 0);
+
+   /* If using a new version put that stuff here after a second null byte 
*/
+   if (get_protocol_version_config() > 0) {
+   strbuf_addch(&request, '\0');
+   strbuf_addf(&request, "version=%d%c",
+   get_protocol_version_config(), '\0');
+   }
+
+   packet_write(fd[1], request.buf, request.len);
+
+   free(target_host);
+   strbuf_release(&request);
+   return conn;
+}
+
 /*
  * This returns the dummy child_process `no_fork` if the transport protocol
  * does not need fork(2), or a struct child_process object if it does.  Once
@@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
-   struct strbuf request = STRBUF_INIT;
-   /*
-* Set up virtual host information based on where we will
-* connect, unless the user has overridden us in
-* the environment.
-*/
-   char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
-   if (target_host)
-   target_host = xstrdup(target_host);
-   else
-   target_host = xstrdup(hostandport);
-
-   transport_check_allowed("git");
-
-   /* These underlying connection commands die() if they
-* cannot connect.
-*/
-   if (git_use_proxy(hostandport))
-   conn = git_proxy_connect(fd, hostandport);
-   else
-   conn = git_tcp_connect(fd, hostandport, flags);
-   /*
-* Separate original protocol components prog and path
-* from extended host header with a NUL byte.
-*
-* Note: Do not add any other headers here!  Doing so
-* will cause older git-daemon servers to crash.
-*/
-   strbuf_addf(&request,
-   "%s %s%chost=%s%c",
-   prog, path, 0,
-   target_host, 0);
-
-   /* If using a new version put that stuff here after a second 
null byte */
-   if (get_protocol_version_config() > 0) {
-   strbuf_addch(&request, '\0');
-   strbuf_addf(&request, "version=%d%c",
-   get_protocol_version_config(), '\0');
-   }
-
-

[PATCH 2/8] connect: move no_fork fallback to git_tcp_connect

[Jonathan Nieder]

git_connect has the structure

struct child_process *conn = &no_fork;

...
switch (protocol) {
case PROTO_GIT:
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
git_tcp_connect(fd, hostandport, flags);
...
break;
case PROTO_SSH:
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
argv_array_push(&conn->args, ssh);
...
break;
...
return conn;

In all cases except the git_tcp_connect case, conn is explicitly
assigned a value. Make the code clearer by explicitly assigning
'conn = &no_fork' in the tcp case and eliminating the default so the
compiler can ensure conn is always correctly assigned.

Noticed-by: Junio C Hamano 
Signed-off-by: Jonathan Nieder 
---
 connect.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/connect.c b/connect.c
index 7fbd396b35..aa994d1518 100644
--- a/connect.c
+++ b/connect.c
@@ -582,12 +582,25 @@ static int git_tcp_connect_sock(char *host, int flags)
 #endif /* NO_IPV6 */
 
 
-static void git_tcp_connect(int fd[2], char *host, int flags)
+/*
+ * Dummy child_process returned by git_connect() if the transport protocol
+ * does not need fork(2).
+ */
+static struct child_process no_fork = CHILD_PROCESS_INIT;
+
+int git_connection_is_socket(struct child_process *conn)
+{
+   return conn == &no_fork;
+}
+
+static struct child_process *git_tcp_connect(int fd[2], char *host, int flags)
 {
int sockfd = git_tcp_connect_sock(host, flags);
 
fd[0] = sockfd;
fd[1] = dup(sockfd);
+
+   return &no_fork;
 }
 
 
@@ -761,8 +774,6 @@ static enum protocol parse_connect_url(const char 
*url_orig, char **ret_host,
return protocol;
 }
 
-static struct child_process no_fork = CHILD_PROCESS_INIT;
-
 static const char *get_ssh_command(void)
 {
const char *ssh;
@@ -851,11 +862,11 @@ static enum ssh_variant determine_ssh_variant(const char 
*ssh_command,
 }
 
 /*
- * 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,
- * finish the connection with finish_connect() with the value returned from
- * this function (it is safe to call finish_connect() with NULL to support
- * the former case).
+ * This returns the dummy child_process `no_fork` if the transport protocol
+ * does not need fork(2), or a struct child_process object if it does.  Once
+ * done, finish the connection with finish_connect() with the value returned
+ * from this function (it is safe to call finish_connect() with NULL to
+ * support the former case).
  *
  * If it returns, the connect is successful; it just dies on errors (this
  * will hopefully be changed in a libification effort, to return NULL when
@@ -865,7 +876,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
  const char *prog, int flags)
 {
char *hostandport, *path;
-   struct child_process *conn = &no_fork;
+   struct child_process *conn;
enum protocol protocol;
 
/* Without this we cannot rely on waitpid() to tell
@@ -901,7 +912,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
-   git_tcp_connect(fd, hostandport, flags);
+   conn = git_tcp_connect(fd, hostandport, flags);
/*
 * Separate original protocol components prog and path
 * from extended host header with a NUL byte.
@@ -1041,11 +1052,6 @@ struct child_process *git_connect(int fd[2], const char 
*url,
return conn;
 }
 
-int git_connection_is_socket(struct child_process *conn)
-{
-   return conn == &no_fork;
-}
-
 int finish_connect(struct child_process *conn)
 {
int code;
-- 
2.15.0.448.gf294e3d99a

[PATCH 4/8] connect: split ssh command line options into separate function

[Jonathan Nieder]

The git_connect function is growing long.  Split the portion that
discovers an ssh command and options it accepts before the service
name and path to a separate function to make it easier to read.

No functional change intended.

Signed-off-by: Jonathan Nieder 
Reviewed-by: Stefan Beller 
---
As before.

 connect.c | 116 +-
 1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/connect.c b/connect.c
index 9425229206..a9dc493db2 100644
--- a/connect.c
+++ b/connect.c
@@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
return conn;
 }
 
+/* Prepare a child_process for use by Git's SSH-tunneled transport. */
+static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
+ const char *port, int flags)
+{
+   const char *ssh;
+   enum ssh_variant variant;
+
+   if (looks_like_command_line_option(ssh_host))
+   die("strange hostname '%s' blocked", ssh_host);
+
+   ssh = get_ssh_command();
+   if (ssh) {
+   variant = determine_ssh_variant(ssh, 1);
+   } else {
+   /*
+* GIT_SSH is the no-shell version of
+* GIT_SSH_COMMAND (and must remain so for
+* historical compatibility).
+*/
+   conn->use_shell = 0;
+
+   ssh = getenv("GIT_SSH");
+   if (!ssh)
+   ssh = "ssh";
+   variant = determine_ssh_variant(ssh, 0);
+   }
+
+   argv_array_push(&conn->args, ssh);
+
+   if (variant == VARIANT_SSH &&
+   get_protocol_version_config() > 0) {
+   argv_array_push(&conn->args, "-o");
+   argv_array_push(&conn->args, "SendEnv=" 
GIT_PROTOCOL_ENVIRONMENT);
+   argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT 
"=version=%d",
+get_protocol_version_config());
+   }
+
+   if (variant != VARIANT_SIMPLE) {
+   if (flags & CONNECT_IPV4)
+   argv_array_push(&conn->args, "-4");
+   else if (flags & CONNECT_IPV6)
+   argv_array_push(&conn->args, "-6");
+   }
+
+   if (variant == VARIANT_TORTOISEPLINK)
+   argv_array_push(&conn->args, "-batch");
+
+   if (port && variant != VARIANT_SIMPLE) {
+   if (variant == VARIANT_SSH)
+   argv_array_push(&conn->args, "-p");
+   else
+   argv_array_push(&conn->args, "-P");
+
+   argv_array_push(&conn->args, port);
+   }
+
+   argv_array_push(&conn->args, ssh_host);
+}
+
 /*
  * This returns the dummy child_process `no_fork` if the transport protocol
  * does not need fork(2), or a struct child_process object if it does.  Once
@@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char 
*url,
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
-   const char *ssh;
-   enum ssh_variant variant;
char *ssh_host = hostandport;
const char *port = NULL;
+
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
-
if (!port)
port = get_port(ssh_host);
-
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", 
prot_name(protocol));
@@ -995,57 +1051,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
strbuf_release(&cmd);
return NULL;
}
-
-   if (looks_like_command_line_option(ssh_host))
-   die("strange hostname '%s' blocked", ssh_host);
-
-   ssh = get_ssh_command();
-   if (ssh) {
-   variant = determine_ssh_variant(ssh, 1);
-   } else {
-   /*
-* GIT_SSH is the no-shell version of
-* GIT_SSH_COMMAND (and must remain so for
-* historical compatibility).
-*/
-   conn->use_shell = 0;
-
-   ssh = getenv

[PATCH 5/8] connect: split ssh option computation to its own function

[Jonathan Nieder]

This puts the determination of options to pass to each ssh variant
(see ssh.variant in git-config(1)) in one place.

A follow-up patch will use this in an initial dry run to detect which
variant to use when the ssh command is ambiguous.

No functional change intended yet.

Signed-off-by: Jonathan Nieder 
---
Split out to make patch 6 easier to read, as suggested at
https://public-inbox.org/git/xmqq60b59toe@gitster.mtv.corp.google.com/.

Added a function comment to make the purpose and API of this internal
helper clearer.

 connect.c | 65 ---
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/connect.c b/connect.c
index a9dc493db2..d2fbb15cc5 100644
--- a/connect.c
+++ b/connect.c
@@ -919,6 +919,42 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
return conn;
 }
 
+/*
+ * Append the appropriate environment variables to `env` and options to
+ * `args` for running ssh in Git's SSH-tunneled transport.
+ */
+static void push_ssh_options(struct argv_array *args, struct argv_array *env,
+enum ssh_variant variant, const char *port,
+int flags)
+{
+   if (variant == VARIANT_SSH &&
+   get_protocol_version_config() > 0) {
+   argv_array_push(args, "-o");
+   argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
+   argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
+get_protocol_version_config());
+   }
+
+   if (variant != VARIANT_SIMPLE) {
+   if (flags & CONNECT_IPV4)
+   argv_array_push(args, "-4");
+   else if (flags & CONNECT_IPV6)
+   argv_array_push(args, "-6");
+   }
+
+   if (variant == VARIANT_TORTOISEPLINK)
+   argv_array_push(args, "-batch");
+
+   if (port && variant != VARIANT_SIMPLE) {
+   if (variant == VARIANT_SSH)
+   argv_array_push(args, "-p");
+   else
+   argv_array_push(args, "-P");
+
+   argv_array_push(args, port);
+   }
+}
+
 /* Prepare a child_process for use by Git's SSH-tunneled transport. */
 static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
  const char *port, int flags)
@@ -947,34 +983,7 @@ static void fill_ssh_args(struct child_process *conn, 
const char *ssh_host,
}
 
argv_array_push(&conn->args, ssh);
-
-   if (variant == VARIANT_SSH &&
-   get_protocol_version_config() > 0) {
-   argv_array_push(&conn->args, "-o");
-   argv_array_push(&conn->args, "SendEnv=" 
GIT_PROTOCOL_ENVIRONMENT);
-   argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT 
"=version=%d",
-get_protocol_version_config());
-   }
-
-   if (variant != VARIANT_SIMPLE) {
-   if (flags & CONNECT_IPV4)
-   argv_array_push(&conn->args, "-4");
-   else if (flags & CONNECT_IPV6)
-   argv_array_push(&conn->args, "-6");
-   }
-
-   if (variant == VARIANT_TORTOISEPLINK)
-   argv_array_push(&conn->args, "-batch");
-
-   if (port && variant != VARIANT_SIMPLE) {
-   if (variant == VARIANT_SSH)
-   argv_array_push(&conn->args, "-p");
-   else
-   argv_array_push(&conn->args, "-P");
-
-   argv_array_push(&conn->args, port);
-   }
-
+   push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
argv_array_push(&conn->args, ssh_host);
 }
 
-- 
2.15.0.448.gf294e3d99a

[PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Jonathan Nieder]

Android's "repo" tool is a tool for managing a large codebase
consisting of multiple smaller repositories, similar to Git's
submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
'simple' ssh variant, 2017-10-16), users noticed that it stopped
handling the port in ssh:// URLs.

The cause: when it encounters ssh:// URLs, repo pre-connects to the
server and sets GIT_SSH to a helper ".repo/repo/git_ssh" that reuses
that connection.  Before 94b8ae5a, the helper was assumed to support
OpenSSH options for lack of a better guess and got passed a -p option
to set the port.  After that patch, it uses the new default of a
simple helper that does not accept an option to set the port.

The next release of "repo" will set GIT_SSH_VARIANT to "ssh" to avoid
that.  But users of old versions and of other similar GIT_SSH
implementations would not get the benefit of that fix.

So update the default to use OpenSSH options again, with a twist.  As
observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles
OpenSSH options: common helpers such as travis-ci's dpl[*] are
configured using GIT_SSH and do not accept OpenSSH options.  So make
the default a new variant "auto", with the following behavior:

 1. First, check for a recognized basename, like today.

 2. If the basename is not recognized, check whether $GIT_SSH supports
OpenSSH options by running

$GIT_SSH -G  

This returns status 0 and prints configuration in OpenSSH if it
recognizes all  and returns status 255 if it encounters
an unrecognized option.  A wrapper script like

exec ssh -- "$@"

would fail with

ssh: Could not resolve hostname -g: Name or service not known

, correctly reflecting that it does not support OpenSSH options.
The command is run with stdin, stdout, and stderr redirected to
/dev/null so even a command that expects a terminal would exit
immediately.

 3. Based on the result from step (2), behave like "ssh" (if it
succeeded) or "simple" (if it failed).

This way, the default ssh variant for unrecognized commands can handle
both the repo and dpl cases as intended.

This autodetection has been running on Google workstations since
2017-10-23 with no reported negative effects.

[*] 
https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c56b068bac0a77c049/lib/dpl/provider.rb#L215

Reported-by: William Yan 
Improved-by: Jonathan Tan 
Signed-off-by: Jonathan Nieder 
---
Added two notes to the commit message:
 - describing the real-world testing this patch has undergone
 - stdin, stdout, and stderr go to /dev/null, preventing a
   hypothetical ssh variant that *ignores* -G from hanging waiting for
   input from the terminal.

This is to address the worries at
https://public-inbox.org/git/xmqq60b59toe@gitster.mtv.corp.google.com/
and 
https://public-inbox.org/git/cagz79kztjuvcq_hkhcqtdoabxt2x+9xcqyc6ao1bhcet2sm...@mail.gmail.com/
about hanging.

No change to the code from last time.

 Documentation/config.txt | 24 +++-
 connect.c| 32 +---
 t/t5601-clone.sh | 21 +
 3 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0460af37e2..0c371ad786 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2081,16 +2081,22 @@ 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::
-   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 ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
-   (simple).
+   By default, Git determines the command line arguments to use
+   based on the basename of the configured SSH command (configured
+   using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or
+   the config setting `core.sshCommand`). If the basename is
+   unrecognized, Git will attempt to detect support of OpenSSH
+   options by first invoking the configured SSH command with the
+   `-G` (print configuration) option and will subsequently use
+   OpenSSH options (if that is successful) or no options besides
+   the host and remote command (if it fails).
 +
-The config variable `ssh.variant` can be set to override this auto-detection;
-valid values are `ssh`, `simple`, `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`.
+The config variable `ssh.variant` can be set to override this detection.
+Valid values are `ssh` (to use OpenSSH options), `plink

[PATCH 7/8] ssh: 'simple' variant does not support -4/-6

[Jonathan Nieder]

If the user passes -4/--ipv4 or -6/--ipv6 to "git fetch" or "git push"
and the ssh command configured with GIT_SSH does not support such a
setting, error out instead of ignoring the option and continuing.

Signed-off-by: Jonathan Nieder 
Acked-by: Stefan Beller 
---
As before.

 connect.c| 25 ++---
 t/t5601-clone.sh | 12 ++--
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/connect.c b/connect.c
index 455c54a2ec..be106d1868 100644
--- a/connect.c
+++ b/connect.c
@@ -938,11 +938,30 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
 get_protocol_version_config());
}
 
-   if (variant != VARIANT_SIMPLE) {
-   if (flags & CONNECT_IPV4)
+   if (flags & CONNECT_IPV4) {
+   switch (variant) {
+   case VARIANT_AUTO:
+   BUG("VARIANT_AUTO passed to push_ssh_options");
+   case VARIANT_SIMPLE:
+   die("ssh variant 'simple' does not support -4");
+   case VARIANT_SSH:
+   case VARIANT_PLINK:
+   case VARIANT_PUTTY:
+   case VARIANT_TORTOISEPLINK:
argv_array_push(args, "-4");
-   else if (flags & CONNECT_IPV6)
+   }
+   } else if (flags & CONNECT_IPV6) {
+   switch (variant) {
+   case VARIANT_AUTO:
+   BUG("VARIANT_AUTO passed to push_ssh_options");
+   case VARIANT_SIMPLE:
+   die("ssh variant 'simple' does not support -6");
+   case VARIANT_SSH:
+   case VARIANT_PLINK:
+   case VARIANT_PUTTY:
+   case VARIANT_TORTOISEPLINK:
argv_array_push(args, "-6");
+   }
}
 
if (variant == VARIANT_TORTOISEPLINK)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 209e2d5604..ad910ae9fa 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -364,9 +364,10 @@ test_expect_success 'OpenSSH variant passes -4' '
expect_ssh "-4 -p 123" myhost src
 '
 
-test_expect_success 'variant can be overriden' '
-   git -c ssh.variant=simple clone -4 "[myhost:123]:src" ssh-simple-clone 
&&
-   expect_ssh myhost src
+test_expect_success 'variant can be overridden' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/putty" &&
+   git -c ssh.variant=putty clone -4 "[myhost:123]:src" ssh-putty-clone &&
+   expect_ssh "-4 -P 123" myhost src
 '
 
 test_expect_success 'variant=auto picks based on basename' '
@@ -375,10 +376,9 @@ test_expect_success 'variant=auto picks based on basename' 
'
expect_ssh "-4 -P 123" myhost src
 '
 
-test_expect_success 'simple is treated as simple' '
+test_expect_success 'simple does not support -4/-6' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
-   git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple &&
-   expect_ssh myhost src
+   test_must_fail git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple
 '
 
 test_expect_success 'uplink is treated as simple' '
-- 
2.15.0.448.gf294e3d99a

[PATCH 8/8] ssh: 'simple' variant does not support --port

[Jonathan Nieder]

When trying to connect to an ssh:// URL with port explicitly specified
and the ssh command configured with GIT_SSH does not support such a
setting, it is less confusing to error out than to silently suppress
the port setting and continue.

This requires updating the GIT_SSH setting in t5603-clone-dirname.sh.
That test is about the directory name produced when cloning various
URLs.  It uses an ssh wrapper that ignores all its arguments but does
not declare that it supports a port argument; update it to set
GIT_SSH_VARIANT=ssh to do so.  (Real-life ssh wrappers that pass a
port argument to OpenSSH would also support -G and would not require
such an update.)

Reported-by: William Yan 
Signed-off-by: Jonathan Nieder 
Acked-by: Stefan Beller 
---
As before.

That's the end of the series.  Thanks for reading.

 connect.c| 15 ---
 t/t5601-clone.sh | 10 --
 t/t5603-clone-dirname.sh |  2 ++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/connect.c b/connect.c
index be106d1868..20ed1d9574 100644
--- a/connect.c
+++ b/connect.c
@@ -967,11 +967,20 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
if (variant == VARIANT_TORTOISEPLINK)
argv_array_push(args, "-batch");
 
-   if (port && variant != VARIANT_SIMPLE) {
-   if (variant == VARIANT_SSH)
+   if (port) {
+   switch (variant) {
+   case VARIANT_AUTO:
+   BUG("VARIANT_AUTO passed to push_ssh_options");
+   case VARIANT_SIMPLE:
+   die("ssh variant 'simple' does not support setting 
port");
+   case VARIANT_SSH:
argv_array_push(args, "-p");
-   else
+   break;
+   case VARIANT_PLINK:
+   case VARIANT_PUTTY:
+   case VARIANT_TORTOISEPLINK:
argv_array_push(args, "-P");
+   }
 
argv_array_push(args, port);
}
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index ad910ae9fa..78dab81d87 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -378,12 +378,18 @@ test_expect_success 'variant=auto picks based on 
basename' '
 
 test_expect_success 'simple does not support -4/-6' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
-   test_must_fail git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple
+   test_must_fail git clone -4 "myhost:src" ssh-4-clone-simple
+'
+
+test_expect_success 'simple does not support port' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
+   test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-simple
 '
 
 test_expect_success 'uplink is treated as simple' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
-   git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
+   test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
+   git clone "myhost:src" ssh-clone-uplink &&
expect_ssh myhost src
 '
 
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index d5af758129..13b5e5eb9b 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -11,7 +11,9 @@ test_expect_success 'setup ssh wrapper' '
git upload-pack "$TRASH_DIRECTORY"
EOF
GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
+   GIT_SSH_VARIANT=ssh &&
export GIT_SSH &&
+   export GIT_SSH_VARIANT &&
export TRASH_DIRECTORY
 '
 
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 3/8] connect: split git:// setup into a separate function

[Jonathan Nieder]

Brandon Williams wrote:
> On 11/20, Jonathan Nieder wrote:

>> +/* These underlying connection commands die() if they
>> + * cannot connect.
>> + */
>
> I know this is really just code motion but maybe we can fix the style of
> the comment here?

How about doing that as a separate commit?

-- >8 --
Subject: connect: correct style of C-style comment

Documentation/CodingGuidelines explains:

 - Multi-line comments include their delimiters on separate lines from
   the text.  E.g.

/*
 * A very long
 * multi-line comment.
 */

Reported-by: Brandon Williams 
Signed-off-by: Jonathan Nieder 
---
 connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git i/connect.c w/connect.c
index 20ed1d9574..e544a5e1dd 100644
--- i/connect.c
+++ w/connect.c
@@ -889,7 +889,8 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
 
transport_check_allowed("git");
 
-   /* These underlying connection commands die() if they
+   /*
+* These underlying connection commands die() if they
 * cannot connect.
 */
if (git_use_proxy(hostandport))

Re: [PATCH 4/8] connect: split ssh command line options into separate function

[Jonathan Nieder]

Hi,

Brandon Williams wrote:
> On 11/20, Jonathan Nieder wrote:
[long stream of quoted context snipped; please cut down the quoted
 text to what you are replying to in the future]
>> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const 
>> char *url,
>>  conn->use_shell = 1;
>>  conn->in = conn->out = -1;
>>  if (protocol == PROTO_SSH) {
>> -const char *ssh;
>> -enum ssh_variant variant;
>>  char *ssh_host = hostandport;
>>  const char *port = NULL;
>> +
>>  transport_check_allowed("ssh");
>>  get_host_and_port(&ssh_host, &port);
>> -
>>  if (!port)
>>  port = get_port(ssh_host);
>> -
>
> Are these random additions and deletions intentional?

Sorry about that.  It was to make the code easier to read, but I can
see how it's jarring during review.  I can resend without the removed
blank lines if you like.

For context, here's the code in question after the current patch:

if (protocol == PROTO_SSH) {
char *ssh_host = hostandport;
const char *port = NULL;

transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
if (!port)
port = get_port(ssh_host);
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : 
"NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");

free(hostandport);
free(path);
free(conn);
strbuf_release(&cmd);
return NULL;
}
fill_ssh_args(conn, ssh_host, port, flags);
} else {
transport_check_allowed("file");
if (get_protocol_version_config() > 0) {
argv_array_pushf(&conn->env_array, 
GIT_PROTOCOL_ENVIRONMENT "=version=%d",
 get_protocol_version_config());
}
}

Jonathan

[PATCH v4 4/8] connect: split ssh command line options into separate function

[Jonathan Nieder]

The git_connect function is growing long.  Split the portion that
discovers an ssh command and options it accepts before the service
name and path to a separate function to make it easier to read.

No functional change intended.

Signed-off-by: Jonathan Nieder 
Reviewed-by: Stefan Beller 
---
Brandon Williams wrote:
> On 11/20, Jonathan Nieder wrote:

>> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const 
>> char *url,
>>  conn->use_shell = 1;
>>  conn->in = conn->out = -1;
>>  if (protocol == PROTO_SSH) {
>> -const char *ssh;
>> -enum ssh_variant variant;
>>  char *ssh_host = hostandport;
>>  const char *port = NULL;
>> +
>>  transport_check_allowed("ssh");
>>  get_host_and_port(&ssh_host, &port);
>> -
>>  if (!port)
>>  port = get_port(ssh_host);
>> -
>
> Are these random additions and deletions intentional?

Thanks again for noticing this.  After looking more closely, I don't
see any reason for these whitespace changes.  Here's a corrected
patch.

 connect.c | 113 +-
 1 file changed, 60 insertions(+), 53 deletions(-)

diff --git a/connect.c b/connect.c
index 9425229206..2113feb4f8 100644
--- a/connect.c
+++ b/connect.c
@@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
return conn;
 }
 
+/* Prepare a child_process for use by Git's SSH-tunneled transport. */
+static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
+ const char *port, int flags)
+{
+   const char *ssh;
+   enum ssh_variant variant;
+
+   if (looks_like_command_line_option(ssh_host))
+   die("strange hostname '%s' blocked", ssh_host);
+
+   ssh = get_ssh_command();
+   if (ssh) {
+   variant = determine_ssh_variant(ssh, 1);
+   } else {
+   /*
+* GIT_SSH is the no-shell version of
+* GIT_SSH_COMMAND (and must remain so for
+* historical compatibility).
+*/
+   conn->use_shell = 0;
+
+   ssh = getenv("GIT_SSH");
+   if (!ssh)
+   ssh = "ssh";
+   variant = determine_ssh_variant(ssh, 0);
+   }
+
+   argv_array_push(&conn->args, ssh);
+
+   if (variant == VARIANT_SSH &&
+   get_protocol_version_config() > 0) {
+   argv_array_push(&conn->args, "-o");
+   argv_array_push(&conn->args, "SendEnv=" 
GIT_PROTOCOL_ENVIRONMENT);
+   argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT 
"=version=%d",
+get_protocol_version_config());
+   }
+
+   if (variant != VARIANT_SIMPLE) {
+   if (flags & CONNECT_IPV4)
+   argv_array_push(&conn->args, "-4");
+   else if (flags & CONNECT_IPV6)
+   argv_array_push(&conn->args, "-6");
+   }
+
+   if (variant == VARIANT_TORTOISEPLINK)
+   argv_array_push(&conn->args, "-batch");
+
+   if (port && variant != VARIANT_SIMPLE) {
+   if (variant == VARIANT_SSH)
+   argv_array_push(&conn->args, "-p");
+   else
+   argv_array_push(&conn->args, "-P");
+
+   argv_array_push(&conn->args, port);
+   }
+
+   argv_array_push(&conn->args, ssh_host);
+}
+
 /*
  * This returns the dummy child_process `no_fork` if the transport protocol
  * does not need fork(2), or a struct child_process object if it does.  Once
@@ -972,8 +1031,6 @@ struct child_process *git_connect(int fd[2], const char 
*url,
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
-   const char *ssh;
-   enum ssh_variant variant;
char *ssh_host = hostandport;
const char *port = NULL;
transport_check_allowed("ssh");
@@ -995,57 +1052,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
strbuf_release(&cmd);
return NULL;
}
-
-   if (looks_like_command_line_option(ssh_host))
-   die("strange host

Re: Draft of Git Rev News edition 33

[Jonathan Nieder]

Hi,

Yubin Ruan wrote:
> 2017-11-20 16:33 GMT+08:00 Christian Couder :

>> A draft of a new Git Rev News edition is available here:
>>
>>   
>> https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-33.md
>>
>> Everyone is welcome to contribute in any section either by editing the
>> above page on GitHub and sending a pull request, or by commenting on
>> this GitHub issue:
>>
>>   https://github.com/git/git.github.io/issues/262
>>
>> You can also reply to this email.
[...]
> For the CRLF issue: I am not really that concerned with this issue
> because I mostly develop all my project on Linux. But can't git treat
> CRLF and LF just as the same thing? That will solves the problem
> gracefully.

Do you mean that it should ignore line ending changes?  What line
endings should a user get when they "git clone" or "git checkout"?

That said, I believe that the gitattributes(5) manpage does an okay
job of covering this and that that thread came to a clear conclusion:

  
https://public-inbox.org/git/20171026203046.fu3z5ngnw7hck...@aiede.mtv.corp.google.com/

I commented at [1] that I found the conclusion of the rev news entry
misleading and confusing but it doesn't appear that there is anything
I can do about that.  It's disappointing because if there is something
that was not covered in that thread, then it would be good to revive
it so we can improve the program's behavior or docs, but I wasn't able
to find out what was missed.

Thanks,
Jonathan

[1] https://github.com/git/git.github.io/issues/262#issuecomment-345804038

Re: [PATCH] list-objects-filter-options: fix up some sparse warnings

[Jonathan Nieder]

Hi,

Ramsay Jones wrote:

> If you need to re-roll your 'jh/object-filtering' branch, could you
> please squash this (or something like it) into the relevant patch
> (commit bf0aedcbe1, "list-objects: filter objects in traverse_commit_list",
> 16-11-2017).

Micronit: can these messages use the ISO 8601 order 2017-11-16?

That way, the date can be read without confusion regardless of what
locale the reader is in.

Thanks,
Jonathan

[PATCH 1/8 v2] ssh test: make copy_ssh_wrapper_as clean up after itself

[Jonathan Nieder]

Simplify by not allowing the copied ssh wrapper to persist between
tests.  This way, tests can be safely reordered, added, and removed
with less fear of hidden side effects.

This also avoids having to call setup_ssh_wrapper to restore the value
of GIT_SSH after this battery of tests, since it means each test will
restore it individually.

Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash
will overwrite that when redirecting via `>uplink`.  A proposed test
wrote a script to 'uplink' after a previous test created uplink.exe
using copy_ssh_wrapper_as, so the script written with '>uplink' had
the wrong filename and failed.

Reported-by: Johannes Schindelin 
Signed-off-by: Jonathan Nieder 
Acked-by: Brandon Williams 
---
Jonathan Nieder wrote:

> Thanks to Dscho for tracking this subtle issue down.
>
>  t/t5601-clone.sh | 28 
>  1 file changed, 12 insertions(+), 16 deletions(-)
[...]
> +++ b/t/t5601-clone.sh
> @@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
[...]
>  copy_ssh_wrapper_as () {
>   cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
> + test_when_finished "rm -f ${1%$X}$X" &&

I noticed while looking this over again that it is not exactly right.
$1 is likely to include spaces, causing the "rm" command not to delete
anything.  Removing the "-f" confirms the bug.

A small tweak fixes it:

 diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh
 index 9d007c0f8d..4a16a0b7dd 100755
 --- i/t/t5601-clone.sh
 +++ w/t/t5601-clone.sh
 @@ -317,7 +317,7 @@ test_expect_success 'set up ssh wrapper' '
  
  copy_ssh_wrapper_as () {
cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
 -  test_when_finished "rm -f ${1%$X}$X" &&
 +  test_when_finished "rm $(git rev-parse --sq-quote "${1%$X}$X")" &&
GIT_SSH="${1%$X}$X" &&
test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""
  }

 t/t5601-clone.sh | 28 
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 86811a0c35..4a16a0b7dd 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
test_cmp fetch.expected fetch.actual
 '
 
-setup_ssh_wrapper () {
-   test_expect_success 'setup ssh wrapper' '
-   cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
-   "$TRASH_DIRECTORY/ssh$X" &&
-   GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
-   export GIT_SSH &&
-   export TRASH_DIRECTORY &&
-   >"$TRASH_DIRECTORY"/ssh-output
-   '
-}
+test_expect_success 'set up ssh wrapper' '
+   cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
+   "$TRASH_DIRECTORY/ssh$X" &&
+   GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
+   export GIT_SSH &&
+   export TRASH_DIRECTORY &&
+   >"$TRASH_DIRECTORY"/ssh-output
+'
 
 copy_ssh_wrapper_as () {
cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
+   test_when_finished "rm $(git rev-parse --sq-quote "${1%$X}$X")" &&
GIT_SSH="${1%$X}$X" &&
-   export GIT_SSH
+   test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""
 }
 
 expect_ssh () {
@@ -344,8 +343,6 @@ expect_ssh () {
(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
 }
 
-setup_ssh_wrapper
-
 test_expect_success 'clone myhost:src uses ssh' '
git clone myhost:src ssh-clone &&
expect_ssh myhost src
@@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink 
detection' '
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/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' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
GIT_SSH_VARIANT=tortoiseplink \
git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
expect_ssh "-batch -P 123" myhost src
@@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '
git clone "[myhost:123]:src" sq-failure
 '
 
-# Reset the GIT_SSH environment variable for clone tests.
-setup_ssh_wrapper
-
 counter=0
 # $1 url
 # $2 none|host
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:

>> Android's "repo" tool is a tool for managing a large codebase
>> consisting of multiple smaller repositories, similar to Git's
>> submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
>> 'simple' ssh variant, 2017-10-16), users noticed that it stopped
>> handling the port in ssh:// URLs.
>>
>> ...
>> Reported-by: William Yan 
>> Improved-by: Jonathan Tan 
>> Signed-off-by: Jonathan Nieder 
>> ---
>
> Not a big issue, but the above made me wonder, due to lack of any
> signed-off-by before improved-by, what "base" was improved by JTan.
> If you were writing a change before formally passing it around with
> your sign-off and somebody had a valuable input to improve it, it
> seems that people say helped-by around here.

Yep, I should have put the Improved-by after my sign-off.

Jonathan Tan's contribution was at
https://public-inbox.org/git/20171023151929.67165aea67353e5c24a15...@google.com/

[...]
>> +The config variable `ssh.variant` can be set to override this detection.
>> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
>> +`tortoiseplink`, `simple` (no options except the host and remote command).
>> +The default auto-detection can be explicitly requested using the value
>> +`auto`.  Any other value is treated as `ssh`.  This setting can also be
>> +overridden via the environment variable `GIT_SSH_VARIANT`.
>>  +
>
> Cleanly written and easily read.  Good.

i.e. that part is all thanks to him. :)

Jonathan

Re: pedantry: is there a standard for what should be in the SYNOPSIS?

[Jonathan Nieder]

Hi,

Robert P. J. Day wrote:

> following up on an earlier question of mine, is there a standard for
> what options should be listed in either the SYNOPSIS or the
> DESCRIPTION sections of a man page? i ask since i'm seeing some
> definite inconsistency.

No standard.  Seems worth starting a discussion on what you'd like the
standard to be.

Jonathan

Re: [PATCH] notes: fix erroneous "git notes prune [-n | -v]" message

[Jonathan Nieder]

Hi,

Robert P. J. Day wrote:

> It seems clear that the man page SYNPOSIS and the usage message
> referring to:
>
>   git notes prune [-n | -v]
>
> is incorrect, as "-n" (dry run) and "-v" (verbose) are not
> alternatives, so fix both places to refer to:
>
>   git notes prune [-n] [-v | -q]
>
> to match the rest of the man page.
>
> Signed-off-by: Robert P. J. Day 
> ---

Hm.  What does "git notes prune -q" do?

The docs describe what --dry-run and --verbose do for "git notes prune"
but its description of --quiet seems to be specific to "git notes
merge".  Should the usage summary be

 git notes [--ref=] prune [-n] [-v]

instead?  That would also be consistent with "git notes prune -h":

  $ git notes prune -h
  usage: git notes prune []

  -n, --dry-run do not remove, show only
  -v, --verbose report pruned notes

Thanks,
Jonathan

Re: [PATCH] notes: fix erroneous "git notes prune [-n | -v]" message

[Jonathan Nieder]

Robert P. J. Day wrote:

> so it should simply be corrected to:
>
>   git notes prune [-n] [-v]
>
> sound about right?

Sounds good to me.

Thanks for finding these confusing docs, by the way.

Jonathan

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Jonathan Nieder]

Hi,

Christian Couder wrote:

> The function calls itself "required", but it does not die when it
> sees an unexpected EOF.
> Let's rename it to "packet_key_val_read()".
>
> Signed-off-by: Christian Couder 
> ---

nit: please wrap lines to a consistent width, to make the message
easier to read.  In the above, it looks like the line break is
intentional --- is it meant to be two paragraphs (i.e. is it missing
another newline)?

optional, just noticed while I'm nitpicking: the description 'rename
packet_required_key_val_read' doesn't tell why the function is being
renamed.  Maybe something like

Git::Packet: clarify that packet_required_key_val_read allows EOF

would do the trick.

[...]
> +++ b/perl/Git/Packet.pm
> @@ -83,7 +83,8 @@ sub packet_txt_read {
>   return ( $res, $buf );
>  }
>  
> -sub packet_required_key_val_read {
> +# Read a text line and check that it is in the form "key=value"
> +sub packet_key_val_read {

This comment doesn't tell me how to use the function.  How do I detect
whether it successfully read a line?  What do the return values
represent?  What happens if the line it read doesn't match the key?

Thanks,
Jonathan

Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'

[Jonathan Nieder]

Hi,

Christian Couder wrote:

> The code is more understandable with 'if' instead of 'unless'.
>
> Signed-off-by: Christian Couder 
> ---
>  perl/Git/Packet.pm | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

I'm agnostic about that.  In some ways it seems like an odd change,
since it is changing from

if (exceptional case) {
die "explanation";
}
common case;

to

if (!exceptional case) {
common case;
}
die "explanation";

when we usually prefer the former (getting the exceptional case over
with early so the reader can concentrate on the common case).  In
perl, it might be even more idiomatic to do

die "explanation" if exceptional case;
common case;

but Documentation/CodingGuidelines appears to recommend not going that
far. :)

[...]
> --- a/perl/Git/Packet.pm
> +++ b/perl/Git/Packet.pm
> @@ -68,16 +68,16 @@ sub packet_bin_read {
>  
>  sub remove_final_lf_or_die {
>   my $buf = shift;
> - unless ( $buf =~ s/\n$// ) {

For readability, I find this whitespace within parens more distracting.

Documentation/CodingGuidelines covers that for C programs but not for
Perl.  Do we have a preferred style either way about it?

Thanks,
Jonathan

Re: [PATCH] notes: correct 'git notes prune' options to '[-n] [-v]'

[Jonathan Nieder]

Robert P. J. Day wrote:

> Currently, 'git notes prune' in man page and usage message
> incorrectly lists options as '[-n | -v]', rather than '[-n] [-v]'.
>
> Signed-off-by: Robert P. J. Day 
> ---

Reviewed-by: Jonathan Nieder 
Thanks.

> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 43677297f..e8dec1b3c 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -18,7 +18,7 @@ SYNOPSIS
>  'git notes' merge --commit [-v | -q]
>  'git notes' merge --abort [-v | -q]
>  'git notes' remove [--ignore-missing] [--stdin] [...]
> -'git notes' prune [-n | -v]
> +'git notes' prune [-n] [-v]
>  'git notes' get-ref
> 
> 
> diff --git a/builtin/notes.c b/builtin/notes.c
> index d7754db14..892e37a03 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -33,7 +33,7 @@ static const char * const git_notes_usage[] = {
>   N_("git notes merge --commit [-v | -q]"),
>   N_("git notes merge --abort [-v | -q]"),
>   N_("git notes [--ref ] remove [...]"),
> - N_("git notes [--ref ] prune [-n | -v]"),
> + N_("git notes [--ref ] prune [-n] [-v]"),
>   N_("git notes [--ref ] get-ref"),
>   NULL
>  };
> 

Re: some apparent inaccuracies in "man git-worktree"

[Jonathan Nieder]

Hi,

Robert P. J. Day wrote:
> On Tue, 14 Nov 2017, Eric Sunshine wrote:
>> On Tue, Nov 14, 2017 at 3:43 AM, Robert P. J. Day  
>> wrote:

>>> from "man git-worktree", there seem to be some inaccuracies in the
>>> SYNOPSIS regarding the "add" subcommand:
>>>
>>>   git worktree add \
>>> [-f] [--detach] [--checkout] [--lock] [-b ]  
>>> []
>>>
>>>   first, there's no mention of "-B" in that SYNOPSIS, even though it's
>>> explained further down the man page.
>>
>> Omission of "-B" from the synopsis was intentional. From cbdf60fa18
>> (worktree: add -b/-B options, 2015-07-06):
[...]
>> Whether or not the omission was actually a good decision is
>> questionable.
[...]
>>>   next, the SYNOPSIS seems misleading as it doesn't make clear that
>>> the options -b, -B and --detach are mutually exclusive, which is made
>>> clear in the worktree.c source:
[...]
>> Failure to update the synopsis to indicate mutual exclusion appears to
>> be a simple oversight in ab0b2c53ed (worktree: make --detach mutually
>> exclusive with -b/-B, 2015-07-17)
[...]
>>>   finally (and maybe i'm just not reading carefully enough), it's not
>>> clear what happens if you add a worktree at a given commit without
>>> specifying *any* of -b, -B or --detach. the obvious result should be a
>>> new worktree checked out at a detached HEAD and, interestingly, if i
>>> do that, then from the main tree, i see:
>>>
>>>   $ git worktree list
>>>   /home/rpjday/k/git   516fb7f2e73d [master]
>>>   /home/rpjday/k/temp  c470abd4fde4 (detached HEAD)
>>>   $
>>>
>>> but from within the worktree, if i ask for the status, i see only:
>>>
>>>   $ git status
>>>   Not currently on any branch.
>>>   nothing to commit, working tree clean
>>>   $
>>>
>>> where i would normally have expected to see "detached HEAD", is there
>>> a reason that's not displayed?
>>
>> Someone more familiar with this bit can correct me if I'm wrong, but I
>> believe that the "HEAD detached at/from " you normally see
>> with 'git status' is derived from the reflog,
[...]
>   i'm not sure what i can add to this, but i'm going to leave it to
> folks higher up the food chain than me to resolve any of the above.

For what it's worth, the folks higher up the food chain you're
referring to don't exist, so the most likely outcome here is that
nothing happens.  People working on patches (suggesting wording,
formatting as a patch, reviewing, testing, etc) are as high on the
food chain as it gets. :)

One thing we've discussed on this list before is setting up a bug
tracker to allow people another way to collaborate (filing a clear
summary of a problem for interested people to work on).  More on that
subject later today --- feel free to poke me if I don't get such a
message out.

Thanks,
Jonathan

Re: [PATCH] recursive submodules: detach HEAD from new state

[Jonathan Nieder]

Stefan Beller wrote:
>> Junio C Hamano  writes:

>>> Also, while I do agree with you that the problem exists, it is
>>> unclear why this patch is a solution and not a hack that sweeps a
>>> problem under the rug.
>>>
>>> It is unclear why this "silently detach HEAD without telling the
>>> user" is a better solution than erroring out, for example [*1*].
[...]
> So I took a step back and wrote about different proposals where
> we want to go long term. See below. This will help us
> figuring out how to approach this bug correctly.

Stefan, do you know what thread I should look at to find the current
state of this patch?  I've had it applied locally for a long time.

The thread I am replying to appears to be where the patch comes from
but I have some memories of more recent discussion that I'm not
finding.

More context:
https://public-inbox.org/git/xmqqshd8i3ze@gitster.mtv.corp.google.com/
says

 * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits
  - Documentation/checkout: clarify submodule HEADs to be detached
  - recursive submodules: detach HEAD from new state

  "git checkout --recursive" may overwrite and rewind the history of
  the branch that happens to be checked out in submodule
  repositories, which might not be desirable.  Detach the HEAD but
  still allow the recursive checkout to succeed in such a case.

  Expecting a reroll.

Thanks,
Jonathan

Re: [PATCH] recursive submodules: detach HEAD from new state

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder  wrote:

>> Stefan, do you know what thread I should look at to find the current
>> state of this patch?  I've had it applied locally for a long time.
>
> It was "Undecided" for some time, then Junio kicked it back to pu, expecting a
> reroll[1]. The "send out a plan" that was referenced is found in [2]
> describing 6 plans for the future of submodules. The approach in [3]
> which is different on the implementation level, but very similar on
> the UX level sounds best currently.  I'll coordinate with JTan to
> come up with patches for that.
>
> [1] 
> https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=a...@mail.gmail.com/
> [2] https://public-inbox.org/git/20171109001007.11894-1-sbel...@google.com/
> [3] 
> https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b...@google.com/

Thanks.  That thread appears to be about a long-term plan; what is the
short-term plan?

E.g. is there any additional documentation that should be added to the
patch that detaches?

Or should it go in as-is?

Thanks,
Jonathan

Re: doc: prefer 'stash push' over 'stash save'

[Jonathan Nieder]

Phil Hord wrote:

> Although `git stash save` was deprecated recently, some parts of the
> documentation still refer to it instead of `push`.
>
> Signed-off-by: Phil Hord 
> ---
>  Documentation/git-stash.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Jonathan Nieder 
Thanks.

Re: stash: learn to parse -m/--message like commit does

[Jonathan Nieder]

Hi,

Phil Hord wrote:

> `git stash push -m foo` uses "foo" as the message for the stash. But
> `git stash push -m"foo"` does not parse successfully.  Similarly
> `git stash push --message="My stash message"` also fails.  Nothing
> in the documentation suggests this syntax should work,

"git help cli" says it should work.  Thanks for working on it.

>but it does
> work for `git commit`, and my fingers have learned this pattern long
> ago.

[PATCH 0/3] Improving build reproducibility

[Jonathan Nieder]

Hi,

The reproducible builds <https://reproducible-builds.org/> project has
been working on making it possible to verify that binary packages of
open source projects were built from the source they were claimed to
have been built from.

To that end, Debian has been carrying patches 1-2 for a while.  Patch
3 is a Google-internal patch with a related but distinct goal of
making builds go faster.

I think these should be ready to apply.  Thoughts of all kinds welcome.

Sincerely,
Anders Kaseorg (2):
  Documentation: allow overriding timestamps of generated asciidoc
  git-gui: sort entries in optimized tclIndex

Jonathan Nieder (1):
  generate-cmdlist: avoid non-deterministic output

 Documentation/Makefile   | 7 +--
 Documentation/technical/api-index.sh | 5 +
 generate-cmdlist.sh  | 2 +-
 git-gui/Makefile | 2 +-
 4 files changed, 12 insertions(+), 4 deletions(-)

[PATCH 1/3] Documentation: allow overriding timestamps of generated asciidoc

[Jonathan Nieder]

From: Anders Kaseorg 
Date: Wed, 30 Nov 2016 22:21:15 -0500

Allow overriding the timestamp in generated documentation by setting
SOURCE_DATE_EPOCH to the number of seconds since 1970-01-01 00:00:00
UTC to use.

This makes the generated documentation reproducible from the source
code as long as that variable is set, without losing the last-modified
dates in the default build.

With this change, the package passes Debian's build reproducibility
test (https://wiki.debian.org/ReproducibleBuilds/TimestampsProposal).

The goal is to make it easier to verify that binary packages of open
source projects were built from the source they were claimed to have
been built from.  https://reproducible-builds.org/ has more details.

Signed-off-by: Anders Kaseorg 
Signed-off-by: Jonathan Nieder 
---
Perhaps this should e.g. be taking the latest timestamp of all its
inputs.  That would be straightforward to do, but what's here is what
we've been running with for the past year, so I'd rather stick to it,
at least as a starting point.

Another tweak I'd be interested in is allowing asciidoc to take the
timestamp as a parameter instead of inferring it from mtimes.
Asciidoc accepts an "--attribute footer-style=none" parameter, but I'm
not aware of an "--attribute footer-date=" parameter to keep the
footer but override its date.

 Documentation/Makefile   | 7 +--
 Documentation/technical/api-index.sh | 5 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 2ab65561af..dfec29f36f 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -410,6 +410,7 @@ $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
 howto-index.txt: howto-index.sh $(wildcard howto/*.txt)
$(QUIET_GEN)$(RM) $@+ $@ && \
'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(wildcard howto/*.txt)) 
>$@+ && \
+   $(if $(SOURCE_DATE_EPOCH),touch -d '@$(SOURCE_DATE_EPOCH)' $@+ &&) \
mv $@+ $@
 
 $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
@@ -420,8 +421,10 @@ WEBDOC_DEST = /pub/software/scm/git/docs
 howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
 $(patsubst %.txt,%.html,$(wildcard howto/*.txt)): %.html : %.txt
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-   sed -e '1,/^$$/d' $< | \
-   $(TXT_TO_HTML) - >$@+ && \
+   sed -e '1,/^$$/d' $< > $<+ && \
+   $(if $(SOURCE_DATE_EPOCH),touch -d '@$(SOURCE_DATE_EPOCH)' $<+ &&) \
+   $(TXT_TO_HTML) -o $@+ $<+ && \
+   rm $<+ && \
mv $@+ $@
 
 install-webdoc : html
diff --git a/Documentation/technical/api-index.sh 
b/Documentation/technical/api-index.sh
index 9c3f4131b8..07b3909627 100755
--- a/Documentation/technical/api-index.sh
+++ b/Documentation/technical/api-index.sh
@@ -20,6 +20,11 @@
sed -n -e '/^\/\/ table of contents end/,$p' "$skel"
 ) >api-index.txt+
 
+if test "${SOURCE_DATE_EPOCH:+set}"
+then
+   touch -d "@$SOURCE_DATE_EPOCH" api-index.txt+
+fi
+
 if test -f api-index.txt && cmp api-index.txt api-index.txt+ >/dev/null
 then
rm -f api-index.txt+
-- 
2.15.0.448.gf294e3d99a

[PATCH 2/3] git-gui: sort entries in optimized tclIndex

[Jonathan Nieder]

From: Anders Kaseorg 
Date: Wed, 16 Nov 2016 16:37:17 -0500

auto_mkindex expands wildcards in directory order, which depends on
the underlying filesystem.  To improve build reproducibility, sort the
list of *.tcl files in the Makefile.

The unoptimized loading case was previously fixed in gitgui-0.21.0~14
(git-gui: sort entries in tclIndex, 2015-01-26).

Signed-off-by: Anders Kaseorg 
Signed-off-by: Jonathan Nieder 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 918a8de369..f10caedaa7 100644
--- a/Makefile
+++ b/Makefile
@@ -254,7 +254,7 @@ $(ALL_MSGFILES): %.msg : %.po
 lib/tclIndex: $(ALL_LIBFILES) GIT-GUI-VARS
$(QUIET_INDEX)if echo \
  $(foreach p,$(PRELOAD_FILES),source $p\;) \
- auto_mkindex lib '*.tcl' \
+ auto_mkindex lib $(patsubst lib/%,%,$(sort $(ALL_LIBFILES))) \
| $(TCL_PATH) $(QUIET_2DEVNULL); then : ok; \
else \
 echo >&2 "* $(TCL_PATH) failed; using unoptimized loading"; \
-- 
2.15.0.448.gf294e3d99a

[PATCH 3/3] generate-cmdlist: avoid non-deterministic output

[Jonathan Nieder]

Date: Fri, 1 Jul 2016 17:32:00 -0700

Non-determinism makes it harder for build tools to discover when a
target needs to be rebuilt.

generate-cmdlist.sh stores the full path in a comment:

 /* Automatically generated by /build/git-agojiD/git-2.15.0/generate-cmdlist.sh 
*/

Use the file name alone instead.

Signed-off-by: Jonathan Nieder 
---
That's the end of the series.  Thanks for reading.

 generate-cmdlist.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index ab0d1b0c06..eeea4b67ea 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-echo "/* Automatically generated by $0 */
+echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
char name[16];
char help[80];
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 1/3] Documentation: allow overriding timestamps of generated asciidoc

[Jonathan Nieder]

Hi,

brian m. carlson wrote:
> On Tue, Nov 21, 2017 at 03:34:32PM -0800, Jonathan Nieder wrote:

>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -410,6 +410,7 @@ $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
>>  howto-index.txt: howto-index.sh $(wildcard howto/*.txt)
>>  $(QUIET_GEN)$(RM) $@+ $@ && \
>>  '$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(wildcard howto/*.txt)) 
>> >$@+ && \
>> +$(if $(SOURCE_DATE_EPOCH),touch -d '@$(SOURCE_DATE_EPOCH)' $@+ &&) \
>
> touch -d @SECONDS isn't POSIX compliant, and non-Linux systems don't
> provide it.  POSIX only allows certain fixed format, and I assume that
> non-Linux parties (maybe OpenBSD) will want to have reproducible builds
> as well.

Interesting.  My knee-jerk preference is still to go with this patch
as-is for now, since the non-portability only triggers when
SOURCE_DATE_EPOCH is set.

> It's unfortunate for shell users that this variable is in seconds from
> the epoch, since there's no portable way to format such a time in shell.
> (POSIX doesn't allow date(1) to format anything but the current time.)
>
> My proposed solution was to use Perl to do so, and simply require that
> if you wanted a reproducible build, then you had to have Perl.  That
> would, of course, require a separate variable in the Makefile holding
> the formatted date.
>
> Maybe something like the following in the Makefile:
>
> ifndef NO_PERL
> SOURCE_DATE_TIMESTAMP=$(shell perl -MPOSIX -e 'print strftime("%FT%TZ", 
> gmtime($ENV{SOURCE_DATE_EPOCH}));')
> endif
>
> and then:
>
> + $(if $(SOURCE_DATE_TIMESTAMP),touch -d '$(SOURCE_DATE_TIMESTAMP)' $@+ 
> &&) \

Neat.  I can play with this a little.

http://asciidoc.org/CHANGELOG.html is stale but asciidoc still seems
to be getting changes at https://github.com/asciidoc/asciidoc.  I
wonder how difficult it would be to add any required SOURCE_DATE_EPOCH
support there.

Longer term, I wonder what it would take to move to a markup language
that is more widely known, like commonmark.

Thanks,
Jonathan

Re: Draft of Git Rev News edition 33

[Jonathan Nieder]

Hi,

Christian Couder wrote:
> On Tue, Nov 21, 2017 at 2:10 AM, Jonathan Nieder  wrote:

>> That said, I believe that the gitattributes(5) manpage does an okay
>> job of covering this and that that thread came to a clear conclusion:
>>
>>   
>> https://public-inbox.org/git/20171026203046.fu3z5ngnw7hck...@aiede.mtv.corp.google.com/
>>
>> I commented at [1] that I found the conclusion of the rev news entry
>> misleading and confusing but it doesn't appear that there is anything
>> I can do about that.
>
> Well, you could have provided a pull request or otherwise suggested
> what you think would be a better conclusion for the article and why.
>
> If you just say that the above email is the conclusion, when it seems
> to me that another email from someone else is also a conclusion with a
> quite different outcome, it does not help much come to an agreement
> about what should be reported as the conclusion of the thread.

This is something I suspect journalists have to deal with all the
time: when one of the subjects of an article feels misrepresented
(which happens inevitably when writing to a deadline), that comes with
a feeling of powerlessness that can lead to grumpiness and harsh
words.

In the end you ended up improving the text enough that I don't mind
any more.  Sorry for the bumpy road along the way.

Thanks,
Jonathan

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:
>> Christian Couder wrote:

>>> +# Read a text line and check that it is in the form "key=value"
>>> +sub packet_key_val_read {
>>
>> This comment doesn't tell me how to use the function.  How do I detect
>> whether it successfully read a line?  What do the return values
>> represent?  What happens if the line it read doesn't match the key?
>
> Would this work for both of you?
>
> # Read a text packet, expecting that it is in the form "key=value" for
> # the given $key.  An EOF does not trigger any error and is reported
> # back to the caller (like packet_txt_read() does).  Die if the "key"
> # part of "key=value" does not match the given $key, or the value part
> # is empty.

Yes, thank you.

Jonathan

Re: [PATCH 1/3] Documentation: allow overriding timestamps of generated asciidoc

[Jonathan Nieder]

Anders Kaseorg wrote:
> On Tue, 21 Nov 2017, Jonathan Nieder wrote:

>> http://asciidoc.org/CHANGELOG.html is stale but asciidoc still seems
>> to be getting changes at https://github.com/asciidoc/asciidoc.  I
>> wonder how difficult it would be to add any required SOURCE_DATE_EPOCH
>> support there.
>
> In fact I already did (https://github.com/asciidoc/asciidoc/pull/106),
> which is why I’d been holding off on trying to upstream this Git patch.
> The trouble was, the AsciiDoc developers had not been cutting new releases
> “because nobody knows how”
> (https://github.com/asciidoc/asciidoc/issues/103#issuecomment-322077321).
> However, it looks like AsciiDoc 8.6.10 was recently tagged and Debian got
> a 8.6.10-1 package yesterday, so I guess that trouble has been quietly
> resolved.

Ah, lovely.  I'll add a build-time dependency on that version to the
Debian package.

Junio, please disregard this patch (patch 1/3).

Thanks,
Jonathan

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:
>> Junio C Hamano wrote:
>>> Jonathan Nieder  writes:

>>>> This comment doesn't tell me how to use the function.  How do I detect
>>>> whether it successfully read a line?  What do the return values
>>>> represent?  What happens if the line it read doesn't match the key?
>>>
>>> Would this work for both of you?
>>>
>>> # Read a text packet, expecting that it is in the form "key=value" for
>>> # the given $key.  An EOF does not trigger any error and is reported
>>> # back to the caller (like packet_txt_read() does).  Die if the "key"
>>> # part of "key=value" does not match the given $key, or the value part
>>> # is empty.
>>
>> Yes, thank you.
>
> Heh.  I actually was expecting a different response: "that describes
> what the reader can easily read out of the implementation and is
> useless", though.

The main context that I'm missing and that this function comment
doesn't answer is what this function is for.  When would I want to
read a line and exit if it doesn't match "key" but not exit if I hit
EOF?  It seems very strange.

The function comment does successfully capture the strangeness,
though, and that already helps.  When looking at the implementation, I
had a bit of a double-take and wondered what I was missing.  This doc
comment says "you weren't missing anything --- that is actually the
contract that this function intends to fulfill".

Thanks,
Jonathan

Re: [PATCH][l10n-fr] list translated to prune in command

[Jonathan Nieder]

Hi,

Louis Bettens wrote:

> "$ git worktree" when in a french locale shows an incorrect usage
> summary.  This comes down to this trivial issue in the i18n.

Good catch.  This comes from v2.7.0-rc3~4^2~7^2~2^2 (l10n: fr v2.7.0
round 1 (2477t), 2015-12-18).

For next time, you can send these three emails in a single email:

Subject: l10n: fr.po: "worktree list" mistranslated as prune

"$ git worktree" when in a french locale shows an incorrect usage
summary.  This comes down to this trivial issue in the i18n.

Signed-off-by: ...
---
Also it seems this is the only .po that has this particular quirk:

$ grep -c "worktree prune" po/*.po
po/bg.po:2
po/ca.po:2
po/de.po:2
po/es.po:2
po/fr.po:3  # outlier
po/is.po:0
po/it.po:0
po/ko.po:2
po/pt_PT.po:2
po/ru.po:2
po/sv.po:2
po/vi.po:2
po/zh_CN.po:2

zero lines -> translation missing, OK
two lines -> msgid and msgstr, OK
three lines -> something wrong. In this case, the present issue.

 po/fr.po | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/po/fr.po b/po/fr.po
index 4deae3318..a12a2ae37 100644
--- a/po/fr.po
+++ b/po/fr.po
[...]

That way, Jean-Noel Avila or Jiang Xin can apply the patch with a
single "git am" command and the explanation (above the "---" line)
shows up in "git log".

I assume they can handle this one fine as-is.  Just pointing it out
for the future.

See the DISCUSSION section in "git help format-patch" for more details.

Thanks,
Jonathan

Re: [PATCH v5 5/6] rev-list: add list-objects filtering support

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:

> Teach rev-list to use the filtering provided by the
> traverse_commit_list_filtered() interface to omit
> unwanted objects from the result.
>
> Object filtering is only allowed when one of the "--objects*"
> options are used.

micronit: the line widths seem to be uneven in these commit messages,
which is a bit distracting when reading.

> When the "--filter-print-omitted" option is used, the omitted
> objects are printed at the end.  These are marked with a "~".
> This option can be combined with "--quiet" to get a list of
> just the omitted objects.

Neat.  Can you give a quick example?

Using --quiet for this feels a bit odd, since it previously meant
to print nothing to stdout.  I wonder if there's another way ---
e.g.

--print-omitted=(yes|no|only)

If I wanted to list all objects matching a filter, even objects
that are not reachable from any ref, is there a way to do that?
(Just curious, trying to think about this interface.)

> Add t6112 test.

This part doesn't need to be in the commit message.  More generally,
anything I could more easily learn from the code or diffstat doesn't
need to be in the commit message: the commit message is about the
"why" more than the details of what in the code changed.

> In the future, we will introduce a "partial clone" mechanism
> wherein an object in a repo, obtained from a remote, may
> reference a missing object that can be dynamically fetched from
> that remote once needed.  This "partial clone" mechanism will
> have a way, sometimes slow, of determining if a missing link
> is one of the links expected to be produced by this mechanism.

Does this mean the s will be part of the wire protocol?
I'll look more carefully at them below with that in mind.

> This patch introduces handling of missing objects to help
> debugging and development of the "partial clone" mechanism,
> and once the mechanism is implemented, for a power user to
> perform operations that are missing-object aware without
> incurring the cost of checking if a missing link is expected.

I had trouble understanding what this paragraph is about.  Can you
give an example?

> Signed-off-by: Jeff Hostetler 
> ---
>  Documentation/git-rev-list.txt  |   4 +-
>  Documentation/rev-list-options.txt  |  36 ++
>  builtin/rev-list.c  | 108 -
>  t/t6112-rev-list-filters-objects.sh | 225 
> 
>  4 files changed, 370 insertions(+), 3 deletions(-)
>  create mode 100755 t/t6112-rev-list-filters-objects.sh

Looks reasonably concise, good.

[...]
> --- a/Documentation/git-rev-list.txt
> +++ b/Documentation/git-rev-list.txt
> @@ -47,7 +47,9 @@ SYNOPSIS
>[ --fixed-strings | -F ]
>[ --date=]
>[ [ --objects | --objects-edge | --objects-edge-aggressive ]
> -[ --unpacked ] ]
> +[ --unpacked ]
> +[ --filter= [ --filter-print-omitted ] ] ]

Does this mean --filter is only useful with --objects?  E.g. I can't
use it to filter commits?

> +  [ --missing= ]

--missing=(error|allow-any|print) would be more informative and about
equally concise.

Since this is mainly for debugging, does it have a different
compatibility guarantee from other options?  Could it be named
accordingly to set expectations?

[...]
> +The form '--filter=blob:none' omits all blobs.

Sounds sensible.

> ++
> +The form '--filter=blob:limit=[kmg]' omits blobs larger than n bytes
> +or units.  The value may be zero.

On second thought, doesn't blob:limit=0 mean blob:none is not needed?
Is it for future consistency with tree:none?

What units do [kmg] use? Are they GB, GiB, or one of the variants in
between?

> ++
> +The form '--filter=sparse:oid=' uses a sparse-checkout
> +specification contained in the object (or the object that the expression
> +evaluates to) to omit blobs that would not be not required for a
> +sparse checkout on the requested refs.

This one makes me a little nervous because it would mean we're
planning on adding sparse-checkout specifications to the wire
protocol.  Maybe that's okay --- they're already part of the on-disk
format --- but it makes me nervous because the sparse-checkout format
is not so great, as I believe MS has already noticed.

What is an ?  Can it just say ?  How would this one
work when passed over the wire?

> ++
> +The form '--filter=sparse:path=' similarly uses a sparse-checkout
> +specification contained in .

Is this  relative to the cwd of the caller, or is it within some
commit?

Sorry it took so long to send this feedback / these questions.
Hopefully it's useful nevertheless.

Thanks and hope that helps,
Jonathan

Re: [PATCH 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Add LIBPCRE1 and LIBPCRE2 prerequisites which are true when git is
> compiled with USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease,
> respectively.
>
> The syntax of PCRE1 and PCRE2 isn't the same in all cases (see
> pcresyntax(3) and pcre2syntax(3)). If test are added that test for
> those they'll need to be guarded by these new prerequisites.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  t/README  | 12 
>  t/test-lib.sh |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/t/README b/t/README
> index 4b079e4494..599cd9808c 100644
> --- a/t/README
> +++ b/t/README
> @@ -808,6 +808,18 @@ use these, and "test_set_prereq" for how to define your 
> own.
> Git was compiled with support for PCRE. Wrap any tests
> that use git-grep --perl-regexp or git-grep -P in these.
>  
> + - LIBPCRE1
> +
> +   Git was compiled with PCRE v1 support via
> +   USE_LIBPCRE1=YesPlease. Wrap any PCRE using tests that for some
> +   reason need v1 of the PCRE library instead of v2 in these.

Are there plans to use the LIBPCRE1 prereq?  It might be simpler to
only have LIBPCRE2, and LIBPCRE1 can still be expressed as

PCRE,!LIBPCRE2

which I think is clearer about the intent.

Thanks,
Jonathan

Re: git status always modifies index?

[Jonathan Nieder]

Hi,

Nathan Neulinger wrote[1]:

> I just got an answer to my stackoverflow question on this,
> apparently it's already implemented:
>
> https://stackoverflow.com/questions/47436939/how-to-run-git-status-without-modifying-git-index-such-as-in-a-prompt-command
>
> There is a "--no-optional-locks" command in 2.15 that looks like it
> does exactly what I need.

I was about to point to
https://public-inbox.org/git/20170921043214.pyhdsrpy4omy5...@sigill.intra.peff.net/
about exactly this thing. :)

That said, I wonder if this use case is an illustration that a name
like --no-lock-index (as was used in Git for Windows when this feature
first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at
coming up with option names) would make the feature easier to
discover.

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/dfbf4af3-e87c-bdcb-7544-685572925...@neulinger.org/

Re: git status always modifies index?

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Nov 22, 2017 at 12:27:20PM -0800, Jonathan Nieder wrote:

>> That said, I wonder if this use case is an illustration that a name
>> like --no-lock-index (as was used in Git for Windows when this feature
>> first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at
>> coming up with option names) would make the feature easier to
>> discover.
[...]
> Or maybe just living with the minor philosophical rough edges,
> since it seems OK in practice.

To be clear, my concern is not philosophical but practical: I'm saying
if it's a "git status" option (or at least shows up in the "git
status" manpage) and it is memorably about $GIT_DIR/index (at least
mentions that in its description) then it is more likely to help
people.

Thanks,
Jonathan

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -386,6 +386,9 @@ For C programs:
>   - Use Git's gettext wrappers to make the user interface
> translatable. See "Marking strings for translation" in po/README.
>  
> + - Prefer the BUG() macro over asserts, as asserts requires that the
> +   NDEBUG flag is unset on compilation to work.

nit: is there some logical place in the list of C guidelines this
should go at, instead of the last item?  Maybe near the top, since
this is one of those straightforward cases and we're just saying that
this is the startegy for asserting invariants that this project
prefers.

Separately: I am not sure we currently universally prefer BUG_ON()
over assert().  In theory, assert() is fine wherever you don't care
whether the assertion is checked at run-time --- in other words, it is
a fancy form of comment.  BUG_ON() is useful for defensive checks that
you *expect* to never trip but want to rely on afterwards.

In a certain ideal world, the preference would be reversed: you'd want
to use assert() wherever you can and require the compiler to check
that all assert()s are verifiable at compile time.  A check that a
static analyzer can verify is more valuable than a run-time check.
When a compile-time check is not possible, you'd have to fall back to
BUG_ON().

But we don't live in that ideal world.  I'm not aware of any widely
available tools for checking assert()s.  So maybe it makes sense to
forbid assert() in our codebase entirely.

That could be enforced using something like check-non-portable-shell.pl,
except

- run on C files instead of sh files
- run on Git's source code instead of t/

What do you think?

Thanks,
Jonathan

Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, 
> const char *buf, size_t size,
>  __attribute__((format (printf, 3, 4))) NORETURN
>  void BUG_fl(const char *file, int line, const char *fmt, ...);
>  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
> +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while 
> (0)
>  #else
>  __attribute__((format (printf, 1, 2))) NORETURN
>  void BUG(const char *fmt, ...);
> +
> +__attribute__((format (printf, 2, 3)))
> +void BUG_ON(int condition, const char *fmt, ...);
>  #endif

I worry that these definitions are mildly incompatible: the macro
accepts anything that can go in an 'if', including pointers, and the
function only accepts an int.

Is there a way for the macro to typecheck that its argument is an
integer to avoid that?

Thanks,
Jonathan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:

>> On reviewing [1] I wondered why there are so many asserts and wondered
>> if these asserts could have been prevented by a better functionality around
>> bug reporting in our code.
>>
>> Introduce a BUG_ON macro, which is superior to assert() by
>>  * being always there, even when compiled with NDEBUG and
>>  * providind an additional human readable error message, like BUG()
>
> I'm not sure I agree with the aim of the series.
>
> If people want to compile with NDEBUG, that's their business, I guess.
> I don't see much _point_ in it for Git, since most of our assertions do
> not respect NDEBUG, and I don't think we tend to assert in expensive
> ways anyway.
>
> I do like human readable messages. But sometimes such a message just
> makes the code harder to read (and to write). E.g., is there any real
> value in:
>
>   BUG_ON(!foo, "called bar() with a foo!");
>
> over:
>
>   assert(foo);

I think you're hinting at wanting

BUG_ON(!foo);

which is something that the Linux kernel has (and which is not done in
this series).

[...]
> I also find (as your third patch switches):
>
>   if (!foo)
>   BUG("foo has not been setup");
>
> more readable than the BUG_ON() version, if only because it uses
> traditional control flow.

Yes, I think you're right.

Thanks,
Jonathan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jonathan Nieder]

Jeff King wrote:

> Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> I'm not sure what it's buying us over assert().

It lets you build with NDEBUG.  It also goes through our own die()
handler, which means that e.g. the error message gets propagated over
remote transports.

Please please please, don't rely on side-effects from assert().  They
will cause me to run into pain over time.  This issue alone might be
worth banning use of assert() in the codebase, if we can't trust
reviewers to catch problematic examples (fortunately reviewers have
been pretty good about that so far, but banning it would free up their
attention to focus on other aspects of a patch).

Jonathan

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

[Jonathan Nieder]

Stefan Beller wrote:
> On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder  wrote:

>> In a certain ideal world, the preference would be reversed: you'd want
>> to use assert() wherever you can and require the compiler to check
>> that all assert()s are verifiable at compile time.  A check that a
>> static analyzer can verify is more valuable than a run-time check.
>> When a compile-time check is not possible, you'd have to fall back to
>> BUG_ON().
>
> Linux has BUILT_BUG_ON as well, which we may desire?

I thought so until I tried to use it:
https://public-inbox.org/git/20170830065631.gh153...@aiede.mtv.corp.google.com/

So I think we are stuck with run-time checking for now.

Thanks,
Jonathan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

>> It lets you build with NDEBUG.
>
> But why do you want to build with NDEBUG if nothing uses assert()? ;)

No idea, but some distros (not Debian) have done it before and I don't
want to be burned again.

> I'm being a little glib, but I think there really is a chicken-and-egg
> question here. Why are people building with NDEBUG if they don't want to
> disable asserts?

It's because they want to disable asserts.

The semantics of assert is that they're allowed to be compiled out.  A
person setting NDEBUG is perfectly within their rights to assume that
the author of a program has followed those semantics.  Of course this
makes assert a terrible API from the point of view of Rusty Russel's
API rating scheme
<http://sweng.the-davies.net/Home/rustys-api-design-manifesto> but
it's what C gives us.

FWIW I think we've done fine at using assert so far.  But if I
understand correctly, the point of this series is to stop having to
worry about it.

[...]
>> It also goes through our own die()
>> handler, which means that e.g. the error message gets propagated over
>> remote transports.
>
> BUG() doesn't go through our die handler. It hits vreportf(), which does
> do some minor munging, but it always goes to stderr. Error message
> propagation over the remote protocol happens either:
>
>   1. From a parent process muxing our stderr onto the sideband.
>
>   2. Via special wrappers like receive-pack's rp_error().
>
> The only program I know that sets a custom die handler to change the
> reporting is git-http-backend (and there it only applies to die("BUG"),
> not BUG(), so with the latter you'd get a hard hangup instead of an
> attempt at an http 500).

Ah, interesting.  That could be worth changing, though I don't think
it's too important.

[...]
> Yes, obviously side effects in an assert() are horrible. But I haven't
> noticed that being a problem in our code base.
>
> For the record, I'm totally fine with banning assert() in favor of a
> custom equivalent. I just don't think we've seen any real problems with
> assert in our codebase so far.

It sounds like we basically agree, then. :)

Jonathan

[ANNOUNCE] Bug tracker for Git

[Jonathan Nieder]

Hi,

As discussed at [1], I've occasionally wanted to have a place to keep
track of bugs I'm working on in Git.  Some kind people on the Chromium
project helped me set an issue tracker up, so now we have one.

 https://crbug.com/git

Feel free to file bugs, feature requests, and leftover bits there.
I'll be happy to triage them to keep the list of bugs meaningful.
Anyone else wanting to help with bug management can feel free to
contact me and I'll grant you permissions to edit issues.

This particular implementation of an issue tracker is Monorail
.
It is similar to the issue tracker that used to run at
code.google.com.  If you find things you don't like about the way it
works, they accept patches. :)

It accepts replies to the emails it sends.

This uses Google accounts to authenticate people filing a bug.  If you
would like a Google account for your existing email address, you can
get one at https://accounts.google.com/SignUpWithoutGmail.

There is an API for programmatic access to the issue tracker: [2].
One thing I would appreciate help with is using that API to make
regular backups.  The chromium project has their own backups using
database dumps but I would be happier if multiple people are making
backups independently.

This is an experiment --- I don't know whether it will stick.  The
configuration will likely change as we get experience with it.  I
expect to be using it to track my own work for the forseeable future.
I'd be happy if it is useful to others as well.

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/20170919160753.ga75...@aiede.mtv.corp.google.com/
https://public-inbox.org/git/vpqiovpubmh@anie.imag.fr/
https://public-inbox.org/git/7vhay9tqs6@alter.siamese.dyndns.org/
https://public-inbox.org/git/7vehzjugdz@alter.siamese.dyndns.org/
https://public-inbox.org/git/4a1fb1de.3070...@op5.se/
[2] 
https://chromium.googlesource.com/infra/infra/+/master/appengine/monorail/doc/api.md

Re: git status always modifies index?

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Mon, 27 Nov 2017, Jeff King wrote:

>> [...] IMHO it argues for GfW trying to land patches upstream first, and
>> then having them trickle in as you merge upstream releases.
>
> You know that I tried that, and you know why I do not do that anymore: it
> simply takes too long, and the review on the list focuses on things I
> cannot focus on as much, I need to make sure that the patches *work*
> first, whereas the patch review on the Git mailing list tends to ensure
> that they have the proper form first.
>
> I upstream patches when I have time.

You have been developing in the open, so no complaints from me, just a
second point of reference:

For Google's internal use we sometimes have needed a patch faster than
upstream can review it.  Our approach in those cases has been to send
a patch to the mailing list and then apply it internally immediately.
If upstream is stalled for months on review, so be it --- we already
have the patch.  But this tends to help ensure that we are moving in
the same direction.

That said, I don't think that was the main issue with
--no-optional-locks.  I'll comment more on that in another subthread.

Thanks,
Jonathan

Re: git status always modifies index?

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Sun, Nov 26, 2017 at 06:35:56PM +0900, Junio C Hamano wrote:

>> Having a large picture option like "--read-only" instead of ending
>> up with dozens of "we implemented a knob to tweak only this little
>> piece, and here is an option to trigger it" would help users in the
>> long run, but we traditionally did not do so because we tend to
>> avoid shipping "incomplete" features, but being perfectionist with
>> such a large undertaking can stall topics with feature bloat.  In a
>> case like this, however, I suspect that an aspirational feature that
>> starts small, promises little and can be extended over time may be a
>> good way to move forward.
>
> I actually consider "--no-optional-locks" to be such an aspirational
> feature. I didn't go digging for other cases (though I'm fairly certain
> that "diff" has one), but hoped to leave it for further bug reports ("I
> used the option, ran command X, and saw lock contention").

I am worried that the project is not learning from what happened here.

My main issue with the --no-optional-locks name is that it does not
connect to the underlying user need.  Your main argument for it is
that it exactly describes the underlying user need.  One of us has to
be wrong.

So let me describe my naive reading:

As a user, I want to inspect the state of the repository without
disrupting it in any way.  That means not breaking concurrent
processes and not upsetting permissions.  --read-only seems to
describe this use case to me perfectly.

If I understood correctly, your objection is that --read-only is not
specific enough.  What I really want, you might say, is not to break
concurrent processes.  Any other aspects of being read-only are not
relevant.  E.g. if I can refresh the on-disk index using O_APPEND
without disrupting concurrent processes then I should be satisfied
with that.

Fair enough, though that feels like overengineering.  But I *still*
don't see what that has to do with the name "no-optional-locks".  When
is a lock *optional*?  And how am I supposed to discover this option?

This also came up during review, and I am worried that this review
feedback is being ignored.  In other words, I have no reason to
believe it won't happen again.

> I would be fine with having a further aspirational "read only" mode.

Excellent, we seem to agree on this much.  If I can find time for it
today then I'll write a patch.

Thanks,
Jonathan

Re: [PATCH on en/rename-progress] diffcore-rename: make diff-tree -l0 mean -l

[Jonathan Nieder]

Elijah Newren wrote:
> On Wed, Nov 29, 2017 at 10:32 AM, Jonathan Tan  
> wrote:

>> In the documentation of diff-tree, it is stated that the -l option
>> "prevents rename/copy detection from running if the number of
>> rename/copy targets exceeds the specified number". The documentation
>> does not mention any special handling for the number 0, but the
>> implementation before commit b520abf ("sequencer: warn when internal
>> merge may be suboptimal due to renameLimit", 2017-11-14) treated 0 as a
>> special value indicating that the rename limit is to be a very large
>> number instead.
>>
>> The commit b520abf changed that behavior, treating 0 as 0. Revert this
>> behavior to what it was previously. This allows existing scripts and
>> tools that use "-l0" to continue working. The alternative (to allow
>> "-l0") is probably much less useful, since users can just refrain from

I think in the parenthesis you mean 'to allow "-l0" to suppress rename
detection', since this patch is all about allowing '-l0' already.

>> specifying -M and/or -C to have the same effect.
>>
>> Signed-off-by: Jonathan Tan 
>> ---
>> Note that this patch is built on en/rename-progress.
>>
>> We noticed this through an automated test for an internal tool - the
>> tool uses git diff-tree with -l0, and no longer produces the same
>> results as before.
>
> Thanks for testing that version and sending along the fix.
>
> I suspect the commit referenced twice in the commit message should
> have been 9f7e4bfa3b ("diff: remove silent clamp of renameLimit",
> 2017-11-13) rather than b520abf ("sequencer: warn when internal merge
> may be suboptimal due to renameLimit", 2017-11-14).
>
> Other than that minor issue, patch and test looks good to me.

Thanks, both.  Looking at that patch, the fix is obviously correct.

With Elijah's commit message tweak,
Reviewed-by: Jonathan Nieder 

Re: [PATCH on en/rename-progress v2] diffcore-rename: make diff-tree -l0 mean -l

[Jonathan Nieder]

Jonathan Tan wrote:

> In the documentation of diff-tree, it is stated that the -l option
> "prevents rename/copy detection from running if the number of
> rename/copy targets exceeds the specified number". The documentation
> does not mention any special handling for the number 0, but the
> implementation before commit 9f7e4bfa3b ("diff: remove silent clamp of
> renameLimit", 2017-11-13) treated 0 as a special value indicating that
> the rename limit is to be a very large number instead.
>
> The commit 9f7e4bfa3b changed that behavior, treating 0 as 0. Revert
> this behavior to what it was previously. This allows existing scripts
> and tools that use "-l0" to continue working. The alternative (to have
> "-l0" suppress rename detection) is probably much less useful, since
> users can just refrain from specifying -M and/or -C to have the same
> effect.
>
> Signed-off-by: Jonathan Tan 
> ---
> v2 is exactly the same as previously, except that the commit message is
> changed following Elijah Newren's and Jonathan Nieder's comments.
>
>  diffcore-rename.c  |  2 ++
>  t/t4001-diff-rename.sh | 15 +++
>  2 files changed, 17 insertions(+)

Reviewed-by: Jonathan Nieder 

Thanks again.

> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 9ca0eaec7..245e999fe 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -392,6 +392,8 @@ static int too_many_rename_candidates(int num_create,
>*
>*num_create * num_src > rename_limit * rename_limit
>*/
> + if (rename_limit <= 0)
> + rename_limit = 32767;
>   if ((num_create <= rename_limit || num_src <= rename_limit) &&
>   ((uint64_t)num_create * (uint64_t)num_src
><= (uint64_t)rename_limit * (uint64_t)rename_limit))
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index 0d1fa45d2..eadf4f624 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -230,4 +230,19 @@ test_expect_success 'rename pretty print common prefix 
> and suffix overlap' '
>   test_i18ngrep " d/f/{ => f}/e " output
>  '
>  
> +test_expect_success 'diff-tree -l0 defaults to a big rename limit, not zero' 
> '
> + test_write_lines line1 line2 line3 >myfile &&
> + git add myfile &&
> + git commit -m x &&
> +
> + test_write_lines line1 line2 line4 >myotherfile &&
> + git rm myfile &&
> + git add myotherfile &&
> + git commit -m x &&
> +
> + git diff-tree -M -l0 HEAD HEAD^ >actual &&
> + # Verify that a rename from myotherfile to myfile was detected
> + grep "myotherfile.*myfile" actual
> +'
> +
>  test_done

Re: [PATCH] hashmap: adjust documentation to reflect reality

[Jonathan Nieder]

Johannes Schindelin wrote:

> The hashmap API is just complicated enough that even at least one
> long-time Git contributor has to look up how to use it every time he
> finds a new use case. When that happens, it is really useful if the
> provided example code is correct...
>
> While at it, "fix a memory leak", avoid statements before variable
> declarations, fix a const -> no-const cast, several %l specifiers (which
> want to be %ld), avoid using an undefined constant, call scanf()
> correctly, use FLEX_ALLOC_STR() where appropriate, and adjust the style
> here and there.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  hashmap.h | 60 +---
>  1 file changed, 29 insertions(+), 31 deletions(-)

Yay, thanks for this.

Reviewed-by: Jonathan Nieder 

Follow-on idea for the interested: would making a test that extracts
this sample code from hashmap.h and confirms it still works be a crazy
idea?

Re: imap-send with gmail: curl_easy_perform() failed: URL using bad/illegal format or missing URL

[Jonathan Nieder]

(+cc: Nicolas)
Hi,

Doron Behar wrote:

> I'm trying to send a patch with the command `git imap-send`, I used the
> examples in the manual page as the main reference for my configuration:
>
> ```
> [imap]
>   folder = "[Gmail]/Drafts"
>   host = imaps://imap.gmail.com
>   user = doron.be...@gmail.com
>   port = 993
>   sslverify = false
> ```
>
> This is my `cat patch.out | git imap-send` output:
>
> ```
> Password for 'imaps://doron.be...@gmail.com@imap.gmail.com':
> sending 3 messages
> curl_easy_perform() failed: URL using bad/illegal format or missing URL
> ```

Thanks for reporting this.  I suspect this is related to
v2.15.0-rc0~63^2 (imap-send: use curl by default when possible,
2017-09-14) --- e.g. perhaps our custom IMAP code was doing some
escaping on the username that libcurl does not do.

"man git imap-send" says this is a recommended configuration, so I
don't think it's a configuration error.

What platform are you on?  What version of libcurl are you using?

In libcurl::lib/easy.c I am also seeing

if(mcode)
  return CURLE_URL_MALFORMAT; /* TODO: return a proper error! */

which looks suspicious.

Nicolas, am I on the right track?

Thanks,
Jonathan

> The URI doesn't seem OK to me, I tried using `imap.user = doron.behar` and the
> URI was `imaps://doron.be...@imap.gmail.com` but that ended up with the same
> error as in the previous case.
>
> I would love to get some help here, a Google Search didn't help as well.
>
> Thanks.

Re: [PATCH] Makefile: replace perl/Makefile.PL with simple make rules

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Replace the perl/Makefile.PL and the fallback perl/Makefile used under
> NO_PERL_MAKEMAKER=NoThanks with a much simpler implementation heavily
> inspired by how the i18n infrastructure's build process works[1].

Yay!  This looks exciting.

One quick comment:

[...]
>  * We don't build the Git(3) Git::I18N(3) etc. man pages from the
>embedded perldoc. I suspect nobody really cares, these are mostly
>internal APIs, and if someone's developing against them they likely
>know enough to issue a "perldoc" against the installed file to get
>the same result.
>
>But this is a change in how Git is installed now on e.g. CentOS &
>Debian which carry these manpages. They could be added (via
>pod2man) if anyone really cares.
>
>I doubt they will. The reason these were built in the first place
>was as a side-effect of how ExtUtils::MakeMaker works.

Debian cares (see
https://www.debian.org/doc/packaging-manuals/perl-policy/ch-module_packages.html
for details).

I'll try applying this patch and seeing what happens some time this
week.

Thanks,
Jonathan

Re: How hard would it be to implement sparse fetching/pulling?

[Jonathan Nieder]

Hi Vitaly,

Vitaly Arbuzov wrote:

> Found some details here: https://github.com/jeffhostetler/git/pull/3
>
> Looking at commits I see that you've done a lot of work already,
> including packing, filtering, fetching, cloning etc.
> What are some areas that aren't complete yet? Do you need any help
> with implementation?

That's a great question!  I've filed https://crbug.com/git/2 to track
this project.  Feel free to star it to get updates there, or to add
updates of your own.

As described at https://crbug.com/git/2#c1, currently there are three
patch series for which review would be very welcome.  Building on top
of them is welcome as well.  Please make sure to coordinate with
jeffh...@microsoft.com and jonathanta...@google.com (e.g. through the
bug tracker or email).

One piece of missing functionality that looks intereseting to me: that
series batches fetches of the missing blobs involved in a "git
checkout" command:

 https://public-inbox.org/git/20171121211528.21891-14-...@jeffhostetler.com/

But if doesn't batch fetches of the missing blobs involved in a "git
diff  " command.  That might be a good place to get
your hands dirty. :)

Thanks,
Jonathan

Re: [PATCH] Makefile: replace perl/Makefile.PL with simple make rules

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Thu, Nov 30 2017, Jonathan Nieder jotted:
>> Ævar Arnfjörð Bjarmason wrote:

>>>  * We don't build the Git(3) Git::I18N(3) etc. man pages from the
>>>embedded perldoc. I suspect nobody really cares, these are mostly
>>>internal APIs, and if someone's developing against them they likely
>>>know enough to issue a "perldoc" against the installed file to get
>>>the same result.
[...]
>> Debian cares (see
>> https://www.debian.org/doc/packaging-manuals/perl-policy/ch-module_packages.html
>> for details).
[...]
> It just says you have to install the manpages in such-and-such a place,
> but I don't have any. There, policy issue fixed :)
>
> More seriously, it seems to me that the only reason we have these
> manpages in the first place is because of emergent effects. *Maybe* I'm
> wrong about someone using Git.pm as an external API, is that the case?

Yeah, people really do use Git.pm as an external API.

Unlike e.g. something on CPAN, its API stability guarantees are not
great, so I am not saying I recommend it, but people have been using
it that way.

If we want to prevent this, then we should not be installing it in the
public perl module path.  Or we should at least add a note to the
manpages we ship :) to recommend not using it.

Thanks,
Jonathan

Re: How hard would it be to implement sparse fetching/pulling?

[Jonathan Nieder]

Hi Vitaly,

Vitaly Arbuzov wrote:

> I think it would be great if we high level agree on desired user
> experience, so let me put a few possible use cases here.

I think one thing this thread is pointing to is a lack of overview
documentation about how the 'partial clone' series currently works.
The basic components are:

 1. extending git protocol to (1) allow fetching only a subset of the
objects reachable from the commits being fetched and (2) later,
going back and fetching the objects that were left out.

We've also discussed some other protocol changes, e.g. to allow
obtaining the sizes of un-fetched objects without fetching the
objects themselves

 2. extending git's on-disk format to allow having some objects not be
present but only be "promised" to be obtainable from a remote
repository.  When running a command that requires those objects,
the user can choose to have it either (a) error out ("airplane
mode") or (b) fetch the required objects.

It is still possible to work fully locally in such a repo, make
changes, get useful results out of "git fsck", etc.  It is kind of
similar to the existing "shallow clone" feature, except that there
is a more straightforward way to obtain objects that are outside
the "shallow" clone when needed on demand.

 3. improving everyday commands to require fewer objects.  For
example, if I run "git log -p", then I way to see the history of
most files but I don't necessarily want to download large binary
files just to print 'Binary files differ' for them.

And by the same token, we might want to have a mode for commands
like "git log -p" to default to restricting to a particular
directory, instead of downloading files outside that directory.

There are some fundamental changes to make in this category ---
e.g. modifying the index format to not require entries for files
outside the sparse checkout, to avoid having to download the
trees for them.

The overall goal is to make git scale better.

The existing patches do (1) and (2), though it is possible to do more
in those categories. :)  We have plans to work on (3) as well.

These are overall changes that happen at a fairly low level in git.
They mostly don't require changes command-by-command.

Thanks,
Jonathan

Re: [PATCH 2/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Jonathan Nieder]

Hi,

Todd Zullinger wrote:

> Previously, setting SVNSERVE_PORT enabled several tests which require a
> local svnserve daemon to be run (in t9113 & t9126).  The tests share the
> setup of the local svnserve via `start_svnserve()`.  The function uses
> the svnserve option `--listen-once` which causes svnserve to accept one
> connection on the port, serve it, and exit.  When running the tests in
> parallel this fails if one test tries to start svnserve while the other
> is still running.

I had trouble reading this because I didn't know what previous time it
was referring to.  Is it about how the option currently behaves?

(Git's commit messages tend to use the present tense to describe the
behavior before the patch, like a bug report, and the imperative to
describe the change the patch proposes to make, like an impolite bug
report. :))

> Use the test number as the svnserve port (similar to httpd tests) to
> avoid port conflicts.  Set GIT_TEST_SVNSERVE to any value other than
> 'false' or 'auto' to enable these tests.

This uses imperative in two ways and also ended up confusing me.  The
second one is a direction to me, not Git, right?  How about:

Use the test number instead of $SVNSERVE_PORT as the svnserve
port (similar to httpd tests) to avoid port conflicts.
Developers can set GIT_TEST_SVNSERVE to any value other than
'false' or 'auto' to enable these tests.
>
> Signed-off-by: Todd Zullinger 
> ---
>  t/lib-git-svn.sh | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

The patch looks good.  Thanks.

> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
> index 84366b2624..4c1f81f167 100644
> --- a/t/lib-git-svn.sh
> +++ b/t/lib-git-svn.sh
> @@ -110,14 +110,16 @@ EOF
>  }
>  
>  require_svnserve () {
> - if test -z "$SVNSERVE_PORT"
> + test_tristate GIT_TEST_SVNSERVE
> + if ! test "$GIT_TEST_SVNSERVE" = true
>   then
> - skip_all='skipping svnserve test. (set $SVNSERVE_PORT to 
> enable)'
> + skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to 
> enable)'
>   test_done
>   fi
>  }
>  
>  start_svnserve () {
> + SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
>   svnserve --listen-port $SVNSERVE_PORT \
>--root "$rawsvnrepo" \
>--listen-once \
> -- 
> 2.15.1
> 

Re: [PATCH 1/2] t/lib-git-svn: whitespace cleanup

[Jonathan Nieder]

Todd Zullinger wrote:

> Subject: t/lib-git-svn: whitespace cleanup
>
> Signed-off-by: Todd Zullinger 
> ---
>  t/lib-git-svn.sh | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Jonathan Nieder 
Thanks.

nit: it would have been a tiny bit easier to review if the commit
message mentioned that this is only changing the indentation from an
inconsistent space/tab mixture to tabs and isn't making any other
changes.

Re: [PATCH 0/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Jonathan Nieder]

Todd Zullinger wrote:

> These tests are not run by default nor are they enabled in travis-ci.  I
> don't know how much testing they get in user or other packager builds.
>
> I've been slowly increasing the test suite usage in fedora builds.  I
> ran into this while testing locally with parallel make test.  The
> official fedora builds don't run in parallel (yet), as even before I ran
> into this issue, builds on the fedora builders randomly failed too
> often.  I'm hoping to eventually enable parallel tests by default
> though, since it's so much faster.

This background could go in the commit message for patch 2, but it
also speaks for itself as an obviously good change so I could go
either way.

> I'm not sure if there's any objection to changing the variable needed to
> enable the tests from SVNSERVE_PORT to GIT_TEST_SVNSERVE.  The way
> SVNSERVE_PORT is set in this patch should allow the port to be set
> explicitly, in case someone requires that -- and they understand that it
> can fail if running parallel tests, of course.  Whether that's a
> feature or a bug, I'm not sure. :)

micronit: can this just say something like

Patch 2 is the important one --- see that one for rationale.

Patch 1 is an optional preparatory style cleanup.

next time?  That way, you get an automatic guarantee that all the
important information is available in "git log" output to people who
need it later.

Thanks,
Jonathan

Re: [PATCH 2/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Jonathan Nieder]

Todd Zullinger wrote:

> Much better, thank you.  How about this for the full commit message:
>
>t/lib-git-svn.sh: improve svnserve tests with parallel make test
>
>Setting SVNSERVE_PORT enables several tests which require a local
>svnserve daemon to be run (in t9113 & t9126).  The tests share setup of
>the local svnserve via `start_svnserve()`.  The function uses svnserve's
>`--listen-once` option, which causes svnserve to accept one connection
>on the port, serve it, and exit.  When running the tests in parallel
>this fails if one test tries to start svnserve while the other is still
>running.
>
>Use the test number as the svnserve port (similar to httpd tests) to
>avoid port conflicts.  Developers can set GIT_TEST_SVNSERVE to any value
>other than 'false' or 'auto' to enable these tests.
>
> ?

Yep, with this description it is
Reviewed-by: Jonathan Nieder 

Thanks for putting up with my nits. :)

Jonathan

Re: [PATCH 1/2] t/lib-git-svn: whitespace cleanup

[Jonathan Nieder]

Todd Zullinger wrote:
> Jonathan Nieder wrote:

>> nit: it would have been a tiny bit easier to review if the commit
>> message mentioned that this is only changing the indentation from an
>> inconsistent space/tab mixture to tabs and isn't making any other
>> changes.
>
> If only you saw how many times I typed a subject and changed it
> before settling on the terse version...

Heh.  No worries, it was a really small nit.

> How about:
>
>t/lib-git-svn: cleanup inconsistent tab/space usage
>
> ?

Sure, looks good.

Thanks again,
Jonathan

Re: How hard would it be to implement sparse fetching/pulling?

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:
> On 11/30/2017 3:03 PM, Jonathan Nieder wrote:

>> One piece of missing functionality that looks intereseting to me: that
>> series batches fetches of the missing blobs involved in a "git
>> checkout" command:
>>
>>   https://public-inbox.org/git/20171121211528.21891-14-...@jeffhostetler.com/
>>
>> But if doesn't batch fetches of the missing blobs involved in a "git
>> diff  " command.  That might be a good place to get
>> your hands dirty. :)
>
> Jonathan Tan added code in unpack-trees to bulk fetch missing blobs
> before a checkout.  This is limited to the missing blobs needed for
> the target commit.  We need this to make checkout seamless, but it
> does mean that checkout may need online access.

Just to clarify: other parts of the series already fetch all missing
blobs that are required for a command.  What that bulk-fetch patch
does is to make that more efficient, by using a single fetch request
to grab all the blobs that are needed for checkout, instead of one
fetch per blob.

This doesn't change the online access requirement: online access is
needed if and only if you don't have the required objects already
available locally.  For example, if at clone time you specified a
sparse checkout pattern and you haven't changed that sparse checkout
pattern, then online access is not needed for checkout.

> I've also talked about a pre-fetch capability to bulk fetch missing
> blobs in advance of some operation.  You could speed up the above
> diff command or back-fill all the blobs I might need before going
> offline for a while.

In particular, something like this seems like a very valuable thing to
have when changing the sparse checkout pattern.

Thanks,
Jonathan

Re: How hard would it be to implement sparse fetching/pulling?

[Jonathan Nieder]

Jeff Hostetler wrote:
> On 11/30/2017 6:43 PM, Philip Oakley wrote:

>> The 'companies' problem is that it tends to force a client-server, always-on
>> on-line mentality. I'm also wanting the original DVCS off-line capability to
>> still be available, with _user_ control, in a generic sense, of what they
>> have locally available (including files/directories they have not yet looked
>> at, but expect to have. IIUC Jeff's work is that on-line view, without the
>> off-line capability.
>>
>> I'd commented early in the series at [1,2,3].
>
> Yes, this does tend to lead towards an always-online mentality.
> However, there are 2 parts:
> [a] dynamic object fetching for missing objects, such as during a
> random command like diff or blame or merge.  We need this
> regardless of usage -- because we can't always predict (or
> dry-run) every command the user might run in advance.
> [b] batch fetch mode, such as using partial-fetch to match your
> sparse-checkout so that you always have the blobs of interest
> to you.  And assuming you don't wander outside of this subset
> of the tree, you should be able to work offline as usual.
> If you can work within the confines of [b], you wouldn't need to
> always be online.

Just to amplify this: for our internal use we care a lot about
disconnected usage working.  So it is not like we have forgotten about
this use case.

> We might also add a part [c] with explicit commands to back-fill or
> alter your incomplete view of the ODB

Agreed, this will be a nice thing to add.

[...]
>> At its core, my idea was to use the object store to hold markers for the
>> 'not yet fetched' objects (mainly trees and blobs). These would be in a
>> known fixed format, and have the same effect (conceptually) as the
>> sub-module markers - they _confirm_ the oid, yet say 'not here, try
>> elsewhere'.
>
> We do have something like this.  Jonathan can explain better than I, but
> basically, we denote possibly incomplete packfiles from partial clones
> and fetches as "promisor" and have special rules in the code to assert
> that a missing blob referenced from a "promisor" packfile is OK and can
> be fetched later if necessary from the "promising" remote.
>
> The main problem with markers or other lists of missing objects is
> that it has scale problems for large repos.

Any chance that we can get a design doc in Documentation/technical/
giving an overview of the design, with a brief "alternatives
considered" section describing this kind of thing?

E.g. some of the earlier descriptions like
 
https://public-inbox.org/git/20170915134343.3814d...@twelve2.svl.corp.google.com/
 https://public-inbox.org/git/cover.1506714999.git.jonathanta...@google.com/
 https://public-inbox.org/git/20170113155253.1644-1-benpe...@microsoft.com/
may help as a starting point.

Thanks,
Jonathan

Re: [PATCH v2 2/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Jonathan Nieder]

Todd Zullinger wrote:

> Reviewed-by: Jonathan Nieder 
> Signed-off-by: Todd Zullinger 
> ---
>  t/lib-git-svn.sh | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

This and the previous one are indeed still
Reviewed-by: Jonathan Nieder 

Thanks.

[PATCH] git-gui: allow Ctrl+T to toggle multiple paths (Re: [BUG] git gui can't commit multiple files)

[Jonathan Nieder]

From: Johannes Schindelin 
Subject: git-gui: allow Ctrl+T to toggle multiple paths

In the Unstaged Changes panel, selecting multiple lines (using
shift+click) and pressing ctrl+t to stage them causes one file to be
staged instead of all of the selected files.  The same also happens
when unstaging files.

This regression appears to have been introduced by gitgui-0.21.0~7^2~1
(Allow keyboard control to work in the staging widgets, 2016-10-01).

Also reported by zosrothko as a Git-for-Windows issue:
https://github.com/git-for-windows/git/issues/1012

[jn: fleshed out commit message]

Reported-by: Timon 
Signed-off-by: Johannes Schindelin 
Signed-off-by: Jonathan Nieder 
---
Hi Timon,

Timon wrote:
>>> On Sun, Dec 04, 2016 at 05:36:46PM +0100, Timon wrote:

>>>> This is a regression in git 2.11.0 (version 2.10.2 is fine).
>>>>
>>>> In git-gui I select multiple files in the Unstaged Changes (using
>>>> shift+click) and press ctrl+t to stage them. Then only one files gets
>>>> staged instead of all of the selected files.
>>>> The same happens when unstaging files.
[...]
> Originally I had this problem in gentoo and assumed in the end it's
> likely due to my specific configuration.
> However recently I switched to nixos and am still experiencing it.
>
> I've search again if I can find anything and lo and behold, it's
> already fixed in the *windows* version of git-gui...
>
> issue thread: https://github.com/git-for-windows/git/issues/1012
> commit: 
> https://github.com/git-for-windows/git/commit/1c4d4e7cbcf404c168df5537d55e9afd57f2b01b
>
> I applied it locally to git-2.15.0 and can confirm it fixed the problem for 
> me.
> If you need any more info/help from me, or would like me to test
> anything, feel free to ask.

Thanks for this pointer.  I'm including the patch here so the project
can consider applying it (it doesn't appear to have been sent upstream
before).  I have not tested it or verified the claim it makes about
what change introduced this regression --- if you're able to help with
that, that would be welcome.

Thanks,
Jonathan

 git-gui.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/git-gui.sh b/git-gui.sh
index ed24aa9..ef98fc2 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2501,6 +2501,19 @@ proc toggle_or_diff {mode w args} {
set pos [split [$w index @$x,$y] .]
foreach {lno col} $pos break
} else {
+   if {$mode eq "toggle"} {
+   if {$w eq $ui_workdir} {
+   do_add_selection
+   set last_clicked {}
+   return
+   }
+   if {$w eq $ui_index} {
+   do_unstage_selection
+   set last_clicked {}
+   return
+   }
+   }
+
if {$last_clicked ne {}} {
set lno [lindex $last_clicked 1]
} else {
-- 
2.15.0.531.g2ccb3012c9

Re: [PATCH] git-gui: allow Ctrl+T to toggle multiple paths (Re: [BUG] git gui can't commit multiple files)

[Jonathan Nieder]

Jonathan Nieder wrote:

> From: Johannes Schindelin 
> Subject: git-gui: allow Ctrl+T to toggle multiple paths
>
> In the Unstaged Changes panel, selecting multiple lines (using
> shift+click) and pressing ctrl+t to stage them causes one file to be
> staged instead of all of the selected files.  The same also happens
> when unstaging files.
>
> This regression appears to have been introduced by gitgui-0.21.0~7^2~1
> (Allow keyboard control to work in the staging widgets, 2016-10-01).
>
> Also reported by zosrothko as a Git-for-Windows issue:
> https://github.com/git-for-windows/git/issues/1012
>
> [jn: fleshed out commit message]
>
> Reported-by: Timon 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Jonathan Nieder 

Gah, this should say:

Signed-off-by; Jonathan Nieder 

[...]
>> I applied it locally to git-2.15.0 and can confirm it fixed the problem for 
>> me.
>> If you need any more info/help from me, or would like me to test
>> anything, feel free to ask.
>
> Thanks for this pointer.  I'm including the patch here so the project
> can consider applying it (it doesn't appear to have been sent upstream
> before).  I have not tested it or verified the claim it makes about
> what change introduced this regression --- if you're able to help with
> that, that would be welcome.

Can you bisect?  That is:

 git clone git://repo.or.cz/git-gui
 cd git-gui
 git bisect start
 git bisect good gitgui-0.20.0
 git bisect bad gitgui-0.21.0

Then cut to the chase:

 git checkout gitgui-0.21.0~7^2~1
 ... test test test ...
 git bisect (good|bad)

 git checkout gitgui-0.21.0~7^2~1^
 ... test test test ...
 git bisect (good|bad)

and follow the instructions if it suggests testing additional versions.

Then I'll be happy to re-send the patch with the results from your
testing.

Thanks again,
Jonathan

Re: [PATCH v1 2/2] t/README: document test_cmp_rev

[Jonathan Nieder]

Hi,

Thomas Gummerer wrote:

> test_cmp_rev is a useful function that's used in quite a few test
> scripts.  It is however not documented in t/README.  Document it.
>
> Signed-off-by: Thomas Gummerer 
> ---
>  t/README | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Jonathan Nieder 

I admit I usually go straight to t/test-lib-functions.sh when I want
to find an appropriate helper.  I think this kind of introductory
documentation in t/README is still useful, though.

Thanks,
Jonathan

Re: [PATCH v1 1/2] t/README: remove mention of adding copyright notices

[Jonathan Nieder]

Hi,

Thomas Gummerer wrote:

> We generally no longer include copyright notices in new test scripts.
> However t/README still mentions it as something to include at the top of
> every new script.

Where can I read more about this change?  Was it a deliberate change
or something that simply happened?

Thanks,
Jonathan

Re: How hard would it be to implement sparse fetching/pulling?

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:
> On 12/2/2017 1:24 PM, Philip Oakley wrote:
>> From: "Jeff Hostetler" 
>> Sent: Friday, December 01, 2017 5:23 PM

>>> Discussing this feature in the context of the defense industry
>>> makes me a little nervous.  (I used to be in that area.)
>>
>> I'm viewing the desire for codebase partitioning from a soft layering
>> of risk view (perhaps a more UK than USA approach ;-)
>
> I'm not sure I know what this means or how the UK defense
> security models/policy/procedures are different from the US,
> so I can't say much here.  I'm just thinking that even if we
> get a *perfectly working* partial clone/fetch/push/etc. that
> it would not pass a security audit.  I might be wrong here
> (and I'm no expert on the subject), but I think they would
> push you towards a different solution architecture.

I'm pretty ignorant about the defense industry, but a few more
comments:

- gitolite implements some features on top of git's server code that I
  consider to be important for security.  So much so that I've been
  considering what it would take to remove the git-shell command from
  git.git and move it to the gitolite project where people would be
  better equipped to use it in an appropriate context

- in particular, git's reachability checking code could use some
  hardening/improvement.  In particular, think of edge cases like
  where someone pushes a pack with deltas referring to objects they
  should not be able to reach.

- Anyone willing to audit git code's security wins my approval.
  Please, please, audit git code and report the issues you find. :)

[...]
> Also omitting certain trees means you now (obviously) have both missing
> trees and blobs.  And both need to be dynamically or batch fetched as
> needed.  And certain operations will need multiple round trips to fully
> resolve -- fault in a tree and then fault in blobs referenced by it.

For omitting trees, we will need to modify the index format, since the
index has entries for all paths today.  That's on the roadmap but has
not been implemented yet.

Thanks,
Jonathan

Re: [PATCH 1/2] git version --build-options: report the build platform, too

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:

> From: Adric Norris 
>
> When asking for bug reports to include the output of `git version
> --build-options`, the idea is that we get a better idea of the
> environment where said bug occurs. In this context, it is useful to
> distinguish between 32 and 64-bit builds.

Neat!

The cover letter gives a clearer idea of the motivation:

In Git for Windows, we ask users to paste the output of said command
into their bug reports, with the idea that this frequently helps
identify where the problems are coming from.

There are some obvious missing bits of information in said output,
though, and this patch series tries to fill the gaps at least a little.

Could some of that text go here, too?

[...]
> --- a/help.c
> +++ b/help.c
> @@ -390,6 +390,7 @@ const char *help_unknown_cmd(const char *cmd)
>  
>  int cmd_version(int argc, const char **argv, const char *prefix)
>  {
> + static char build_platform[] = GIT_BUILD_PLATFORM;
>   int build_options = 0;
>   const char * const usage[] = {
>   N_("git version []"),
> @@ -413,6 +414,7 @@ int cmd_version(int argc, const char **argv, const char 
> *prefix)
>  
>   if (build_options) {
>   printf("sizeof-long: %d\n", (int)sizeof(long));
> + printf("machine: %s\n", build_platform);

Can this use GIT_BUILD_PLATFORM directly instead of going via the indirection
of a mutable static string?  That is, something like

printf("machine: %s\n", GIT_BUILD_PLATFORM);

>   /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */

What do you think of this idea?  uname_M isn't one of the variables
in that file, though, so that's orthogonal to this patch.

[...]
> --- a/help.h
> +++ b/help.h
> @@ -33,3 +33,16 @@ extern void list_commands(unsigned int colopts, struct 
> cmdnames *main_cmds, stru
>   */
>  extern void help_unknown_ref(const char *ref, const char *cmd, const char 
> *error);
>  #endif /* HELP_H */
> +
> +/*
> + * identify build platform
> + */
> +#ifndef GIT_BUILD_PLATFORM
> + #if defined __x86__ || defined __i386__ || defined __i586__ || defined 
> __i686__
> + #define GIT_BUILD_PLATFORM "x86"
> + #elif defined __x86_64__
> + #define GIT_BUILD_PLATFORM "x86_64"
> + #else
> + #define GIT_BUILD_PLATFORM "unknown"
> + #endif
> +#endif

This code needs to be inside the HELP_H header guard.

Thanks and hope that helps,
Jonathan

Re: [PATCH 2/2] version --build-options: report commit, too, if possible

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:

> In particular when local tags are used (or tags that are pushed to some
> fork) to build Git, it is very hard to figure out from which particular
> revision a particular Git executable was built.

Hm, can you say more about how this comes up in practice?  I wonder if
we should always augment the version string with the commit hash.
E.g. I am running

git version 2.15.1.424.g9478a66081

now, which already includes the commit hash because it disambiguates
the .424 thing, but depending on the motivation, maybe we would also
want

git version 2.15.1.0.g9b185bef0c15

for people running v2.15.1 (or at least when such a tag is not a well
known, published tag).

> We need to be careful, though, to report when the current commit cannot be
> determined, e.g. when building from a tarball without any associated Git
> repository.

This means that on Debian, it would always print

built from commit: (unknown)

Maybe I shouldn't care, but I wonder if there's a way to improve on
that. E.g. should there be a makefile knob to allow Debian to specify
what to put there?

Thanks,
Jonathan

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Jonathan Nieder]

Hi,

Yaroslav Halchenko wrote:

> Example to show that TFM outlines precedence and --global correctly:
>
> $> grep xdg .gitconfig .config/git/config
> .gitconfig:xdg-and-user = user
> .config/git/config: xdg = xdg
> .config/git/config: xdg-and-user = xdg
> $> git config user.xdg ; git config user.xdg-and-user
> xdg
> user

I agree, this is confusing.

Reverse engineering from source, I find that git reads the following
files in sequence:

system:
/etc/gitconfig
global:
$XDG_CONFIG_HOME/git/config
$HOME/.gitconfig
repo:
$GIT_DIR/config
commandline:
options passed with -c or GIT_CONFIG_PARAMETERS

These terms (system, global, repo, etc) are accessible in code as
current_config_scope().  I don't think there's any user-visible effect
to $XDG_CONFIG_HOME/git/config and $HOME/.gitconfig both being global
--- it would probably be a good cleanup to rename the scope for one of
them.

I think the documentation

~/.gitconfig
User-specific configuration file. Also called "global"
configuration file.

should be clarified --- e.g. it could say

$XDG_CONFIG_HOME/git/config
~/.gitconfig
User-specific configuration files. Because options in
these files are not specific to any repository, thes
are sometimes called global configuration files.

As for "git config --global", I think the best thing would be to split
it into two options: something like "git config --user" and "git
config --xdg-user".  That way, it is unambiguous which configuration
file the user intends to inspect or modify.  When a user calls "git
config --global" and both files exist, it could warn that the command
is ambiguous.

Thoughts?

Thanks,
Jonathan

Re: What's cooking in git.git (Dec 2017, #03; Wed, 13)

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> * sb/diff-blobfind (2017-12-12) 1 commit
>   (merged to 'next' on 2017-12-13 at 9a27a20c5f)
>  + diffcore: add a filter to find a specific blob
>
>  "git diff" family of commands learned --blobfind= that
>  allows you to limit the output only to a change that involves the
>  named blob object (either changing the contents from or to it).
>
>  Will merge to 'master'.

Sorry, I should have replied about this a long time ago: I love this
option but I am not sure that --blobfind is the right name for it.
If we can think of a better name quickly then it would be nice to
change it before people start relying on it, so I'd rather hold off
on merging this to 'master' for the moment.

That said, if we don't have any better ideas for the option name
within a few days then my objection goes away.

I'll reply in the patch thread.

Thanks,
Jonathan

Re: [PATCH] diffcore: add a filter to find a specific blob

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Junio hinted at a different approach of solving this problem, which this
> patch implements. Teach the diff machinery another flag for restricting
> the information to what is shown. For example:
>
>   $ ./git log --oneline --find-object=v2.0.0:Makefile
>   b2feb64309 Revert the whole "ask curl-config" topic for now
>   47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"
>
> we observe that the Makefile as shipped with 2.0 was appeared in
> v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.

Neat!

Nit: s/was appeared/appeared/ (not a big deal though, since this is
already in 'next').

>From the above it's not clear to me whether this is about commits
where the object was added or where the object was removed.
Fortunately, the docs are a bit clearer:

 ... one side of the diff
   matches the given object id. The object can be a blob,
   gitlink entry or tree (when `-t` is given).

So this appears to be about both additions and removals.  Followup
questions:

- are copies and renames shown (if I am passing -M -C)?
- what about mode changes?  If the file became executable but the
  blob content didn't change, does that commit match?

Nit, not related to this change: it would be nice to have a long
option to go along with the short name '-t' --- e.g. --include-trees.

Another nit: s/gitlink entry/submodule commit/, perhaps.  The commit
object is not a gitlink entry: it is pointed to by a gitlink entry.

Another documentation idea: it may be nice to point out that this
is only about the preimage and postimage submodule commit and that
it doesn't look at the history in between.

>  The
> reason why these commits both occur prior to v2.0.0 are evil
> merges that are not found using this new mechanism.

Would it be worth the doc mentioning that this doesn't look at merges
unless you use one of the -m/-c/--cc options, or does that go without
saying?

[...]
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -500,6 +500,12 @@ information.
>  --pickaxe-regex::
>   Treat the  given to `-S` as an extended POSIX regular
>   expression to match.
> +
> +--find-object=::
> + Restrict the output such that one side of the diff
> + matches the given object id. The object can be a blob,
> + gitlink entry or tree (when `-t` is given).

I like this name --find-object more than --blobfind!  I am not sure it
quite matches what the user is looking for, though.  We are not
looking for all occurences of the object; we only care about when the
object appears (was added or removed) in the diff.

Putting it in context, we have:

pickaxe options:
-S: detect changes in occurence count of a string
-G: grep lines in diff for a string

--pickaxe-all:
do not filter the diff when the patch matches pickaxe
conditions.

kind of like log --full-diff, but restricted to pickaxe
options.
--pickaxe-regex: treat -S argument as a regex, not a string

Is this another kind of pickaxe option?  It feels similar to -S, but
at an object level instead of a substring level, so in a way it would
be appealing to call it --pickaxe-object.  Does --pickaxe-all affect
it like it affects -S and -G?

Another context to put it in is:

--diff-filter:
limit paths (but not commits?) to those with a change
matching optarg

If I understand correctly, then --diff-filter does not interact with
--pickaxe-all, or in other words it is a different filtering
condition.  Is this another kind of diff filter?  In that context, it
may be appealing to call it something like --object-filter.

--diff-filter is an example where it seems appealing to have a
--full-diff option to diff-tree that could apply to all filters and
not just pickaxe.

[... implementation snipped ...]

The implementation looks lovely and I'm especially happy about the
tests.  Thanks for writing it.

Thoughts?
Jonathan

Re: What's cooking in git.git (Dec 2017, #03; Wed, 13)

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:

>>> * sb/diff-blobfind (2017-12-12) 1 commit
>>>   (merged to 'next' on 2017-12-13 at 9a27a20c5f)
>>>  + diffcore: add a filter to find a specific blob
>>>
>>>  "git diff" family of commands learned --blobfind= that
>>>  allows you to limit the output only to a change that involves the
>>>  named blob object (either changing the contents from or to it).
>>>
>>>  Will merge to 'master'.
>>
>> Sorry, I should have replied about this a long time ago: I love this
>> option but I am not sure that --blobfind is the right name for it.
>
> Sorry.  I should have updated the description when the option name
> was updated in the latest round.

Sure.  My worry still applies:
https://public-inbox.org/git/20171214212234.gc32...@aiede.mtv.corp.google.com/

But as I said, if nothing comes of it within a few days then I'm happy
to conclude that we did our best.

Thanks,
Jonathan

Re: [PATCH] diffcore: add a filter to find a specific blob

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Thu, Dec 14, 2017 at 1:22 PM, Jonathan Nieder  wrote:

>> - what about mode changes?  If the file became executable but the
>>   blob content didn't change, does that commit match?
>
> ./git log --find-object=$(git rev-parse ba67504f:t/perf/p3400-rebase.sh)
>
> claims it does find the mode change (commit ba67504f is just a mode
> change)

Thanks.  Reminder to self to add a test + docs about that (as a followup
change; this isn't a complaint about the patch).

>> - are copies and renames shown (if I am passing -M -C)?
>
> It restricts the commits shown, not the renamed files. But I assume
> you mean it the same way as with mode changes.
> I did not find a good commit in gits history to demonstrate, but as
> it is orthogonal to the object id restrictions, I would think it works

Ok, will add test + doc.

>> Nit, not related to this change: it would be nice to have a long
>> option to go along with the short name '-t' --- e.g. --include-trees.
>
> follow up patches welcome. :)

Will think more and try to send a patch if it still seems like a good
idea in a day or so. ;)

>> Another nit: s/gitlink entry/submodule commit/, perhaps.  The commit
>> object is not a gitlink entry: it is pointed to by a gitlink entry.
>
> Well, what if the user doesn't have a submodule, but uses gitlinks
> for other purposes? We do inspect the gitlink, so it is correct IMHO.

It's a language nit.  The argument to --find-object is a commit object
name, not a gitlink entry.  A gitlink entry looks like

 16  

>> Another documentation idea: it may be nice to point out that this
>> is only about the preimage and postimage submodule commit and that
>> it doesn't look at the history in between.
>
> That is sensible. One might be tempted to ask: "Which superproject
> commit contains a submodule pointer, that has commit $X in the
> submodule history?", but this new option is totally not answering this.

Ok, will try to come up with wording.

>>>  The
>>> reason why these commits both occur prior to v2.0.0 are evil
>>> merges that are not found using this new mechanism.
>>
>> Would it be worth the doc mentioning that this doesn't look at merges
>> unless you use one of the -m/-c/--cc options, or does that go without
>> saying?
>
> I assumed it goes without saying, just like the lacking -t could mean
> to ignore trees. ;)

I suspect it's worth a mention, based on the discussion in this thread
(i.e. without such docs it was non-obvious and took some time to
diagnose).

[...]
>>> +--find-object=::
>>> + Restrict the output such that one side of the diff
>>> + matches the given object id. The object can be a blob,
>>> + gitlink entry or tree (when `-t` is given).
>>
>> I like this name --find-object more than --blobfind!  I am not sure it
>> quite matches what the user is looking for, though.  We are not
>> looking for all occurences of the object; we only care about when the
>> object appears (was added or removed) in the diff.
>
> Thanks! Yes, but the 'edges' are so few commits that a further walk
> will reveal all you need to know?

Sorry for the lack of clarity: I actually like this behavior *more*
than a "find trees pointing to object" behavior.  I'm just saying the
name sets an unclear expectation.

[...]
> Regarding finding a better name, I would want to hear from others,
> I am happy with --find-object, though I can see --pickaxe-object
> or --object--filter to be a good narrative as well.

Drat, I was hoping for an opinion.

Based on the answers above about mode changes and renames, at the
moment --object-filter seems clearest to me.

Thanks,
Jonathan

Re: feature-request: git "cp" like there is git mv.

[Jonathan Nieder]

Hi Simon,

Simon Doodkin wrote:

> please develop a new feature, git "cp" like there is git mv tomovefile1 
> tofile2
> (to save space).
>
> there is a solution in https://stackoverflow.com/a/44036771/466363
> however, it is not single easy command.

This sounds like a reasonable thing to add.  See builtin/mv.c for how
"git mv" works if you're looking for inspiration.

cmd_mv in that file looks rather long, so I'd also be happy if someone
interested refactors to break it into multiple self-contained pieces
for easier reading (git mostly follows
https://www.kernel.org/doc/html/latest/process/coding-style.html#functions).

Thanks,
Jonathan

Re: [PATCH v3] imap-send: URI encode server folder

[Jonathan Nieder]

Kaartic Sivaraam wrote:

> From: Nicolas Morey-Chaisemartin 
>
> When trying to send a patch using 'imap-send' with 'curl' and the
> following configuration:
>
> [imap]
>   folder = "[Gmail]/Drafts"
>   host = imaps://imap.gmail.com
>   port = 993
>   sslverify = false
>
> results in the following error,
>
> curl_easy_perform() failed: URL using bad/illegal format or missing URL
>
> This is a consequence of not URI-encoding the folder portion of
> the URL which contains characters such as '[' which are not
> allowed in a URI. According to RFC3986, these characters should be
> URI-encoded.
>
> So, URI-encode the folder before adding it to the URI to ensure it doesn't
> contain characters that aren't allowed in a URI.
>
> Reported-by: Doron Behar 
> Signed-off-by: Nicolas Morey-Chaisemartin 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  imap-send.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Thanks!
Reviewed-by: Jonathan Nieder 

Is there a straightforward way to check that this behavior gets
preserved in tests?

Sincerely,
Jonathan

Re: [PATCH] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

Hi,

Wei Shuyu wrote:

> HTTP proxy over SSL is supported by curl since 7.52.0.
> This is very useful for networks with protocol whitelist.
>
> Signed-off-by: Wei Shuyu 
> ---
>  http.c | 5 +
>  1 file changed, 5 insertions(+)

Thanks for writing this.  Can you give an example of how I'd use it
(ideally in the form of a test in t/ so we avoid this functionality
regressing, but if that's not straightforward then an example for the
commit message is fine as well)?

> diff --git a/http.c b/http.c
> index 215bebef1..32d33261c 100644
> --- a/http.c
> +++ b/http.c
> @@ -865,6 +865,11 @@ static CURL *get_curl_handle(void)
>   else if (starts_with(curl_http_proxy, "socks"))
>   curl_easy_setopt(result,
>   CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x073400

Can this use #ifdef CURLPROXY_HTTPS instead?  That way, if someone's
copy of curl has backported support then they get the benefit of this
change as well.

> + else if (starts_with(curl_http_proxy, "https"))
> + curl_easy_setopt(result,
> + CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
>  #endif
>   if (strstr(curl_http_proxy, "://"))
>   credential_from_url(&proxy_auth, curl_http_proxy);

Thanks and hope that helps,
Jonathan

Re: [PATCH] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:
>> Wei Shuyu wrote:

>>> diff --git a/http.c b/http.c
>>> index 215bebef1..32d33261c 100644
>>> --- a/http.c
>>> +++ b/http.c
>>> @@ -865,6 +865,11 @@ static CURL *get_curl_handle(void)
>>> else if (starts_with(curl_http_proxy, "socks"))
>>> curl_easy_setopt(result,
>>> CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
>>> +#endif
>>> +#if LIBCURL_VERSION_NUM >= 0x073400
>>
>> Can this use #ifdef CURLPROXY_HTTPS instead?  That way, if someone's
>> copy of curl has backported support then they get the benefit of this
>> change as well.
>
> It sounds like a worthwhile thing to do (assuming that these are
> always implemented as preprocessor macros).

Oh, good point!  It's an enumerator, not a preprocessor macro.  But
there is a preprocessor macro CURL_VERSION_HTTPS_PROXY.

Anyway, using LIBCURL_VERSION_NUM is consistent with the surrounding
code.

Thanks,
Jonathan

Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> The test is marked as a failure as the fix comes in a later patch.
>
> Signed-off-by: Stefan Beller 
> ---
>  t/lib-submodule-update.sh | 11 +++
>  1 file changed, 11 insertions(+)

I think I'd find this easier to undrestand if it were squashed with the
patch that fixes it.

This is part of test_submodule_foced_switch --- does that mean it
affects both checkout -f and reset --hard, or only the latter?

Thanks,
Jonathan

Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case

[Jonathan Nieder]

Hi,

I had trouble understanding what this fixes, so I'll try nitpicking a
bit as a sideways way to address that.

Stefan Beller wrote:

> With the previous patch applied (fix of the same() function),

This tripped me up a bit.  Usually commits assume that all previous
patches have already been applied, since that allows the maintainer to
apply the early part of a series on one day and the later part another
day without losing too much context.

I think this intends to say something like

Now that we allow recursing into an unchanged submodule (see
"unpack-trees: Fix same() for submodules", 2017-12-19), it is
possible for ...

>   the
> function `submodule_move_head` may be invoked with the same argument
> for the `old` and `new` state of a submodule, for example when you
> run `reset --hard --recurse-submodules` in the superproject that has no
> change in the gitlink entry, but only worktree related change in the
> submodule. The read-tree call in the submodule is not amused about
> the duplicate argument.

What is the symptom of read-tree being unamused?  Is that a sign that
these patches are out of order (i.e. that we should prepare to handle an
unchanged submodule first, and then adjust the caller to pass in
unchanged submodules)?

> It turns out that we can omit the duplicate old argument in all forced
> cases anyway, so let's do that.

What is the end-user visibile effect of such a change?  E.g. what
becomes possible to a user that wasn't possible before?

Among the commands you mentioned before:

  checkout -f
I think I would expect this not to touch a submodule that
hasn't changed, since that would be consistent with its
behavior on files that haven't changed.

  reset --hard
Nice!  Yes, recursing into unchanged submodules is a big part
of the psychological comfort of being able to say "you can
always use reset --hard  to get back to a known
state".

  read-tree -u --reset
This is essentially the plumbing equivalent of reset --hard,
so it makes sense for them to be consistent.  Ok.

Based on the checkout -f case, if I've understood correctly then patch
4/5 goes too far on it (but I could easily be convinced otherwise).

> Signed-off-by: Stefan Beller 
> ---
>  submodule.c   | 4 +++-
>  t/lib-submodule-update.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index fa25888783..db0f7ac51e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,
>   else
>   argv_array_push(&cp.args, "-m");
>  
> - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
> + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
> + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);

Interesting.  What is the effect when old != new?

Thanks,
Jonathan

Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder  wrote:

>>   checkout -f
>> I think I would expect this not to touch a submodule that
>> hasn't changed, since that would be consistent with its
>> behavior on files that haven't changed.
[...]
> I may have a different understanding of git commands than you do,
> but a plain "checkout -f" with no further arguments is the same as
> a hard reset IMHO, and could be used interchangeably?

Ah, good catch.  Filed https://crbug.com/git/5 to clarify this in
documentation.  Thanks for clarifying.

Jonathan

Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case

[Jonathan Nieder]

Stefan Beller wrote:
> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder  wrote:

>>   checkout -f
>> I think I would expect this not to touch a submodule that
>> hasn't changed, since that would be consistent with its
>> behavior on files that haven't changed.
[...]
> I may have a different understanding of git commands than you do,
> but a plain "checkout -f" with no further arguments is the same as
> a hard reset IMHO, and could be used interchangeably?

A kind person pointed out privately that you were talking about
"git checkout -f" with no further arguments, not "git checkout -f
".  In that context, the competing meanings I mentioned in
https://crbug.com/git/5 don't exist: either a given entry in the
worktree matches the index or it doesn't.

So plain "git checkout -f" is similar to plain "git reset --hard"
in that it means "make the worktree (and index, in the reset case)
look just like this".  checkout -f makes the worktree look like the
index; reset --hard makes the worktree and index look like HEAD.

In that context, more aggressively making the submodule in the
worktree get into the defined state sounds to me like a good change.

Hopefully my confusion helps illustrate what a commit message focusing
on the end user experience might want to mention.

Thanks again,
Jonathan

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:

> Created core.aheadbehind config setting and core_ahead_behind
> global variable.  This value defaults to true.
>
> This value will be used in the next few commits as the default value
> for the --ahead-behind parameter.
>
> Signed-off-by: Jeff Hostetler 
> ---
>  Documentation/config.txt | 8 
>  cache.h  | 1 +
>  config.c | 5 +
>  environment.c| 1 +
>  4 files changed, 15 insertions(+)

Not a reason to reroll on its own, but this seems out of order: the
series is easier to explain and easier to merge down in stages if the
patch for --ahead-behind comes first, then the config setting.

More generally, new commandline flags tend to be less controversial
than new config settings since they cannot affect a script by mistake,
and for that reason, they can go earlier in the series.

As a bonus, that makes it possible to include tests.  It's probably
worth adding a test or two for this new config setting.

[...]
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9593bfa..c78d6be 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -895,6 +895,14 @@ core.abbrev::
>   abbreviated object names to stay unique for some time.
>   The minimum length is 4.
>  
> +core.aheadbehind::
> + If true, tells commands like status and branch to print ahead and
> + behind counts for the branch relative to its upstream branch.
> + This computation may be very expensive when there is a great
> + distance between the two branches.  If false, these commands
> + only print that the two branches refer to different commits.
> + Defaults to true.

This doesn't seem like a particularly core feature to me.  Should it be
e.g. status.aheadbehind (even though it also affects "git branch") or
even something like diff.aheadbehind?  I'm not sure.

I also wonder if there's a way to achieve the same benefit without
having it be configurable.  E.g. if a branch is way behind, couldn't
we terminate the walk early to get the same bounded cost per branch
without requiring configuration?

Thanks,
Jonathan

Re: [PATCH v2 2/5] stat_tracking_info: return +1 when branches are not equal

[Jonathan Nieder]

Jeff Hostetler wrote:
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1239,7 +1239,7 @@ static void fill_remote_ref_details(struct used_atom 
> *atom, const char *refname,
>   *s = show_ref(&atom->u.remote_ref.refname, refname);
>   else if (atom->u.remote_ref.option == RR_TRACK) {
>   if (stat_tracking_info(branch, &num_ours,
> -&num_theirs, NULL)) {
> +&num_theirs, NULL, ABF_FULL) < 0) {

What does ABF stand for?  It made me think of airport codes.

Would a name like AHEADBEHIND_FULL work?

[...]
> --- a/remote.c
> +++ b/remote.c
> @@ -1977,16 +1977,22 @@ int ref_newer(const struct object_id *new_oid, const 
> struct object_id *old_oid)
>  }
>  
>  /*
> - * Compare a branch with its upstream, and save their differences (number
> - * of commits) in *num_ours and *num_theirs. The name of the upstream branch
> - * (or NULL if no upstream is defined) is returned via *upstream_name, if it
> - * is not itself NULL.
> + * Compare a branch with its upstream and report on their differences.
> + * If abf is ABF_FULL, save their differences (number of commits) in
> + * *num_ours and *num_theirs.
> + * If abf is ABF_QUICK, skip the (possibly expensive) ahead/behind

Please format these comments as paragraphs, with a consistent
line-width and a "blank" (space-star-newline) line between paragraphs.
That makes them much easier to read.

[...]
> @@ -2019,6 +2025,8 @@ int stat_tracking_info(struct branch *branch, int 
> *num_ours, int *num_theirs,
>   *num_theirs = *num_ours = 0;
>   return 0;
>   }
> + if (abf == ABF_QUICK)
> + return 1;

nit: I think this is missing a blank line before the 'if'.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2 3/5] status: add --[no-]ahead-behind to porcelain V2 output

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:

> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -141,6 +141,7 @@ static int sequencer_in_use;
>  static int use_editor = 1, include_status = 1;
>  static int show_ignored_in_status, have_option_m;
>  static struct strbuf message = STRBUF_INIT;
> +static int ahead_behind_opt = -1;

nit: is there a logical place amid these constants to put the new option
instead of chronological order to make it easier to read through later?
That also has the side-benefit of making the new option less likely to
collidate with other patches that add a new option to commit.

That collection of options seems to be mostly about how the commit
message is generated.  Maybe this one could go after status_format:

static enum wt_status_format status_format = ...;
static int ahead_behind;

Even better if it can be made into a local in cmd_status.

[...]
> @@ -1369,6 +1370,8 @@ int cmd_status(int argc, const char **argv, const char 
> *prefix)
> N_("ignore changes to submodules, optional when: all, dirty, 
> untracked. (Default: all)"),
> PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
>   OPT_COLUMN(0, "column", &s.colopts, N_("list untracked files in 
> columns")),
> + OPT_BOOL(0, "ahead-behind", &ahead_behind_opt,
> +  N_("compute branch ahead/behind values")),
>   OPT_END(),

Similar question: is there a natural place in "git status -h" to show
the new option instead of chronological order?

What does the value of the ahead_behind variable represent?  -1 means
unset so that we use config?  A comment might help.

[...]
> @@ -1389,6 +1392,21 @@ int cmd_status(int argc, const char **argv, const char 
> *prefix)
>  PATHSPEC_PREFER_FULL,
>  prefix, argv);
>  
> + /*
> +  * Porcelain formats only look at the --[no-]ahead-behind command
> +  * line argument and DO NOT look at the config setting.  Non-porcelain
> +  * formats use both.
> +  */

nit: No need to shout: s/DO NOT/do not/

> + if (status_format == STATUS_FORMAT_PORCELAIN ||
> + status_format == STATUS_FORMAT_PORCELAIN_V2) {
> + if (ahead_behind_opt < 0)
> + ahead_behind_opt = ABF_FULL;
> + } else {
> + if (ahead_behind_opt < 0)
> + ahead_behind_opt = core_ahead_behind;
> + }

Can be more concise, to save the reader some time if they don't care
about the defaulting behavior:

if (ahead_behind_opt == -1) {
if (status_format == ...)
ahead_behind_opt = ...;
else
ahead_behind_opt = ...;
}
}

> + s.ab_flags = ((ahead_behind_opt) ? ABF_FULL : ABF_QUICK);

nit: both parens here are unnecessary and don't make the code clearer

[...]
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -390,6 +390,66 @@ test_expect_success 'verify upstream fields in branch 
> header' '
>   )
>  '
>  
> +test_expect_success 'verify --no-ahead-behind generates branch.qab' '
> + git checkout master &&
> + test_when_finished "rm -rf sub_repo" &&
> + git clone . sub_repo &&
> + (
> + ## Confirm local master tracks remote master.
> + cd sub_repo &&
> + HUF=$(git rev-parse HEAD) &&
> +
> + cat >expect <<-EOF &&
[...]

This looks like a collection of multiple tests.  Is there a
straightforward way to split them into multiple independent
test_expect_successes?

That way, it's easier to tell which failed if there is a regression
later and to run only one of them (using GIT_SKIP_TESTS) when
debugging such a failure.

Thanks,
Jonathan

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

Wei Shuyu wrote:

> Git has been taught to support an https:// used for http.proxy when
> using recent versions of libcurl.

nit: commit messages use the imperative mood, as though commanding the
code base to do something:

Support https:// for http.proxy when using recent versions of
curl.

[...]
> To use https proxy with self-signed certs, we need a way to
> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST
> just like direct SSL connection. This is required if we want
> use t/lib-httpd to test proxy.

Interesting.  Sounds promising.

> In this patch I reuse http.sslverify to do this, do we need an
> independent option like http.sslproxyverify?

I think a separate option is a good idea.  This way, I can use

[http "https://example.com";]
sslVerify = false

to fetch from a misconfigured server or

[http "https://example.com";]
proxy = https://127.0.0.1
sslVerifyProxy = false

to proxy through a misconfigured proxy.  Alternatively, is there a
straightforward way to make

[http "https://example.com";]
proxy = https://127.0.0.1
[http "https://127.0.0.1";]
sslVerify = false

do that?  Something like

struct urlmatch_config proxy_config = { STRING_LIST_INIT_DUP };
config.section = "http";
config.key = NULL;
config.collect_fn = proxy_options;
config.cascade_fn = git_default_config;
config.cb = NULL;
config.url = normalized_proxy_url;
git_config(urlmatch_config_entry, &config);

How does this interact with the GIT_SSL_NO_VERIFY environment
variable?

> To fully support https proxy, we also need ways to set more options
> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to
> support them.

That's for client certs, right?  Would it work to make that
configurable as

[http "https://127.0.0.1";]
sslCert = ...

?

That said, I agree with you that it's not a prerequisite for this
initial patch.

Thanks,
Jonathan

Re: [PATCH] diffcore: add a filter to find a specific blob

[Jonathan Nieder]

Stefan Beller wrote:
> On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano  wrote:

>> I think it would make it a better companion to --pickaxe but we need
>> to align its behaviour a little bit so that it plays better with the
>> "--pickaxe-all" option, and also needs to hide mode and name only
>> changes just like pickaxe.
>
> I looked into this, and the small changes needed led me to thinking
> it could be integrated into the diffcore-pickaxe code completely,
> roughly like (spaces mangled):

Nice, this looks promising.

[...]
> But then, it seems as if any pickaxe option is incompatible with
> any other, i.e. from reading the code, you cannot combine -S
> and -G, or even give one of them twice.
>
> I guess that would be not a big deal for the --pickaxe-object,
> but just want to point it out.

Agreed that that's not a big deal for --pickaxe-object.

>> After all, the diffcore-blobfind code was written while looking at
>> the diffcore-pickaxe's code in another window shown in the pager,
>> and I tend to agree with your earlier message that this is an
>> extreme case of -S where the contents happens to be the
>> whole file.
>
> I disagree, as the user doesn't have the content, but the hash
> over the content only and wants to know more about it. The new
> option cannot be used to find a file whose partial content hashes to
> the given sha1, either.
>
> So with these considerations, I would keep the patch as currently\
> queued at sb/diff-blobfind.

Interesting --- I come to the opposite conclusion.

The pickaxe-style behavior seems more consistent and simpler to
explain and better matches the use cases I can think of.

Thanks,
Jonathan

Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> This is similar to Jeff King's jk/drop-ancient-curl series in that
> we're dropping perl releases that are rarely tested anymore, however
> unlike those patches git still works on e.g. 5.8.8 (I couldn't build
> anything older).

FWIW I think this is a good change.  If someone is affected and offers
help with keeping git working well with older perl (e.g. setting up CI
for us) then we can keep supporting it, but it seems more likely to me
that there will just not be anyone affected.

And being able to assume a newer baseline perl version seems
beneficial both from a security point of view and from a developer
experience pov.

Some minor nits:

[...]
>  contrib/diff-highlight/DiffHighlight.pm | 2 +-
>  contrib/examples/git-difftool.perl  | 2 +-
>  contrib/mw-to-git/Git/Mediawiki.pm  | 2 +-

I'm less opinionated about these.  They're already using the same perl
baseline as the rest of git, so I suppose it's good to keep them
consistent, but if any of them gives you trouble then I think you can
just ignore it and let the relevant contrib maintainer take care of
it. :)

[...]
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1,6 +1,6 @@
>  #!/usr/bin/perl
>  
> -use 5.008;
> +use v5.10.0;

As brian carlson mentioned, 'use 5.010' would produce a clearer error
message on ancient perl, which could result in happier users on
ancient platforms or clearer bug reports.

That said, I don't have a strong opinion there.

Reviewed-by: Jonathan Nieder 

Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0

[Jonathan Nieder]

Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:

>> This is similar to Jeff King's jk/drop-ancient-curl series in that
>> we're dropping perl releases that are rarely tested anymore, however
>> unlike those patches git still works on e.g. 5.8.8 (I couldn't build
>> anything older).
[...]
> Reviewed-by: Jonathan Nieder 

One caveat I forgot: please also update the INSTALL file:

 - Git is reasonably self-sufficient, but does depend on a few external
   programs and libraries.  Git can be used without most of them by adding
   the approriate "NO_=YesPlease" to the make command line or
   config.mak file.
[...]
- "Perl" version 5.8 or later is needed to use some of the

Thanks again,
Jonathan

Re: [PATCH 1/2] travis-ci: don't try to create the cache directory unnecessarily

[Jonathan Nieder]

SZEDER Gábor wrote:

> Travis CI creates that directory for us anyway, even when a previous
> cache doesn't exist for the current build job.
>
> In itself it doesn't hurt to try, of course, but the following patch
> will access the Travis CI cache much earlier in the build process, and
> then creating the cache directory this late might cause confusion.
>
> Signed-off-by: SZEDER Gábor 
> ---
>  ci/run-tests.sh | 1 -
>  1 file changed, 1 deletion(-)

Is this behavior documented anywhere?
https://docs.travis-ci.com/user/caching#Arbitrary-directories doesn't
say anything about it.

Thanks,
Jonathan

Re: [PATCH 1/2] Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Stop supplying BLK_SHA1=YesPlease when NO_OPENSSL=UnfortunatelyYes is
> supplied. This has been done ever since [1], when switching to DC_SHA1
> by default in [2] this should have been changed as well.

I had trouble parsing this, I think for a few reasons:

 1. Too much 'this', so I end up not being able to keep track of what
has been done ever since (1) and (2)

 2. The ',' should be a ';'

 3. Can the commit names go inline in the message instead of being
footnotes?  That way, my eye doesn't have to chase so far to
find what kind of dates you are talking about.

 4. Why should switching to DC_SHA1 by default have resulted in
simply dropping the NO_OPENSSL => BLK_SHA1 behavior?  At first
glance it would be more intuitive to change it to
NO_OPENSSL => DC_SHA1 instead.

Putting those all together, I end up with

 Use the collision detecting SHA-1 implementation by default even
 when NO_OPENSSL is set.

 Setting NO_OPENSSL=UnfortunatelyYes has implied BLK_SHA1=1 ever since
 the former was introduced in dd53c7ab29 (Support for NO_OPENSSL,
 2005-07-29).  That implication should have been removed when the
 default SHA-1 implementation changed from OpenSSL to DC_SHA1 in
 e6b07da278 (Makefile: make DC_SHA1 the default, 2017-03-17).  Finish
 what that commit started by removing the BLK_SHA1 fallback setting so
 the default DC_SHA1 implementation can be used.

What happens if I set both OPENSSL_SHA1 and NO_OPENSSL?  Should this
block set

OPENSSL_SHA1 =

so that the latter wins, or should we detect it as an error?

[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -23,7 +23,6 @@ all::
>  # it at all).
>  #
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
> -# This also implies BLK_SHA1.
>  #
>  # Define USE_LIBPCRE if you have and want to use libpcre. Various
>  # commands such as log and grep offer runtime options to use
> @@ -1260,7 +1259,6 @@ ifndef NO_OPENSSL
>   endif
>  else
>   BASIC_CFLAGS += -DNO_OPENSSL
> - BLK_SHA1 = 1
>   OPENSSL_LIBSSL =
>  endif
>  ifdef NO_OPENSSL
> diff --git a/configure.ac b/configure.ac
> index 2f55237e65..7f8415140f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -241,7 +241,6 @@ AC_MSG_NOTICE([CHECKS for site configuration])
>  # a bundled SHA1 routine optimized for PowerPC.
>  #
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
> -# This also implies BLK_SHA1.

With or without some of those commit message tweaks
Reviewed-by: Jonathan Nieder 

Thanks,
Jonathan

Re: [PATCH 2/2] Windows: stop supplying BLK_SHA1=YesPlease by default

[Jonathan Nieder]

+git-for-windows
Ævar Arnfjörð Bjarmason wrote:

> Using BLK_SHA1 in lieu of the OpenSSL routines was done in [1], but
> since DC_SHA1 is now the default for git in general it makes sense for
> Windows to use that too, this looks like something that was missed
> back in [2].
>
> As noted in [3] OpenSSL has a performance benefit compared to BLK_SHA1
> on MinGW, so perhaps that and the Windows default should be changed
> around again, but that's a topic for another series, it seems clear
> that this specific flag is nobody's explicit intention.

I have some memory of performance issues on Windows when DC_SHA1 was
introduced leading to interest in a mixed configuration with DC_SHA1
only being used where it is security sensitive (e.g. for object naming
but not for packfile trailers).

Did anything come of that?

In any event removing this BLK_SHA1 setting looks like a good change
to me, but I'd rather that Windows folks weigh in.

Thanks,
Jonathan

[...]
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -361,7 +361,6 @@ ifeq ($(uname_S),Windows)
>   NO_REGEX = YesPlease
>   NO_GETTEXT = YesPlease
>   NO_PYTHON = YesPlease
> - BLK_SHA1 = YesPlease
>   ETAGS_TARGET = ETAGS
>   NO_INET_PTON = YesPlease
>   NO_INET_NTOP = YesPlease

Re: [PATCH 1/2] travis-ci: don't try to create the cache directory unnecessarily

[Jonathan Nieder]

SZEDER Gábor wrote:
> On Wed, Dec 27, 2017 at 8:46 PM, Jonathan Nieder  wrote:
> > SZEDER Gábor wrote:

>>> Travis CI creates that directory for us anyway, even when a previous
>>> cache doesn't exist for the current build job.
>>>
>>> In itself it doesn't hurt to try, of course, but the following patch
>>> will access the Travis CI cache much earlier in the build process, and
>>> then creating the cache directory this late might cause confusion.
[...]
>> Is this behavior documented anywhere?
>> https://docs.travis-ci.com/user/caching#Arbitrary-directories doesn't
>> say anything about it.
>
> No, I'm afraid it isn't explicitly mentioned.
> I seem to remember an example implicitly relying on it, though, but
> can't find it anymore, so either misremembered or misunderstood one of
> the examples.
> OK, then I'll move this 'mkdir' to 'ci/lib-travisci.sh', to ensure that
> the cache directory exists in all build jobs.

Thanks.  Sounds good to me.

Another alternative would be to contact Travis CI folks to get the
behavior documented more clearly.  But here an early 'mkdir' doesn't
hurt, so it seems simplest to do it.

Sincerely,
Jonathan

Re: [PATCH v2 1/2] Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Use the collision detecting SHA-1 implementation by default even when
> NO_OPENSSL is set.
>
> Setting NO_OPENSSL=UnfortunatelyYes has implied BLK_SHA1=1 ever since
> the former was introduced in dd53c7ab29 (Support for NO_OPENSSL,
> 2005-07-29).  That implication should have been removed when the
> default SHA-1 implementation changed from OpenSSL to DC_SHA1 in
> e6b07da278 (Makefile: make DC_SHA1 the default, 2017-03-17).  Finish
> what that commit started by removing the BLK_SHA1 fallback setting so
> the default DC_SHA1 implementation will be used.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> Reviewed-by: Jonathan Nieder 

This is indeed
Reviewed-by: Jonathan Nieder 

Thanks for your patient work.

Re: [PATCH v2 2/2] Windows: stop supplying BLK_SHA1=YesPlease by default

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Using BLK_SHA1 in lieu of the OpenSSL routines was done in
> 9bccfcdbff ("Windows: use BLK_SHA1 again", 2009-10-22), since DC_SHA1
> is now the default for git in general it makes sense for Windows to
> use that too, this looks like something that was missed back in
> e6b07da278 ("Makefile: make DC_SHA1 the default", 2017-03-17).

micronit: the commas should be periods.

With or without such a tweak, this is indeed
Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> +++ b/Makefile
[...]
> -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
> -$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES 
> GIT-VERSION-FILE
> +PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
> +$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
>   $(QUIET_GEN)$(RM) $@ $@+ && \
> - INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
> instlibdir` && \
>   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>   sed -e '1{' \
>   -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
>   -e 'h' \
> - -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
> "'"$$INSTLIBDIR"'"));=' \
> + -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
> "'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \

This appears to have broken a build with INSTLIBDIR set.

 $ head -2 /usr/local/git/current/libexec/git-core/git-add--interactive
 #!/usr/bin/perl
 use lib (split(/:/, $ENV{GITPERLLIB} || 
":/Applications/Xcode.app/Contents/Developer/Library/Perl/5.@{[sub{use Config; 
$Config{api_version}}->()]}/darwin-thread-multi-2level" || 
"/usr/local/git/current/share/perl5"));

(forgive the hackiness there).

Is there a reason we don't do

INSTLIBDIR='$(perllibdir_SQ)' && \
INSTLIBDIR_EXTRA=... &&
INSTLIBDIR=...

and

use lib ... || "'"$$INSTLIBDIR"'"));=' \

?

Thanks,
Jonathan