Re: [RFC 0/7] transitioning to protocol v2

[Junio C Hamano]

Brandon Williams  writes:

> The best way to preserve functionality with old servers and clients would be 
> to
> communicate using the same end point but have the client send a bit of extra
> information with its initial request.  This extra information would need to be
> sent in such a way that old servers ignore it and operate normally (using
> protocol v1).  The client would then need to be able to look at a server's
> response to determine whether the server understands and is speaking v2 or has
> ignored the clients request to use a newer protocol and is speaking v1.

Good. I think the idle talk last round was for the server to tell
the v1 client "we are still doing the slow ls-remote comes first
protocol with this exchange, but just for future reference, you can
use the v2 endpoint with me", which was way less desirable (even
though it may be safer).

> Patches 1-5 enable a client to unconditionally send this back-channel
> information to a server.  This is done by sending a version number after a
> second NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://,
> and in an http header in http://, https://.  Patches 6-7 teach a client and
> upload-pack to send and recognize a request to use protocol v2.

All sounds sensible.

 - for git://, if you say you found a hole in the protocol to stuff
   this information, I simply believe you ;-)  Good job.

 - http:// and https:// should be a no-brainer as the HTTP headers
   give ample room to send information from the initiator side.

 - For ssh://, I do not think it is sane to assume that we can
   convince server operators to allow passing any random environment
   variable.  If the use of this specific variable turns out to be
   popular enough, however, and its benefit outweighs administrative
   burden, perhaps it is not impossible to convince them to allow
   AcceptEnv on this single variable.

Thanks.

Agent In (USA/Canada) Needed

[Shougang Group]

--
Shougang Group
45 Huagong Road Xinji City,
Hebei Province China.
webpage: www.shougang.com.cn

This is an official request for Professional/consultants who will stand as
our regional representative to run logistics on behalf of Shougang Group.
We are only looking for individual or company from USA and Canada. You
will be entitle to ten percent (10%) of every payment you receive from our
customers in your region on behalf of the company.

NOTE: You are not require to travel, all communications with our customers
will be through email or phone. All fees, including taxes will be handled
by us.

You can apply for this position by filling the information below and send
back to us;

*(1)Full Name:
*(2)Residential or office address:
*a.Town:
*b.State:
*c.Zip code:
*d.Country:
*(3)Current Job:
*(4)If you own a company, please state the company name:
*(5)Preferred Email:
*(6)Sex:
*(7)Age:
*(8)Telephone:

If you have a current job, you can still be part of our business, as
services for us will not bother with your hours of work.

Respectfully,
Mr. Zhu Jimin (Chief Executive Officer)
Shougang Group.

[RFC 4/7] t: fix ssh tests to cope with using '-o SendEnv=GIT_PROTOCOL'

[Brandon Williams]

Update some of our tests to cope with ssh being launched with the option
to send the protocol version.

Signed-off-by: Brandon Williams 
---
 t/lib-proto-disable.sh   |  1 +
 t/t5601-clone.sh | 10 +-
 t/t5602-clone-remote-exec.sh |  4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index 83babe57d..d19c88f96 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -194,6 +194,7 @@ setup_ssh_wrapper () {
test_expect_success 'setup ssh wrapper' '
write_script ssh-wrapper <<-\EOF &&
echo >&2 "ssh: $*"
+   shift; shift
host=$1; shift
cd "$TRASH_DIRECTORY/$host" &&
eval "$*"
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9c56f771b..7e65013c5 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -332,13 +332,13 @@ expect_ssh () {
1)
;;
2)
-   echo "ssh: $1 git-upload-pack '$2'"
+   echo "ssh: -o SendEnv=GIT_PROTOCOL $1 git-upload-pack 
'$2'"
;;
3)
-   echo "ssh: $1 $2 git-upload-pack '$3'"
+   echo "ssh: -o SendEnv=GIT_PROTOCOL $1 $2 
git-upload-pack '$3'"
;;
*)
-   echo "ssh: $1 $2 git-upload-pack '$3' $4"
+   echo "ssh: $1 -o SendEnv=GIT_PROTOCOL $2 $3 
git-upload-pack '$4'"
esac
} >"$TRASH_DIRECTORY/ssh-expect" &&
(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
@@ -390,7 +390,7 @@ test_expect_success 'double quoted plink.exe in 
GIT_SSH_COMMAND' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
-   expect_ssh "-v -P 123" myhost src
+   expect_ssh "-v" "-P 123" myhost src
 '
 
 SQ="'"
@@ -398,7 +398,7 @@ test_expect_success 'single quoted plink.exe in 
GIT_SSH_COMMAND' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
-   expect_ssh "-v -P 123" myhost src
+   expect_ssh "-v" "-P 123" myhost src
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index cbcceab9d..b0d80cadd 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh
@@ -13,14 +13,14 @@ test_expect_success setup '
 
 test_expect_success 'clone calls git upload-pack unqualified with no -u 
option' '
test_must_fail env GIT_SSH=./not_ssh git clone localhost:/path/to/repo 
junk &&
-   echo "localhost git-upload-pack '\''/path/to/repo'\''" >expected &&
+   echo "-o SendEnv=GIT_PROTOCOL localhost git-upload-pack 
'\''/path/to/repo'\''" >expected &&
test_cmp expected not_ssh_output
 '
 
 test_expect_success 'clone calls specified git upload-pack with -u option' '
test_must_fail env GIT_SSH=./not_ssh \
git clone -u ./something/bin/git-upload-pack 
localhost:/path/to/repo junk &&
-   echo "localhost ./something/bin/git-upload-pack '\''/path/to/repo'\''" 
>expected &&
+   echo "-o SendEnv=GIT_PROTOCOL localhost ./something/bin/git-upload-pack 
'\''/path/to/repo'\''" >expected &&
test_cmp expected not_ssh_output
 '
 
-- 
2.14.1.342.g6490525c54-goog

[RFC 5/7] http: send Git-Protocol-Version header

[Brandon Williams]

Tell a serve that protocol v2 can be used by sending an http header
indicating this.

Signed-off-by: Brandon Williams 
---
 http.c  | 7 +++
 t/t5551-http-fetch-smart.sh | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/http.c b/http.c
index fa8666a21..504a14a5a 100644
--- a/http.c
+++ b/http.c
@@ -896,6 +896,11 @@ static void set_from_env(const char **var, const char 
*envname)
*var = val;
 }
 
+static const char *get_version(void)
+{
+   return "Git-Protocol-Version: 2";
+}
+
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
char *low_speed_limit;
@@ -926,6 +931,8 @@ void http_init(struct remote *remote, const char *url, int 
proactive_auth)
if (remote)
var_override(_proxy_authmethod, 
remote->http_proxy_authmethod);
 
+   extra_http_headers = curl_slist_append(extra_http_headers, 
get_version());
+
pragma_header = curl_slist_append(http_copy_default_headers(),
"Pragma: no-cache");
no_pragma_header = curl_slist_append(http_copy_default_headers(),
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index a51b7e20d..ce13f2425 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -27,6 +27,7 @@ cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
 > Accept-Encoding: gzip
+> Git-Protocol-Version: 2
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
@@ -34,6 +35,7 @@ cat >exp < POST /smart/repo.git/git-upload-pack HTTP/1.1
 > Accept-Encoding: gzip
+> Git-Protocol-Version: 2
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
-- 
2.14.1.342.g6490525c54-goog

[RFC 2/7] pkt-line: add strbuf_packet_read

[Brandon Williams]

Add function which can be used to read the contents of a single pkt-line
into a strbuf.

Signed-off-by: Brandon Williams 
---
 pkt-line.c | 21 +
 pkt-line.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index cf98f371b..875524ab8 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -312,6 +312,27 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
return len;
 }
 
+int strbuf_packet_read(int fd_in, struct strbuf *sb_out, int options)
+{
+   int packet_len;
+   strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
+   packet_len = packet_read(fd_in, NULL, NULL,
+/*
+ * strbuf_grow() above always allocates one 
extra byte to
+ * store a '\0' at the end of the string. 
packet_read()
+ * writes a '\0' extra byte at the end, too. 
Let it know
+ * that there is already room for the extra 
byte.
+ */
+sb_out->buf, LARGE_PACKET_DATA_MAX+1,
+options);
+   if (packet_len < 0)
+   return packet_len;
+
+   sb_out->len = packet_len;
+
+   return sb_out->len;
+}
+
 static char *packet_read_line_generic(int fd,
  char **src, size_t *src_len,
  int *dst_len)
diff --git a/pkt-line.h b/pkt-line.h
index d9e9783b1..c24c4f290 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -65,6 +65,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, 
int fd_out);
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
*buffer, unsigned size, int options);
 
+int strbuf_packet_read(int fd_in, struct strbuf *sb_out, int options);
 /*
  * Convenience wrapper for packet_read that is not gentle, and sets the
  * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
-- 
2.14.1.342.g6490525c54-goog

[RFC 6/7] transport: teach client to recognize v2 server response

[Brandon Williams]

Teach a client to recognize that a server understand protocol v2 by
looking at the first pkt-line the server sends in response.  This is
done by looking for the response "version 2" sent by upload-pack.

Signed-off-by: Brandon Williams 
---
 builtin/fetch-pack.c |   4 +-
 builtin/send-pack.c  |   5 +-
 connect.c| 165 ++-
 remote-curl.c|   7 ++-
 remote.h |  22 ++-
 transport.c  |  60 ---
 6 files changed, 178 insertions(+), 85 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 366b9d13f..a2a5e1c73 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -52,6 +52,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct fetch_pack_args args;
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
+   struct remote_refs_scanner scanner;
 
packet_trace_identity("fetch-pack");
 
@@ -193,7 +194,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
if (!conn)
return args.diag_url ? 0 : 1;
}
-   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
+   remote_refs_scanner_init(, , 0, NULL, );
+   get_remote_heads(fd[0], NULL, 0, );
 
ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
 , pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fc4f0bb5f..92ec1f871 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -154,6 +154,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
int progress = -1;
int from_stdin = 0;
struct push_cas_option cas = {0};
+   struct remote_refs_scanner scanner;
 
struct option options[] = {
OPT__VERBOSITY(),
@@ -256,8 +257,8 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.verbose ? CONNECT_VERBOSE : 0);
}
 
-   get_remote_heads(fd[0], NULL, 0, _refs, REF_NORMAL,
-_have, );
+   remote_refs_scanner_init(, _refs, REF_NORMAL, 
_have, );
+   get_remote_heads(fd[0], NULL, 0, );
 
transport_verify_remote_names(nr_refspecs, refspecs);
 
diff --git a/connect.c b/connect.c
index d609267be..732b651d9 100644
--- a/connect.c
+++ b/connect.c
@@ -107,97 +107,124 @@ static void annotate_refs_with_symref_info(struct ref 
*ref)
string_list_clear(, 0);
 }
 
-/*
- * Read all the refs from the other end
- */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+void remote_refs_scanner_init(struct remote_refs_scanner *scanner,
  struct ref **list, unsigned int flags,
  struct oid_array *extra_have,
  struct oid_array *shallow_points)
 {
-   struct ref **orig_list = list;
+   memset(scanner, 0, sizeof(*scanner));
+
+   scanner->orig_list = list;
+   *list = NULL;
+   scanner->list = list;
+   scanner->flags = flags;
+   scanner->extra_have = extra_have;
+   scanner->shallow_points = shallow_points;
+}
+
+int process_ref(struct remote_refs_scanner *scanner,
+   const char *buffer, int len)
+{
+   struct ref *ref;
+   struct object_id old_oid;
+   const char *name;
+   int name_len;
+   const char *arg;
+   int ret = 1;
+
+   if (len < 0)
+   die_initial_contact(scanner->seen_response);
+
+   if (!len) {
+   ret = 0;
+   goto out;
+   }
+
+   if (len > 4 && skip_prefix(buffer, "ERR ", ))
+   die("remote error: %s", arg);
+
+   if (len == GIT_SHA1_HEXSZ + strlen("shallow ") &&
+   skip_prefix(buffer, "shallow ", )) {
+   if (get_oid_hex(arg, _oid))
+   die("protocol error: expected shallow sha-1, got '%s'", 
arg);
+   if (!scanner->shallow_points)
+   die("repository on the other end cannot be shallow");
+   oid_array_append(scanner->shallow_points, _oid);
+   goto out;
+   }
+
+   if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, _oid) ||
+   buffer[GIT_SHA1_HEXSZ] != ' ')
+   die("protocol error: expected sha/ref, got '%s'", buffer);
+   name = buffer + GIT_SHA1_HEXSZ + 1;
+
+   name_len = strlen(name);
+   if (len != name_len + GIT_SHA1_HEXSZ + 1) {
+   free(server_capabilities);
+   server_capabilities = xstrdup(name + name_len + 1);
+   }
+
+   if (scanner->extra_have && !strcmp(name, ".have")) {
+   oid_array_append(scanner->extra_have, _oid);
+   goto out;
+   }
+
+   if (!strcmp(name, "capabilities^{}")) {
+   if (scanner->seen_response)
+   die("protocol 

[RFC 1/7] pkt-line: add packet_write function

[Brandon Williams]

Add a function which can be used to write the contents of an arbitrary
buffer.  This makes it easy to build up data in a strbuf before writing
the packet.

Signed-off-by: Brandon Williams 
---
 pkt-line.c | 6 ++
 pkt-line.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 7db911957..cf98f371b 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -188,6 +188,12 @@ static int packet_write_gently(const int fd_out, const 
char *buf, size_t size)
return error("packet write failed");
 }
 
+void packet_write(const int fd_out, const char *buf, size_t size)
+{
+   if (packet_write_gently(fd_out, buf, size))
+   die_errno("packet write failed");
+}
+
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
va_list args;
diff --git a/pkt-line.h b/pkt-line.h
index 66ef610fc..d9e9783b1 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -22,6 +22,7 @@
 void packet_flush(int fd);
 void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format 
(printf, 2, 3)));
 void packet_buf_flush(struct strbuf *buf);
+void packet_write(const int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
-- 
2.14.1.342.g6490525c54-goog

[RFC 7/7] upload-pack: ack version 2

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 upload-pack.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index 7efff2fbf..0f853152f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1032,9 +1032,15 @@ static int upload_pack_config(const char *var, const 
char *value, void *unused)
return parse_hide_refs_config(var, value, "uploadpack");
 }
 
+void upload_pack_v2(void)
+{
+   packet_write_fmt(1, "%s\n", "version 2");
+}
+
 int cmd_main(int argc, const char **argv)
 {
const char *dir;
+   const char *version;
int strict = 0;
struct option options[] = {
OPT_BOOL(0, "stateless-rpc", _rpc,
@@ -1067,6 +1073,11 @@ int cmd_main(int argc, const char **argv)
die("'%s' does not appear to be a git repository", dir);
 
git_config(upload_pack_config, NULL);
+
+   version = getenv("GIT_PROTOCOL");
+   if (!strcmp(version, "2"))
+   upload_pack_v2();
+
upload_pack();
return 0;
 }
-- 
2.14.1.342.g6490525c54-goog

[RFC 3/7] protocol: tell server that the client understands v2

[Brandon Williams]

Teach the connection logic to tell a serve that it understands protocol
v2.  This is done in 2 different ways for the built in protocols.

1. git://
   A normal request is structured as "command path/to/repo\0host=..\0"
   and due to a bug in an old version of git-daemon 73bb33a94 (daemon:
   Strictly parse the "extra arg" part of the command, 2009-06-04) we
   aren't able to place any extra args (separated by NULs) besides the
   host.  In order to get around this limitation put protocol version
   information after a second NUL byte so the request is structured
   like: "command path/to/repo\0host=..\0\0version=2\0".  git-daemon can
   then parse out the version number and set GIT_PROTOCOL.

2. ssh://, file://
   Set GIT_PROTOCOL envvar with the desired protocol version.  The
   envvar can be sent across ssh by using '-o SendEnv=GIT_PROTOCOL' and
   having the server whitelist this envvar.

Signed-off-by: Brandon Williams 
---
 connect.c | 31 ++-
 daemon.c  | 28 +---
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/connect.c b/connect.c
index 49b28b83b..d609267be 100644
--- a/connect.c
+++ b/connect.c
@@ -793,6 +793,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
@@ -820,12 +821,23 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 * Note: Do not add any other headers here!  Doing so
 * will cause older git-daemon servers to crash.
 */
-   packet_write_fmt(fd[1],
-"%s %s%chost=%s%c",
-prog, path, 0,
-target_host, 0);
+   strbuf_addf(,
+   "%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 */
+   strbuf_addch(, '\0');
+   strbuf_addf(, "version=%d%c", 2, '\0');
+   /* subsequent supported versions can also be added */
+   strbuf_addf(, "version=%d%c", 3, '\0');
+
+   packet_write(fd[1], request.buf, request.len);
+
free(target_host);
+   strbuf_release();
} else {
+   const char *const *var;
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
 
@@ -837,7 +849,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
sq_quote_buf(, path);
 
/* remove repo-local variables from the environment */
-   conn->env = local_repo_env;
+   for (var = local_repo_env; *var; var++)
+   argv_array_push(>env_array, *var);
+
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
@@ -890,6 +904,12 @@ struct child_process *git_connect(int fd[2], const char 
*url,
}
 
argv_array_push(>args, ssh);
+
+   /* protocol v2! */
+   argv_array_push(>args, "-o");
+   argv_array_push(>args, "SendEnv=GIT_PROTOCOL");
+   argv_array_push(>env_array, "GIT_PROTOCOL=2");
+
if (flags & CONNECT_IPV4)
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)
@@ -904,6 +924,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, ssh_host);
} else {
transport_check_allowed("file");
+   argv_array_push(>env_array, "GIT_PROTOCOL=2");
}
argv_array_push(>args, cmd.buf);
 
diff --git a/daemon.c b/daemon.c
index 30747075f..76a7b2d64 100644
--- a/daemon.c
+++ b/daemon.c
@@ -574,7 +574,7 @@ static void canonicalize_client(struct strbuf *out, const 
char *in)
 /*
  * Read the host as supplied by the client connection.
  */
-static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
+static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {
char *val;
int vallen;
@@ -602,6 +602,22 @@ static void parse_host_arg(struct hostinfo *hi, char 
*extra_args, int buflen)
if (extra_args < end && *extra_args)
die("Invalid request");
}
+
+   return extra_args;
+}
+
+static void parse_extra_args(const char *extra_args, int 

[RFC 0/7] transitioning to protocol v2

[Brandon Williams]

Another version of Git's wire protocol is a topic that has been discussed and
attempted by many in the community over the years.  The biggest challenge, as
far as I understand, has been coming up with a transition plan to using the new
server without breaking existing clients and servers.  As such this RFC is
really only concerned with solidifying a transition plan.  Once it has been
decided how we can transition to a new protocol we can get into decided what
this new protocol would look like (though it would obviously eliminate the ref
advertisement ;).

The best way to preserve functionality with old servers and clients would be to
communicate using the same end point but have the client send a bit of extra
information with its initial request.  This extra information would need to be
sent in such a way that old servers ignore it and operate normally (using
protocol v1).  The client would then need to be able to look at a server's
response to determine whether the server understands and is speaking v2 or has
ignored the clients request to use a newer protocol and is speaking v1.

Patches 1-5 enable a client to unconditionally send this back-channel
information to a server.  This is done by sending a version number after a
second NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://,
and in an http header in http://, https://.  Patches 6-7 teach a client and
upload-pack to send and recognize a request to use protocol v2.

The biggest question I'm trying to answer is if these are reasonable ways with
which to communicate a request to a server to use a newer protocol, without
breaking current servers/clients.  As far as I've tested, with patches 1-5
applied I can still communicate with current servers without causing any
problems.

Any comments/discussion is welcome!

Brandon Williams (7):
  pkt-line: add packet_write function
  pkt-line: add strbuf_packet_read
  protocol: tell server that the client understands v2
  t: fix ssh tests to cope with using '-o SendEnv=GIT_PROTOCOL'
  http: send Git-Protocol-Version header
  transport: teach client to recognize v2 server response
  upload-pack: ack version 2

 builtin/fetch-pack.c |   4 +-
 builtin/send-pack.c  |   5 +-
 connect.c| 196 +++
 daemon.c |  28 ++-
 http.c   |   7 ++
 pkt-line.c   |  27 ++
 pkt-line.h   |   2 +
 remote-curl.c|   7 +-
 remote.h |  22 -
 t/lib-proto-disable.sh   |   1 +
 t/t5551-http-fetch-smart.sh  |   2 +
 t/t5601-clone.sh |  10 +--
 t/t5602-clone-remote-exec.sh |   4 +-
 transport.c  |  60 +++--
 upload-pack.c|  11 +++
 15 files changed, 286 insertions(+), 100 deletions(-)

-- 
2.14.1.342.g6490525c54-goog

Re: [PATCH v4 08/16] revision.c: use refs_for_each*() instead of for_each_*_submodule()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

So... the idea is that the caller gives a ref-store and tells these
functions to iterate over refs in it, and the existing submodule
related callers can prepare a ref-store for the submodule---that
way, refs.[ch] layer does not have to _care_ that the set of refs it
was asked to look at is for submodule processing.

Nice.  Very nice.

> @@ -2120,8 +2129,14 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
>  {
>   const char *arg = argv[0];
>   const char *optarg;
> + struct ref_store *refs;
>   int argcount;
>  
> + if (submodule) {
> + refs = get_submodule_ref_store(submodule);
> + } else
> + refs = get_main_ref_store();
> +

Re: [PATCH v4 07/16] refs: add refs_head_ref()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

... which does what?

Unlike refs_for_each_ref() and friends, this does not iterate.
It just uses the same function signature to make a single call
of fn on the "HEAD" ref.

Did I capture what it does right?

Thanks.

Re: [RFC 0/3] imap-send curl tunnelling support

[Daniel Stenberg]

On Thu, 24 Aug 2017, Jeff King wrote:

Oh good. While I have you here, have you given any thought to a curl handle 
that has two half-duplex file descriptors, rather than a single full-duplex 
socket? That would let us tunnel over pipes rather than worrying about the 
portability of socketpair().


I suspect it would be quite complicated, because I imagine that lots of 
internal bits of curl assume there's a single descriptor.


Yeah, it would take quite some surgery deep down in the heart of curl to 
implement something like that. It wouldn't call it impossible but it would 
take a certain level of determination and amount of time. I presume the 
descriptor-pair would be passed in via an API so it wouldn't affect the 
connect phase. We also have decent test coverage, making an overhaul like this 
a less scary thought - as if the existing tests say OK we can be fairly 
certain there aren't any major regressions...


(I may also have forgotten some tiny detail for the moment that makes it very 
hard.)



 / daniel.haxx.se (who landed the IMAP PREAUTH fix in curl)


Don't you land most of the fixes in curl? :)


I do, but I don't expect readers of the git list to know that!

--

 / daniel.haxx.se

Re: [RFC PATCH 1/2] send-email: fix garbage removal after address

[Junio C Hamano]

Matthieu Moy  writes:

> This is a followup over 9d33439 (send-email: only allow one address
> per body tag, 2017-02-20). The first iteration did allow writting
>
>   Cc:  # garbage
>
> but did so by matching the regex ([^>]*>?), i.e. stop after the first
> instance of '>'. However, it did not properly deal with
>
>   Cc: f...@example.com # garbage
>
> Fix this using a new function strip_garbage_one_address, which does
> essentially what the old ([^>]*>?) was doing, but dealing with more
> corner-cases. Since we've allowed
>
>   Cc: "Foo # Bar" 
>
> in previous versions, it makes sense to continue allowing it (but we
> still remove any garbage after it). OTOH, when an address is given
> without quoting, we just take the first word and ignore everything
> after.
>
> Signed-off-by: Matthieu Moy 
> ---
> Also available as: https://github.com/git/git/pull/398
>
>  git-send-email.perl   | 26 --
>  t/t9001-send-email.sh |  4 
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index fa6526986e..33a69ffe5d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1089,6 +1089,26 @@ sub sanitize_address {
>  
>  }
>  
> +sub strip_garbage_one_address {
> + my ($addr) = @_;
> + chomp $addr;
> + if ($addr =~ /^(("[^"]*"|[^"<]*)? *<[^>]*>).*/) {
> + # "Foo Bar"  [possibly garbage here]
> + # Foo Bar  [possibly garbage here]
> + return $1;
> + }
> + if ($addr =~ /^(<[^>]*>).*/) {
> + #  [possibly garbage here]
> + # if garbage contains other addresses, they are ignored.
> + return $1;
> + }

Isn't this already covered by the first one, which allows an
optional "something", followed by an optional run of SPs, in front
of this exact pattern, so the case where the optional "something"
does not appear and the number of optional SP is zero would exactly
match the one this pattern is meant to cover.

> + if ($addr =~ /^([^"#,\s]*)/) {
> + # address without quoting: remove anything after the address
> + return $1;
> + }
> + return $addr;
> +}

By the way, these three regexps smell like they were written
specifically to cover three cases you care about (perhaps the ones
in your proposed log message), but what will be our response when
somebody else comes next time to us and says that their favourite
formatting of "Cc:" line is not covered by these rules?  

Will we add yet another pattern?  Where will it end?  There will be
a point where we instead start telling them to update the convention
of their project so that it will be covered by one of the patterns
we have already developed, I would imagine.

So, from that point of view, I, with devil's advocate hat on, wonder
why we are not saying

"Cc: s...@k.org # cruft"?  Use "Cc:  # cruft" instead
and you'd be fine.

right now, without this patch.

I do not _mind_ us trying to be extra nice for a while, and I
certainly do not mind _this_ particular patch that gives us a single
helper function that future "here is another way to spell cruft"
rules can go, but I feel that there should be some line that lets us
say that we've done enough.

Thanks.

Re: [Patch size_t V3 11/19] Use size_t for config parsing

[Johannes Sixt]

Am 16.08.2017 um 22:16 schrieb Martin Koegler:
> +int git_parse_size_t(const char *value, size_t *ret)
> +{
> + uintmax_t tmp;
> + if (!git_parse_unsigned(value, , 
> maximum_signed_value_of_type(size_t)))
> + return 0;
> + *ret = tmp;
> + return 1;
> +}
> +

I think this requires the following on top:

diff --git a/config.c b/config.c
index 81d46602f9..b3075aa1c4 100644
--- a/config.c
+++ b/config.c
@@ -866,7 +866,7 @@ static int git_parse_ssize_t(const char *value, ssize_t 
*ret)
 int git_parse_size_t(const char *value, size_t *ret)
 {
uintmax_t tmp;
-   if (!git_parse_unsigned(value, , 
maximum_signed_value_of_type(size_t)))
+   if (!git_parse_unsigned(value, , 
maximum_unsigned_value_of_type(size_t)))
return 0;
*ret = tmp;
return 1;

Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message

[Junio C Hamano]

Kaartic Sivaraam  writes:

>  As has been noted by Junio, 
>
>  "It would be a backward incompatible tightening of the established
>  rule, but it may not be a bad change."
>
>  The "It" above refers to this change. Expecting comments from people to 
> ensure
>  this change isn't a bad one.

FWIW, I am fairly neutral; I do not mind accepting this change if
other people are supportive, but I do not miss this patch if we end
up not applying it at all.  The latter is easier for me as we do not
have to worry about breaking people's scripts and tools used in
their established workflows at all.

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Junio C Hamano]

Sonny Michaud  writes:

> diff --git a/wt-status.c b/wt-status.c
>  
> index 77c27c511..651bb01f0 100644 
>  
> --- a/wt-status.c 
>  
> +++ b/wt-status.c 
>  
> @@ -1827,6 +1827,15 @@ static void wt_shortstatus_print_tracking(struct 
> wt_status *s) 
>  
> fputc(s->null_termination ? '\0' : '\n', s->fp);  
>  
>  }
>  
>   
>  
> +static void wt_shortstatus_print_stash_summary(struct wt_status *s)  
>  
> +{
>  
> +   int stash_count = 0;  
>  
> + 
>  
> +   for_each_reflog_ent("refs/stash", stash_count_refs, _count);
>  

A singleton instance of this in wt_longstatus_print_stash_summary()
thing was OK, but let's not duplicate and spread the badness.  Have
a simple there-liner helper function "static int stash_count(void);"
that does the above and returns the stash_count, and use it from
both places.

> +   if (stash_count > 0)  
>  
> +color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## Stash entries: %d", 
> stash_count); 
>

That's a funny way to indent (dedent?) a body of an if() statement.

Don't scripts that read this output (I notice that this is also
called by wt_porcelain_print() function) expect that entries that
are led by "##" are about the current branch and its tracking
information?  

This patch would break these script by adding this new line using
the same "##" leader.

RE: TREAT URGENT !

[Dr. Mallon Liam]

Hello.

I am Dr. Mallon Liam, an Engineer with ExxonMobil in United Kingdom 
http://www.exxonmobil.co.uk). I got your contact address from an associate 
working with the British Export Promotion Council. Before I continue, let me 
stress that I am NOT contacting you for financial assistance.

I am in search of a business partner to assist us in the transfer of 
(GBP35million) and subsequent investment in your country.

If you decide to participate in this business, 10% of the total amount of 
(GBP35million) will be for you for your participation and sincere assistance. I 
shall communicate further details as soon as i received your response.

I look forward to your partnership.

Best Regards,
Dr. Mallon Liam.
Fawley Refinery, Fawley, Southampton SO45 1TX, 
United Kingdom
Ph: +(44) 203 322 4197

[GSoC][PATCH v3 4/4] submodule: port submodule subcommand 'status' from shell to C

[Prathamesh Chavan]

This aims to make git-submodule 'status' a built-in. Hence, the function
cmd_status() is ported from shell to C. This is done by introducing
three functions: module_status(), submodule_status() and print_status().

The function module_status() acts as the front-end of the subcommand.
It parses subcommand's options and then calls the function
module_list_compute() for computing the list of submodules. Then
this functions calls for_each_listed_submodule() looping through the
list obtained.

Then for_each_listed_submodule() calls submodule_status() for each of the
submodule in its list. The function submodule_status() is responsible
for generating the status each submodule it is called for, and
then calls print_status().

Finally, the function print_status() handles the printing of submodule's
status.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 156 
 git-submodule.sh|  49 +-
 2 files changed, 157 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6ea6408c2..577494e31 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -561,6 +561,161 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct status_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+   unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+const struct object_id *oid, const char *displaypath)
+{
+   if (info->quiet)
+   return;
+
+   printf("%c%s %s", state, oid_to_hex(oid), displaypath);
+
+   if (state == ' ' || state == '+') {
+   struct argv_array get_rev_args = ARGV_ARRAY_INIT;
+
+   argv_array_pushl(_rev_args, "get-rev-name",
+path, oid_to_hex(oid), NULL);
+   get_rev_name(get_rev_args.argc, get_rev_args.argv,
+info->prefix);
+
+   argv_array_clear(_rev_args);
+   } else {
+   printf("\n");
+   }
+}
+
+static int handle_submodule_head_ref(const char *refname,
+const struct object_id *oid, int flags,
+void *cb_data)
+{
+   struct object_id *output = cb_data;
+   if (oid)
+   oidcpy(output, oid);
+
+   return 0;
+}
+
+static void status_submodule(const struct cache_entry *list_item, void 
*cb_data)
+{
+   struct status_cb *info = cb_data;
+   char *displaypath;
+   struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+   if (!submodule_from_path(_oid, list_item->name))
+   die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
+ list_item->name);
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (ce_stage(list_item)) {
+   print_status(info, 'U', list_item->name,
+_oid, displaypath);
+   goto cleanup;
+   }
+
+   if (!is_submodule_active(the_repository, list_item->name)) {
+   print_status(info, '-', list_item->name, _item->oid,
+displaypath);
+   goto cleanup;
+   }
+
+   argv_array_pushl(_files_args, "diff-files",
+"--ignore-submodules=dirty", "--quiet", "--",
+list_item->name, NULL);
+
+   if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+   info->prefix)) {
+   print_status(info, ' ', list_item->name, _item->oid,
+displaypath);
+   } else {
+   if (!info->cached) {
+   struct object_id oid;
+
+   if (head_ref_submodule(list_item->name,
+  handle_submodule_head_ref, ))
+   die(_("could not resolve HEAD ref inside the"
+ "submodule '%s'"), list_item->name);
+
+   print_status(info, '+', list_item->name, ,
+displaypath);
+   } else {
+   print_status(info, '+', list_item->name,
+_item->oid, displaypath);
+   }
+   }
+
+   if (info->recursive) {
+   struct child_process cpr = CHILD_PROCESS_INIT;
+
+   cpr.git_cmd = 1;
+   cpr.dir = list_item->name;
+   prepare_submodule_repo_env(_array);
+
+   argv_array_pushl(, "--super-prefix", displaypath,
+

[GSoC][PATCH v3 2/4] submodule--helper: introduce for_each_listed_submodule()

[Prathamesh Chavan]

Introduce function for_each_listed_submodule() and replace a loop
in module_init() with a call to it.

The new function will also be used in other parts of the
system in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 39 +--
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e666f84ba..8cd81b144 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -14,6 +14,9 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
+ void *cb_data);
+
 static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static void init_submodule(const char *path, const char *prefix, int quiet)
+static void for_each_listed_submodule(const struct module_list *list,
+ each_submodule_fn fn, void *cb_data)
+{
+   int i;
+   for (i = 0; i < list->nr; i++)
+   fn(list->entries[i], cb_data);
+}
+
+struct init_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+};
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const struct cache_entry *list_item, void *cb_data)
 {
+   struct init_cb *info = cb_data;
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
char *upd = NULL, *url = NULL, *displaypath;
 
-   /* Only loads from .gitmodules, no overlay with .git/config */
-   gitmodules_config();
-   displaypath = get_submodule_displaypath(path, prefix);
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(_oid, list_item->name);
 
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -375,7 +391,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 *
 * Set active flag for the submodule being initialized
 */
-   if (!is_submodule_active(the_repository, path)) {
+   if (!is_submodule_active(the_repository, list_item->name)) {
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
strbuf_reset();
@@ -417,7 +433,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
if (git_config_set_gently(sb.buf, url))
die(_("Failed to register url for submodule path '%s'"),
displaypath);
-   if (!quiet)
+   if (!info->quiet)
fprintf(stderr,
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
@@ -446,10 +462,10 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
 static int module_init(int argc, const char **argv, const char *prefix)
 {
+   struct init_cb info = INIT_CB_INIT;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
-   int i;
 
struct option module_init_options[] = {
OPT__QUIET(, N_("Suppress output for initializing a 
submodule")),
@@ -474,8 +490,11 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
if (!argc && git_config_get_value_multi("submodule.active"))
module_list_active();
 
-   for (i = 0; i < list.nr; i++)
-   init_submodule(list.entries[i]->name, prefix, quiet);
+   info.prefix = prefix;
+   info.quiet = !!quiet;
+
+   gitmodules_config();
+   for_each_listed_submodule(, init_submodule, );
 
return 0;
 }
-- 
2.13.0

[GSoC][PATCH v3 3/4] submodule: port set_name_rev() from shell to C

[Prathamesh Chavan]

Function set_name_rev() is ported from git-submodule to the
submodule--helper builtin. The function compute_rev_name() generates the
value of the revision name as required.
The function get_rev_name() calls compute_rev_name() and receives the
revision name, and later handles its formating and printing.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 63 +
 git-submodule.sh| 16 ++--
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8cd81b144..6ea6408c2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, 
const char *prefix)
}
 }
 
+static char *compute_rev_name(const char *sub_path, const char* object_id)
+{
+   struct strbuf sb = STRBUF_INIT;
+   const char ***d;
+
+   static const char *describe_bare[] = {
+   NULL
+   };
+
+   static const char *describe_tags[] = {
+   "--tags", NULL
+   };
+
+   static const char *describe_contains[] = {
+   "--contains", NULL
+   };
+
+   static const char *describe_all_always[] = {
+   "--all", "--always", NULL
+   };
+
+   static const char **describe_argv[] = {
+   describe_bare, describe_tags, describe_contains,
+   describe_all_always, NULL
+   };
+
+   for (d = describe_argv; *d; d++) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env(_array);
+   cp.dir = sub_path;
+   cp.git_cmd = 1;
+   cp.no_stderr = 1;
+
+   argv_array_push(, "describe");
+   argv_array_pushv(, *d);
+   argv_array_push(, object_id);
+
+   if (!capture_command(, , 0) && sb.len) {
+   strbuf_strip_suffix(, "\n");
+   return strbuf_detach(, NULL);
+   }
+   }
+
+   strbuf_release();
+   return NULL;
+}
+
+static int get_rev_name(int argc, const char **argv, const char *prefix)
+{
+   char *revname;
+   if (argc != 3)
+   die("get-rev-name only accepts two arguments:  ");
+
+   revname = compute_rev_name(argv[1], argv[2]);
+   if (revname && revname[0])
+   printf(" (%s)", revname);
+   printf("\n");
+
+   free(revname);
+   return 0;
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -1243,6 +1305,7 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"get-rev-name", get_rev_name, 0},
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..91f043ec6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -759,18 +759,6 @@ cmd_update()
}
 }
 
-set_name_rev () {
-   revname=$( (
-   sanitize_submodule_env
-   cd "$1" && {
-   git describe "$2" 2>/dev/null ||
-   git describe --tags "$2" 2>/dev/null ||
-   git describe --contains "$2" 2>/dev/null ||
-   git describe --all --always "$2"
-   }
-   ) )
-   test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1042,14 +1030,14 @@ cmd_status()
fi
if git diff-files --ignore-submodules=dirty --quiet -- 
"$sm_path"
then
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper get-rev-name "$sm_path" 
"$sha1")
say " $sha1 $displaypath$revname"
else
if test -z "$cached"
then
sha1=$(sanitize_submodule_env; cd "$sm_path" && 
git rev-parse --verify HEAD)
fi
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper get-rev-name "$sm_path" 
"$sha1")
say "+$sha1 $displaypath$revname"
fi
 
-- 
2.13.0

[GSoC][PATCH v3 1/4] submodule--helper: introduce get_submodule_displaypath()

[Prathamesh Chavan]

Introduce function get_submodule_displaypath() to replace the code
occurring in submodule_init() for generating displaypath of the
submodule with a call to it.

This new function will also be used in other parts of the system
in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 38 +-
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 84562ec83..e666f84ba 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char 
**argv, const char *pr
return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+   const char *super_prefix = get_super_prefix();
+
+   if (prefix && super_prefix) {
+   BUG("cannot have prefix '%s' and superprefix '%s'",
+   prefix, super_prefix);
+   } else if (prefix) {
+   struct strbuf sb = STRBUF_INIT;
+   char *displaypath = xstrdup(relative_path(path, prefix, ));
+   strbuf_release();
+   return displaypath;
+   } else if (super_prefix) {
+   int len = strlen(super_prefix);
+   const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s" 
: "%s/%s";
+
+   return xstrfmt(format, super_prefix, path);
+   } else {
+   return xstrdup(path);
+   }
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -339,16 +361,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
-
-   if (prefix && get_super_prefix())
-   die("BUG: cannot have prefix and superprefix");
-   else if (prefix)
-   displaypath = xstrdup(relative_path(path, prefix, ));
-   else if (get_super_prefix()) {
-   strbuf_addf(, "%s%s", get_super_prefix(), path);
-   displaypath = strbuf_detach(, NULL);
-   } else
-   displaypath = xstrdup(path);
+   displaypath = get_submodule_displaypath(path, prefix);
 
sub = submodule_from_path(_oid, path);
 
@@ -363,9 +376,9 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 * Set active flag for the submodule being initialized
 */
if (!is_submodule_active(the_repository, path)) {
-   strbuf_reset();
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
+   strbuf_reset();
}
 
/*
@@ -373,7 +386,6 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 * To look up the url in .git/config, we must not fall back to
 * .gitmodules, so look it up directly.
 */
-   strbuf_reset();
strbuf_addf(, "submodule.%s.url", sub->name);
if (git_config_get_string(sb.buf, )) {
if (!sub->url)
@@ -410,9 +422,9 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
}
+   strbuf_reset();
 
/* Copy "update" setting when it is not set yet */
-   strbuf_reset();
strbuf_addf(, "submodule.%s.update", sub->name);
if (git_config_get_string(sb.buf, ) &&
sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
-- 
2.13.0

[GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules

[Prathamesh Chavan]

Changes in v3:

* The name of the iterator function for_each_submodule() was changed
  to the for_each_listed_submodule(), as the function fits the naming
  pattern for_each_Y_X(), as here we iterate over group of listed
  submodules (X) which are listed (Y) by the function module_list_compute()
* The name of the call back function type for the above pattern
  for_each_Y_X() is each_X_fn. Hence, this pattern was followed and
  the name of the call back function type for for_each_listed_submodule()
  was changed from submodule_list_func_t to each_submodule_fn.

As before you can find this series at: 
https://github.com/pratham-pc/git/commits/week-14-1

And its build report is available at: 
https://travis-ci.org/pratham-pc/git/builds/
Branch: week-14-1
Build #164

Prathamesh Chavan (4):
  submodule--helper: introduce get_submodule_displaypath()
  submodule--helper: introduce for_each_listed_submodule()
  submodule: port set_name_rev() from shell to C
  submodule: port submodule subcommand 'status' from shell to C

 builtin/submodule--helper.c | 294 
 git-submodule.sh|  61 +
 2 files changed, 273 insertions(+), 82 deletions(-)

-- 
2.13.0

Re: [PATCH v1] convert: display progress for filtered objects that have been delayed

[Junio C Hamano]

Lars Schneider  writes:

> In 2841e8f ("convert: add "status=delayed" to filter process protocol",
> 2017-06-30) we taught the filter process protocol to delayed responses.
> These responses are processed after the "Checking out files" phase.
> If the processing takes noticeable time, then the user might think Git
> is stuck.
>
> Display the progress of the delayed responses to let the user know that
> Git is still processing objects. This works very well for objects that
> can be filtered quickly. If filtering of an individual object takes
> noticeable time, then the user might still think that Git is stuck.
> However, in that case the user would at least know what Git is doing.
>
> It would be technical more correct to display "Checking out files whose
> content filtering has been delayed". For brevity we only print
> "Filtering content".
>
> The finish_delayed_checkout() call was moved below the stop_progress()
> call in unpack-trees.c to ensure that the "Checking out files" progress
> is properly stopped before the "Filtering content" progress starts in
> finish_delayed_checkout().
>
> Signed-off-by: Lars Schneider 
> Suggested-by: Taylor Blau 
> ---

Makes sense.  The only thing that made me wonder was if we want the
change in unpack-trees.c in this patch.  After all, the procedure to
finish up the delayed checkout _is_ a part of the work need to be
done to populate the working tree files, so stopping the progress
before feels somewhat wrong at the phylosophical level.

I think our output cannot express nested progress bars, and I think
that is the reason why this patch tweaks unpack-trees.c; so I am
fine with the end result (and that is why I said "made me wonder
was", not "makes me wonder", the latter would imply "this we might
want fix before applying", but I do not think we want to change
anything this patch does to unpack-trees.c in this case).

The delayed progress API is being simplified so I'll probably do a
bit of evil merge while merging this to 'pu'.

Thanks.

>  entry.c| 16 +++-
>  unpack-trees.c |  2 +-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 65458f07a4..1d1a09f47e 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -3,6 +3,7 @@
>  #include "dir.h"
>  #include "streaming.h"
>  #include "submodule.h"
> +#include "progress.h"
>  
>  static void create_directories(const char *path, int path_len,
>  const struct checkout *state)
> @@ -161,16 +162,23 @@ static int remove_available_paths(struct 
> string_list_item *item, void *cb_data)
>  int finish_delayed_checkout(struct checkout *state)
>  {
>   int errs = 0;
> + unsigned delayed_object_count;
> + off_t filtered_bytes = 0;
>   struct string_list_item *filter, *path;
> + struct progress *progress;
>   struct delayed_checkout *dco = state->delayed_checkout;
>  
>   if (!state->delayed_checkout)
>   return errs;
>  
>   dco->state = CE_RETRY;
> + delayed_object_count = dco->paths.nr;
> + progress = start_progress_delay(
> + _("Filtering content"), delayed_object_count, 50, 1);
>   while (dco->filters.nr > 0) {
>   for_each_string_list_item(filter, >filters) {
>   struct string_list available_paths = 
> STRING_LIST_INIT_NODUP;
> + display_progress(progress, delayed_object_count - 
> dco->paths.nr);
>  
>   if (!async_query_available_blobs(filter->string, 
> _paths)) {
>   /* Filter reported an error */
> @@ -216,11 +224,17 @@ int finish_delayed_checkout(struct checkout *state)
>   }
>   ce = index_file_exists(state->istate, 
> path->string,
>  strlen(path->string), 0);
> - errs |= (ce ? checkout_entry(ce, state, NULL) : 
> 1);
> + if (ce) {
> + errs |= checkout_entry(ce, state, NULL);
> + filtered_bytes += 
> ce->ce_stat_data.sd_size;
> + display_throughput(progress, 
> filtered_bytes);
> + } else
> + errs = 1;
>   }
>   }
>   string_list_remove_empty_items(>filters, 0);
>   }
> + stop_progress();
>   string_list_clear(>filters, 0);
>  
>   /* At this point we should not have any delayed paths anymore. */
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 862cfce661..90fb270d77 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -395,8 +395,8 @@ static int check_updates(struct unpack_trees_options *o)
>   }
>   }
>   }
> - errs |= finish_delayed_checkout();
>   

Re: What's cooking in git.git (Aug 2017, #05; Tue, 22)

[Junio C Hamano]

Lars Schneider  writes:

> On 22 Aug 2017, at 21:56, Junio C Hamano  wrote:
>
>> Here are the topics that have been cooking.  Commits prefixed with
>> '-' are only in 'pu' (proposed updates) while commits prefixed with
>> '+' are in 'next'.  The ones marked with '.' do not appear in any of
>> the integration branches, but I am still holding onto them.
>> 
>> The second batch of topics are in.  This cycle is going a bit slower
>> than the previous one (as of mid-week #3 of this cycle, we have
>> about 200 patches on 'master' since v2.14, compared to about 300
>> patches in the cycle towards v2.14 at a similar point in the cycle),
>> but hopefully we can catch up eventually.  
>> 
>> I am planning to be offline most of the next week, by the way.
>> 
>> You can find the changes described here in the integration branches
>> of the repositories listed at
>> 
>>http://git-blame.blogspot.com/p/git-public-repositories.html
>> 
>> --
>> [Graduated to "master"]
>> 
>
> Hi Junio,
>
> just in case this got lost: I posted a patch with an improvement to 
> 2841e8f ("convert: add "status=delayed" to filter process protocol", 
> 2017-06-30) which was merged to master in the beginning of 2.15.
>
> https://public-inbox.org/git/20170820154720.32259-1-larsxschnei...@gmail.com/

Thanks for pinging, but next time ping the thread itself if it is
about something that is not in What's cooking report you are
responding to.

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Junio C Hamano]

Sonny Michaud  writes:

> On 08/22/2017 11:25 AM, Sonny Michaud wrote:
>> I am just bumping this thread; I presume there is something amiss
>> with my submissions, so if someone can let me know how to fix any
>> issues, I will gladly re-submit the patch.
>>
>> Thanks!
>>
>> - Sonny
>
> Bumping again for visibility.

It's probably not due to "something amiss" but nobody was interested
in what the patch tries to achieve and had nothing useful to say on
it (hence everybody was quiet).

Some other people may have skipped seeing only [0/1] without [1/1],
as reading only the cover letter without an accompanying patch will
be waste of their time for busy reviewers---by the time [1/1] comes
it is likely they forgot what [0/1] said and they need to go back
and read it again, so they would likely have said "I'll read both
when [1/1] appears on the list", moved on, and then have forgotten
about it.  I am in that camp.

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Martin Ågren]

On 24 August 2017 at 20:29, Brandon Casey  wrote:
> On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano  wrote:
>> Brandon Casey  writes:
>>
>>> Ah, you probably meant something like this:
>>>
>>>const char strbuf_slopbuf = '\0';
>>>
>>> which gcc will apparently place in the read-only segment.  I did not know 
>>> that.
>>
>> Yes but I highly suspect that it would be very compiler dependent
>> and not something the language lawyers would recommend us to rely
>> on.
>
> I think compilers may have the option of placing variables that are
> explicitly initialized to zero in the bss segment too, in addition to
> those that are not explicitly initialized.  So I agree that no one
> should write code that depends on their variables being placed in one
> segment or the other, but I could see someone using this behavior as
> an additional safety check; kind of a free assert, aside from the
> additional space in the .rodata segment.
>
>> My response was primarily to answer "why?" with "because we did not
>> bother".  The above is a mere tangent, i.e. "multiple copies of
>> empty strings is a horrible implementation (and there would be a way
>> to do it with a single instance)".
>
> Merely adding const to our current strbuf_slopbuf declaration does not
> buy us anything since it will be allocated in r/w memory.  i.e. it
> would still be possible to modify it via the buf member of strbuf.  So
> you can't just do this:
>
>const char strbuf_slopbuf[1];
>
> That's pretty much equivalent to what we currently have since it only
> restricts modifying the contents of strbuf_slopbuf directly through
> the strbuf_slopbuf variable, but it does not restrict modifying it
> through a pointer to that object.
>
> Until yesterday, I was under the impression that the only way to
> access data in the rodata segment was through a constant literal.  So
> my initial thought was that we could do something like:
>
>const char * const strbuf_slopbuf = "";
>
> ..but that variable cannot be used in a static assignment like:
>
>struct strbuf foo = {0, 0, (char*) strbuf_slopbuf};
>
> So it seemed like our only option was to use a literal "" everywhere
> instead of a slopbuf variable _if_ we wanted to have the guarantee
> that our "slopbuf" could not be modified.
>
> But what I learned yesterday, is that at least gcc/clang will place
> the entire variable in the rodata segment if the variable is both
> marked const _and_ initialized.
>
> i.e. this will be allocated in the .rodata segment:
>
>const char strbuf_slopbuf[1] = "";
>
>>>#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = (char*) 
>>> _slopbuf }
>>>
>>> respectively.  Yeah, that's definitely preferable to a macro.
>>> Something similar could be done in object.c.
>>
>> What is the main objective for doing this change?  The "make sure we
>> do not write into that slopbuf" assert() bothers you and you want to
>> replace it with an address in the read-only segment?
>
> Actually nothing about the patch bothers me.  The point of that patch
> is to make sure we don't accidentally modify the slopbuf.  I was just
> looking for a way for the compiler to help out and wondering if there
> was a reason we didn't attempt to do so in the first place.
>
> I think the main takeaway here is that I learned something yesterday
> :-)  I didn't actually intend to submit a patch for any of this, but
> if anything useful came out of the discussion I thought Martin may
> incorporate it into his patch if he wanted to.

Thanks for interesting information. I also learned something new. :-)

My first thought was, well, maybe someone writes '\0' to sb.buf[len].
That should intuitively be a no-op and "ok", but the documentation
actually only says that it's safe to write to positions 0 .. len-1, so
sb.buf[len] is supposedly not safe (no-op or not). Maybe a degenerate
and rarely tested case of otherwise sane code could end up writing '\0'
to slopbuf[0]. (Arguably strbuf_setlen should have been used instead.)

I can see the value of placing the slopbuf in read-only memory, but I
think that would be a follow-up patch with its own pros and cons.

Martin

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Junio C Hamano]

Jeff King  writes:

> On Thu, Aug 24, 2017 at 12:27:52AM -0700, Jacob Keller wrote:
>
>> > For the sneaker-net case, you are much better off generating a single
>> > pack and then using "split" and "cat" to reconstruct it on the other end
>> > Not that I think we should go into such detail in the manpage, but I
>> > have to wonder if --max-pack-size has outlived its usefulness. The only
>> > use case I can think of is a filesystem that cannot hold files larger
>> > than N bytes.
>> 
>> Is it possible to detect on the file system that we can't store files
>> that large, and remove the option, while enabling it only when we
>> detect the filesystem is unable to store large files?
>
> I'm not sure how easy it would be to do such a check. But even if it
> was, I'm not sure that buys us much. We'd still carry the code. We could
> in theory remove the option, simplifying the interface. But that removes
> the possibility of somebody wanting to stage the smaller packfiles on a
> more capable filesystem in preparation for moving them to the
> more-limited one.

I agree that it would not help anybody to _disable_ --max-pack-size
feature for those on certain filesystems, but it _might_ make sense
to automatically _enable_ it (and set it to the maximum number) when
your destination filesystem is limited.

Even in that case, failing with an error code from the filesystem
and then asking the user to redo with --max-pack-size specified
wouldn't be the end of the world, though.

Re: [PATCH] Fix branch renaming not updating HEADs correctly

[Nish Aravamudan]

On 24.08.2017 [17:41:24 +0700], Nguyễn Thái Ngọc Duy wrote:
> There are two bugs that sort of work together and cause problems. Let's
> start with one in replace_each_worktree_head_symref.



> Correct add_head_info(), remove RESOLVE_REF_READING flag. With the flag
> gone, we should always return good "head_ref" in orphan checkouts (either
> temporary or permanent). With good head_ref, things start to work again.
> 
> Noticed-by: Nish Aravamudan 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
Tested-by: Nish Aravamudan 

Thank you for the quick turnaround on the patch!

-Nish
-- 
Nishanth Aravamudan
Ubuntu Server
Canonical Ltd

Re: [PATCH/RFC] push: anonymize URL in error output

[Ivan Vyshnevskyi]

On 23/08/17 18:58, Jeff King wrote:
> On Wed, Aug 23, 2017 at 12:49:29PM +0300, Ivan Vyshnevskyi wrote:
> 
>> Commits 47abd85 (fetch: Strip usernames from url's before storing them,
>> 2009-04-17) and later 882d49c (push: anonymize URL in status output,
>> 2016-07-14) made fetch and push strip the authentication part of the
>> remote URLs when used in the merge-commit messages or status outputs.
>> The URLs that are part of the error messages were not anonymized.
>>
>> A commonly used pattern for storing artifacts from a build server in a
>> remote repository utilizes a "secure" environment variable with
>> credentials to embed them in the URL and execute a push. Given enough
>> runs, an intermittent network failure will cause a push to fail, leaving
>> a non-anonymized URL in the build log.
>>
>> To prevent that, reuse the same anonymizing function to scrub
>> credentials from URL in the push error output.
> 
> This makes sense. I suspect that most errors we output should be using
> the anonymized URL. Did you poke around for other calls?
Yes, I tried to check and unfortunately there are couple of places with
possible leaks:
* 'discover_refs()' in remote-curl.c when there's a HTTP error (see a
real-life scenario with an authz error in my response to Lars) -- is it
ok to include transport.h just to use one function or is there a cleaner
way?
* 'setup_push_upstream()' in push.c when a command doesn't have a branch
names (haven't saw problems with this in the wild, but could occur
during the CI setup) -- for this one, probably anonymization should
happen when the 'remote->name' field is set in the 'make_remote()'; same
question though, is it ok to include transport.h here?

Also there's an case of verbose output: I'm not sure I should change it,
but it does print out the non-anonymized URLs at least during push.
> 
> The general structure of the patch looks good, but I have a few minor
> comments below.
> 
>> Not sure how much of the background should be included in the commit message.
>> The "commonly used pattern" I mention could be found in the myriad of
>> online tutorials and looks something like this:
> 
> My knee-jerk reaction is if it's worth writing after the dashes, it's
> worth putting in the commit message.
> 
> However, in the case I think it is OK as-is (the motivation of "we
> already avoid leaking auth info to stdout, so we should do the same for
> error messages" seems self-contained and reasonable)
Well, I tend to be wordy, and most of the commit messages I saw were
rather short, so decided to split. Wonder, if maybe example command
should be included without the rest of it. Would it be useful?
> 
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 03846e837..59f3bc975 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -336,7 +336,7 @@ static int push_with_options(struct transport 
>> *transport, int flags)
>>  err = transport_push(transport, refspec_nr, refspec, flags,
>>   _reasons);
>>  if (err != 0)
>> -error(_("failed to push some refs to '%s'"), transport->url);
>> +error(_("failed to push some refs to '%s'"), 
>> transport_anonymize_url(transport->url));
> 
> This leaks the return value. That's probably not a _huge_ deal since the
> program is likely to exit, but it's a bad pattern. I wonder if we should
> be setting up transport->anonymous_url preemptively, and just let its
> memory belong to the transport struct.
Ah. Thanks! I knew I'd fail in the memory management even with the
one-line patch. :)

About 'transport->anonymous_url': not sure if it's worth it. There are
four calls to 'transport_anonymize_url' right now and it looks like the
new one in my patch is the first that has a transport struct instance
near by. The next likely candidate for update 'discover_refs()' also
gets the url as an argument.
> 
>> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
>> index d38bf3247..0b6fb6252 100755
>> --- a/t/t5541-http-push-smart.sh
>> +++ b/t/t5541-http-push-smart.sh
>> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs 
>> password' '
>>  grep "^To $HTTPD_URL/smart/test_repo.git" status
>>  '
>>  
>> +cat >"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" <> +#!/bin/sh
>> +exit 1
>> +EOF
>> +chmod a+x "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
>> +
>> +cat >exp <> +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'
>> +EOF
> 
> I know the t5541 script, which is old and messy, led you into these bad
> constructs. But usually in modern tests we:
> 
>  1. Try to keep all commands inside test_expect blocks to catch
> unexpected failures or unwanted output.
> 
>  2. Use write_script for writing scripts, like:
> 
>   write_script "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" 
> <<-\EOF
>   exit 1
>   EOF
> 
>  3. Backslash our here-doc delimiter to suppress interpolation.
> 
>> +test_expect_success 'failed push status 

Re: [PATCH/RFC] push: anonymize URL in error output

[Ivan Vyshnevskyi]

On 23/08/17 13:57, Lars Schneider wrote:
> 
>> On 23 Aug 2017, at 11:49, Ivan Vyshnevskyi  wrote:
>>
>> Commits 47abd85 (fetch: Strip usernames from url's before storing them,
>> 2009-04-17) and later 882d49c (push: anonymize URL in status output,
>> 2016-07-14) made fetch and push strip the authentication part of the
>> remote URLs when used in the merge-commit messages or status outputs.
>> The URLs that are part of the error messages were not anonymized.
>>
>> A commonly used pattern for storing artifacts from a build server in a
>> remote repository utilizes a "secure" environment variable with
>> credentials to embed them in the URL and execute a push. Given enough
>> runs, an intermittent network failure will cause a push to fail, leaving
>> a non-anonymized URL in the build log.
>>
>> To prevent that, reuse the same anonymizing function to scrub
>> credentials from URL in the push error output.
>>
>> Signed-off-by: Ivan Vyshnevskyi 
>> ---
>>
>> This is my first attempt to propose a patch, sorry if I did something wrong!
>>
>> I've tested my changes on Travis CI, and the build is green [1].
>>
>> Not sure how much of the background should be included in the commit message.
>> The "commonly used pattern" I mention could be found in the myriad of
>> online tutorials and looks something like this:
>>
>>git push -fq https://$git_cr...@github.com/$REPO_SLUG
>>
>> Note, that a lot of developers mistakenly assume that '--quiet' ('-q') will
>> suppress all output. Sometimes, they would redirect standard output to
>> /dev/null, without realizing that errors are printed out to stderr.
>>
>> I didn't mention this in the commit, but another typical offender is a tool 
>> that
>> calls 'git push' as part of its execution. This is a spectrum that starts 
>> with
>> shell scripts, includes simple one-task apps in Python or Ruby, and ends with
>> the plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to
>> avoid shaming their authors publicly, but could send you a few examples
>> privately.)
>>
>> I gathered the data by going through build logs of popular open source 
>> projects
>> (and projects of their contributors) hosted on GitHub and built by Travis CI.
>> I found about 2.3k unique credentials (but only about nine hundred were 
>> active
>> at the time), and more than a half of those were exposed by a failed push. 
>> See
>> the advisory from Travis CI [2] for results of their own scan.
>>
>> While the issue is public for several months now and addressed on Travis CI,
>> I keep finding build logs with credentials on the internet. So I think it's
>> worth fixing in the git itself.
>>
>> [1]: https://travis-ci.org/sainaen/git/builds/267180560
>> [2]: https://blog.travis-ci.com/2017-05-08-security-advisory
>>
> 
> This sounds very reasonable to me! Thanks for the contribution!>
Thank you!

> I wonder if we should even extend this. Consider this case:
> 
>   $ git push https://lars:secret@server/org/repo1
>   remote: Invalid username or password.
>   fatal: Authentication failed for 'https://lars:secret@server/org/repo1/'
> 
> I might not have valid credentials for repo1 but my credentials could
> very well be valid for repo2.
> 
> - Lars
> 
Yeah, you're right. In fact, there was a similar scenario:
1. maintainer creates a "-bot" GitHub account to use for
pushing from CI back to the repository, but forgets to add this new
account to the project's organization
2. then they update the build to do the push with new credentials
3. auto-triggered build fails because the "*-bot" has no access yet

After that, they'd typically revoke exposed token and create a new one,
but sometimes they forget, and so the active token stays in the build log.

I found the place where this and couple of other errors seem to be
emitted ('discover_refs()' in remote-curl.c), but, to be honest, it just
seemed harder to figure out for the first patch: do I include the
transport.h here just to use this one function or should I copy it over?
Or move it to url.c (I guess?) and replace other usages?

Though, with some help, I'd be happy to tackle harder cases too. :)

> 
>> builtin/push.c |  2 +-
>> t/t5541-http-push-smart.sh | 18 ++
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 03846e837..59f3bc975 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -336,7 +336,7 @@ static int push_with_options(struct transport 
>> *transport, int flags)
>>  err = transport_push(transport, refspec_nr, refspec, flags,
>>   _reasons);
>>  if (err != 0)
>> -error(_("failed to push some refs to '%s'"), transport->url);
>> +error(_("failed to push some refs to '%s'"), 
>> transport_anonymize_url(transport->url));
>>
>>  err |= transport_disconnect(transport);
>>  if (!err)
>> diff --git a/t/t5541-http-push-smart.sh 

Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

> Le 23/08/2017 à 22:13, Junio C Hamano a écrit :
>> Nicolas Morey-Chaisemartin  writes:
>>
>>> Signed-off-by: Nicolas Morey-Chaisemartin 
>>> ---
>>>  imap-send.c | 34 --
>>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/imap-send.c b/imap-send.c
>>> index 09f29ea95..448a4a0b3 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, 
>>> struct imap_cmd *cmd, const cha
>>> return 0;
>>>  }
>>>  
>>> +static void server_fill_credential(struct imap_server_conf *srvc, struct 
>>> credential *cred)
>>> +{
>>> +   if (srvc->user && srvc->pass)
>>> +   return;
>>> +
>>> +   cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>>> +   cred->host = xstrdup(srvc->host);
>>> +
>>> +   cred->username = xstrdup_or_null(srvc->user);
>>> +   cred->password = xstrdup_or_null(srvc->pass);
>>> +
>>> +   credential_fill(cred);
>>> +
>>> +   if (!srvc->user)
>>> +   srvc->user = xstrdup(cred->username);
>>> +   if (!srvc->pass)
>>> +   srvc->pass = xstrdup(cred->password);
>>> +}
>>> +
>> This looks straight-forward code movement.  The only thing that
>> makes me wonder is if this is "server".  The existing naming of the
>> variables screams at me that this is not "server" but is "service".
>
> I read srvc as server conf not service.

Oh, yeah, "server conf" is OK (I didn't think of it).

> But I can change the name if you prefer

Nah.  Please keep it.

What's cooking in git.git (Aug 2017, #06; Thu, 24)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

As I am preparing to go offline next week for about a week, a
handful of topics have been merged to 'master' and 'next' is getting
thinner.  There are a few topics of importance that are still only
on the mailing list archive and not in my tree that I'd like to
review them and help them get into a reasonable shape, but that
hasn't happened yet.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ah/doc-empty-string-is-false (2017-08-14) 1 commit
  (merged to 'next' on 2017-08-19 at df47ffeffa)
 + doc: clarify "config --bool" behaviour with empty string

 Doc update.


* as/grep-quiet-no-match-exit-code-fix (2017-08-17) 1 commit
  (merged to 'next' on 2017-08-19 at 362f88fb41)
 + git-grep: correct exit code with --quiet and -L

 "git grep -L" and "git grep --quiet -L" reported different exit
 codes; this has been corrected.


* cc/subprocess-handshake-missing-capabilities (2017-08-16) 1 commit
  (merged to 'next' on 2017-08-19 at c512710fda)
 + sub-process: print the cmd when a capability is unsupported

 When handshake with a subprocess filter notices that the process
 asked for an unknown capability, Git did not report what program
 the offending subprocess was running.  This has been corrected.

 We may want a follow-up fix to tighten the error checking, though.


* hv/t5526-andand-chain-fix (2017-08-17) 1 commit
  (merged to 'next' on 2017-08-19 at fa95053653)
 + t5526: fix some broken && chains

 Test fix.


* jc/diff-sane-truncate-no-more (2017-08-17) 1 commit
  (merged to 'next' on 2017-08-19 at 3ba3980eed)
 + diff: retire sane_truncate_fn

 Code clean-up.


* jc/simplify-progress (2017-08-19) 1 commit
  (merged to 'next' on 2017-08-22 at 9077ff6912)
 + progress: simplify "delayed" progress API

 The API to start showing progress meter after a short delay has
 been simplified.


* jk/doc-the-this (2017-08-20) 1 commit
  (merged to 'next' on 2017-08-22 at 6625e50025)
 + doc: fix typo in sendemail.identity

 Doc clean-up.


* jt/sha1-file-cleanup (2017-08-11) 2 commits
  (merged to 'next' on 2017-08-19 at 93f4c94578)
 + sha1_file: remove read_packed_sha1()
 + sha1_file: set whence in storage-specific info fn
 (this branch is used by jt/packmigrate.)

 Preparatory code clean-up.


* kd/stash-with-bash-4.4 (2017-08-14) 1 commit
  (merged to 'next' on 2017-08-19 at 79b2c4b052)
 + stash: prevent warning about null bytes in input

 bash 4.4 or newer gave a warning on NUL byte in command
 substitution done in "git stash"; this has been squelched.


* ks/prepare-commit-msg-sample-fix (2017-08-14) 1 commit
  (merged to 'next' on 2017-08-19 at e75c30de64)
 + hook: use correct logical variable

 An "oops" fix to a topic that is already in 'master'.


* kw/commit-keep-index-when-pre-commit-is-not-run (2017-08-16) 1 commit
  (merged to 'next' on 2017-08-19 at 2b5a25e5ae)
 + commit: skip discarding the index if there is no pre-commit hook

 "git commit" used to discard the index and re-read from the filesystem
 just in case the pre-commit hook has updated it in the middle; this
 has been optimized out when we know we do not run the pre-commit hook.


* kw/rebase-progress (2017-08-14) 2 commits
  (merged to 'next' on 2017-08-19 at 1958f378dd)
 + rebase: turn on progress option by default for format-patch
 + format-patch: have progress option while generating patches

 "git rebase", especially when it is run by mistake and ends up
 trying to replay many changes, spent long time in silence.  The
 command has been taught to show progress report when it spends
 long time preparing these many changes to replay (which would give
 the user a chance to abort with ^C).


* lg/merge-signoff (2017-07-25) 1 commit
  (merged to 'next' on 2017-08-19 at cb53d7b026)
 + merge: add a --signoff flag

 "git merge" learned a "--signoff" option to add the Signed-off-by:
 trailer with the committer's name.


* mg/format-ref-doc-fix (2017-08-18) 2 commits
  (merged to 'next' on 2017-08-19 at 6490525c54)
 + Documentation/git-for-each-ref: clarify peeling of tags for --format
 + Documentation: use proper wording for ref format strings

 Doc fix.


* nm/stash-untracked (2017-08-11) 1 commit
  (merged to 'next' on 2017-08-19 at 70990d7eb3)
 + stash: clean untracked files before reset

 "git stash -u" used the contents of the committed version of the
 ".gitignore" file to decide which paths are ignored, even when the
 file has local changes.  The command has been taught to instead use
 the locally modified contents.


* rs/commit-h-single-parent-cleanup (2017-08-19) 1 commit
  (merged to 'next' 

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano  wrote:
> Brandon Casey  writes:
>
>> Ah, you probably meant something like this:
>>
>>const char strbuf_slopbuf = '\0';
>>
>> which gcc will apparently place in the read-only segment.  I did not know 
>> that.
>
> Yes but I highly suspect that it would be very compiler dependent
> and not something the language lawyers would recommend us to rely
> on.

I think compilers may have the option of placing variables that are
explicitly initialized to zero in the bss segment too, in addition to
those that are not explicitly initialized.  So I agree that no one
should write code that depends on their variables being placed in one
segment or the other, but I could see someone using this behavior as
an additional safety check; kind of a free assert, aside from the
additional space in the .rodata segment.

> My response was primarily to answer "why?" with "because we did not
> bother".  The above is a mere tangent, i.e. "multiple copies of
> empty strings is a horrible implementation (and there would be a way
> to do it with a single instance)".

Merely adding const to our current strbuf_slopbuf declaration does not
buy us anything since it will be allocated in r/w memory.  i.e. it
would still be possible to modify it via the buf member of strbuf.  So
you can't just do this:

   const char strbuf_slopbuf[1];

That's pretty much equivalent to what we currently have since it only
restricts modifying the contents of strbuf_slopbuf directly through
the strbuf_slopbuf variable, but it does not restrict modifying it
through a pointer to that object.

Until yesterday, I was under the impression that the only way to
access data in the rodata segment was through a constant literal.  So
my initial thought was that we could do something like:

   const char * const strbuf_slopbuf = "";

..but that variable cannot be used in a static assignment like:

   struct strbuf foo = {0, 0, (char*) strbuf_slopbuf};

So it seemed like our only option was to use a literal "" everywhere
instead of a slopbuf variable _if_ we wanted to have the guarantee
that our "slopbuf" could not be modified.

But what I learned yesterday, is that at least gcc/clang will place
the entire variable in the rodata segment if the variable is both
marked const _and_ initialized.

i.e. this will be allocated in the .rodata segment:

   const char strbuf_slopbuf[1] = "";

>>#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = (char*) 
>> _slopbuf }
>>
>> respectively.  Yeah, that's definitely preferable to a macro.
>> Something similar could be done in object.c.
>
> What is the main objective for doing this change?  The "make sure we
> do not write into that slopbuf" assert() bothers you and you want to
> replace it with an address in the read-only segment?

Actually nothing about the patch bothers me.  The point of that patch
is to make sure we don't accidentally modify the slopbuf.  I was just
looking for a way for the compiler to help out and wondering if there
was a reason we didn't attempt to do so in the first place.

I think the main takeaway here is that I learned something yesterday
:-)  I didn't actually intend to submit a patch for any of this, but
if anything useful came out of the discussion I thought Martin may
incorporate it into his patch if he wanted to.

-Brandon

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Junio C Hamano]

Brandon Casey  writes:

> Ah, you probably meant something like this:
>
>const char strbuf_slopbuf = '\0';
>
> which gcc will apparently place in the read-only segment.  I did not know 
> that.

Yes but I highly suspect that it would be very compiler dependent
and not something the language lawyers would recommend us to rely
on.

My response was primarily to answer "why?" with "because we did not
bother".  The above is a mere tangent, i.e. "multiple copies of
empty strings is a horrible implementation (and there would be a way
to do it with a single instance)".

>#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = (char*) 
> _slopbuf }
>
> respectively.  Yeah, that's definitely preferable to a macro.
> Something similar could be done in object.c.

What is the main objective for doing this change?  The "make sure we
do not write into that slopbuf" assert() bothers you and you want to
replace it with an address in the read-only segment?

Re: [RFC PATCH 0/5] Add option to autostage changes when continuing a rebase

[Junio C Hamano]

Phillip Wood  writes:

> It could be expensive to check that the local modifications will not
> interfere with the rebase - wouldn't it have to look at all the files
> touched by each commit before starting? What do cherry-pick and am do
> here when picking several commits?

Yes, so?

"I do not think of a way to implement it cheaply, so we forbid
anybody from finding a clever optimization to implement it cheaply
by adding a feature that will not work well with it when it
happens?"

Re: What does 'git blame --before -- ' supposed to mean?

[Junio C Hamano]

Kevin Daudt  writes:

> Git blame takes options that are fed to git rev-list, to limit the
> commits being taken into account for blaming.
>
> The man page shows "[--since=]", which is one such option, but
> before is valid as well.
>
> git blame -h shows:
>
>  are documented in git-rev-list(1) 
>
> and man git-blame shows under specifying ranges (emphasis mine): 
>
>  When you are not interested in changes older than version v2.6.18,
>  or changes older than 3 weeks, *you can use revision range
>  specifiers similar to git rev-list*:
>
> So these options are not documented under git blame, but git rev-list.
>
> Perhaps the synopsis of man git-blame could be expanded so that that
> it's clear it accepts rev-list options.

While nothing in what you said is incorrect per-se, many options
that rev-list takes are parsed but then ignored by blame, simply
because they do not make much sense in the context of the command,
and "--before" is one of them.  

It is interesting to realize that "--since" (and its synonym
"--after") does make sense, unlike "--before" (and its synonym
"--until") which does not.

Let's imagine a history like this (time flows from left to right):

 --c1--c2--c4--c6--c8--c9
 \ /
 c3--c5--c7

where the tip of the history is at commit "c9", and the number in
the name of each commit denotes its timestamp.

 - "git rev-list c9" starts from "c9", and follows the chain of
   parents and would produce c9, c8, c7, c6, ..., c1, 

 - If you add "--after 2", i.e. "git rev-list --after 2 c9" does
   exactly the same traversal as the above, but stops following the
   chain of parents for commits that is earlier than the specified
   time.

 - If you add "--before 6", i.e. "git rev-list --before 6 c9" does
   exactly the same traversal as the first one, but does not show
   the commit whose timestamp is later than the specified time.  It
   would be like saying "git rev-list c5 c6" in the above topology;
   the traversal from c9 down to c5 and c6 is done only to find c5
   and c6 to start the "real" traversal from. 

Now, "--after 2" will still show "c9", the tip you start your
traversal from, and this is important in the context of "blame".

Unlike "git rev-list" (and "git log" family of commands), which can
take more than one positive endpoints in the history (e.g. it is
perfectly sensible to ask "git log -p ^c1 c5 c6 -- myfile" in the
example topology above), "git blame" must be given exactly one
positive endpoint, as "git blame ^c1 c5 c6 -- myfile" would not make
any sense (ask: "do we want to know about myfile in c5?  or myfile
in c6?").

I think we also ignore "-n 4" in "blame -n 4 c9 -- myfile" (I didn't
check), but that is not because the operation fundamentally does not
make sense (like "--until") but because whoever wrote "blame" did
not think of the need to support it---in other words, if somebody
wants to add support for that option, I offhand do not see a
fundamental reason to reject it.

I do think appearing to take and understand and then silently
ignoring an option is bad, and it will help users if we tighten the
error checking while parsing the command line.

Re: [RFC PATCH 0/7] Implement ref namespaces as a ref storage backend

[Michael Haggerty]

On Sun, Aug 13, 2017 at 9:36 PM, Richard Maw  wrote:
> Forewarning: I don't consider this work complete
> and am unlikely to find time to finish it any time soon.
> I've mostly sent this because it may include valuable feedback
> on how well the ref storage backends works
> from trying to use it to change how git namespaces work.
>
> Introduction
> 
>
> I work on a git server called Gitano,
> and I'd like to add support for git namespaces to: [...]

Thanks so much for your efforts and your description of the problems
that you faced. That will be really valuable for whomever might follow
up on your work (even if it is you :-) ).

> Unfortunately namespace handling was never implemented for any other part of 
> git
> and at least gitolite makes use of namespaces,
> and will have to work around it not being implemented fully,
> but implementing it more fully will break work-arounds.

I agree that the current namespace feature is not a great foundation
for future work.

> [...]
> Fortunately the pluggable ref backends work provided an easier starting point.

:-) I'm glad my years-long obsession is finally yielding fruit.

First a general comment about the approach...

I've always thought that a workable "Git with namespaces" would
probably look more like git worktrees:

* One main repository holding all of the objects and all of the
non-pseudo references.

* One lightweight directory per namespace-view, holding the
"core.namespace" config and the pseudorefs. HEAD should probably be
stored in the main repository (?). Both the main repository and the
namespace-view directories would probably be bare, though perhaps
somebody can think of an application for allowing non-bare
repositories.

Even though this scheme implies the need for extra directories, I
think that it would make it easier to fix a lot of your problems:

* Each namespace-view could define its own namespace,
quasi-permanently. You wouldn't have to pass it via the environment.
(You might even want to *forbid* changing the namespace via the
environment or command line!) So fetches and pushes from one namespace
to another would work correctly.

* There would be one place for each namespace-view's pseudorefs. You
wouldn't have to squeeze them into a single reference tree.

* The main repository would know all of the references of all of the
namespace views, so maintenance tools like `git gc` would work without
changes.

* The namespace-view directories wouldn't be mistakable for full
repositories, so tools like `git gc` would refuse to run in them. I
think this would also make it a little bit harder for reference values
to "leak" from one namespace-view to another.

* Remote access could use different paths for different
namespace-views. The externally-visible path need not, of course, be
the same as the path of the namespace-view's directory.

By the way, there are certainly still places in the code that don't go
through the refs API (e.g., the one that Junio found). That's because
the refs API has still not been used for anything very interesting, so
the bugs haven't been flushed out. I see you've found some more.
That's because you're doing something interesting :-)

> [...]
> Bugs
> 
>
> Most boil down to how special refs like HEAD are handled.
>
> 1.  Logged messages display the namespaced path,
> which a human may deal with but confuses the test suite.

I think it's clear that the logged messages should reflect the shorter
reference names, and it is the test suite that needs to be fixed.

> 2.  Reflogs for namespaced HEAD are not updated.
>
> This is because resolving HEAD to split the transaction's updates
> to add a log only update to HEAD works by transaction_prepare resolving 
> HEAD
> using its own ref store rather than the main one,
> so the namespace translation isn't performed.
> See split_head_update.
>
> The fix for this may be to move the transaction mangling out of the 
> backend,
> unless it should be implied that every backend implementation
> must be responsible for symbolic ref reflog updates implicitly.

It probably makes sense for the namespace layer to do this step.

I think there is a similar problem with `split_symref_update()`. Here
the problem is trickier, because you don't know how to split the
update until you have locked the symref, but the locking necessarily
has to happen in the main-repo backend. So I think there will be
places where the main-repo backend needs to call back to the namespace
layer for some things, like deciding what reference names to use in
error messages and things.

You'd also want to prevent actions in a namespace-view from affecting
references outside of that namespace. For example, you shouldn't be
able to follow a symref from a namespace-view ref to another reference
in a different namespace. This also implies some cooperation between
the file-level backend and the namespace layer.

I guess it is also clear that symrefs 

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Sonny Michaud]

On 08/22/2017 11:25 AM, Sonny Michaud wrote:
I am just bumping this thread; I presume there is something amiss with 
my submissions, so if someone can let me know how to fix any issues, I 
will gladly re-submit the patch.


Thanks!

- Sonny


Bumping again for visibility.

Re: [PATCH] Retry acquiring reference locks for 100ms

[Jeff King]

On Mon, Aug 21, 2017 at 01:51:34PM +0200, Michael Haggerty wrote:

> The philosophy of reference locking has been, "if another process is
> changing a reference, then whatever I'm trying to do to it will
> probably fail anyway because my old-SHA-1 value is probably no longer
> current". But this argument falls down if the other process has locked
> the reference to do something that doesn't actually change the value
> of the reference, such as `pack-refs` or `reflog expire`. There
> actually *is* a decent chance that a planned reference update will
> still be able to go through after the other process has released the
> lock.
> 
> So when trying to lock an individual reference (e.g., when creating
> "refs/heads/master.lock"), if it is already locked, then retry the
> lock acquisition for approximately 100 ms before giving up. This
> should eliminate some unnecessary lock conflicts without wasting a lot
> of time.

It will probably comes as little surprise to others on the list that
I agree with the intent of this patch. This version is cleaned up a
little from what we're running at GitHub right now, so I'll try to
review with fresh eyes.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d5c9c4cab6..2c04b9dfb4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -776,6 +776,12 @@ core.commentChar::
>  If set to "auto", `git-commit` would select a character that is not
>  the beginning character of any line in existing commit messages.
>  
> +core.filesRefLockTimeout::
> + The length of time, in milliseconds, to retry when trying to
> + lock an individual reference. Value 0 means not to retry at
> + all; -1 means to try indefinitely. Default is 100 (i.e.,
> + retry for 100ms).
> +
>  core.packedRefsTimeout::
>   The length of time, in milliseconds, to retry when trying to
>   lock the `packed-refs` file. Value 0 means not to retry at

Do we need a separate config from packedRefsTimeout that is in the
context? I guess so, since the default values are different. And
rightfully so, I think, since writing a packed-refs file is potentially
a much larger operation (being O(n) in the number of refs rather than a
constant 40 bytes).

It probably doesn't matter all that much either way, as I wouldn't
expect people to need to tweak either of these in practice.

At some point when we have another ref backend that needs to take a
global lock, we'd probably have a third timeout. If we were starting
from scratch, I'd suggest that these might be part of refstorage.files.*
instead of core.*. And then eventually we might have
refstorage.reftable.timeout, refstorage.lmdb.timeout, etc.

But since core.packedRefsTimeout has already sailed, I'm not sure it's
worth caring about.

> diff --git a/refs.c b/refs.c
> index fe4c59aa8b..29dbb9b610 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -561,6 +561,21 @@ enum ref_type ref_type(const char *refname)
> return REF_TYPE_NORMAL;
>  }
>  
> +long get_files_ref_lock_timeout_ms(void)
> +{
> + static int configured = 0;
> +
> + /* The default timeout is 100 ms: */
> + static int timeout_ms = 100;
> +
> + if (!configured) {
> + git_config_get_int("core.filesreflocktimeout", _ms);
> + configured = 1;
> + }
> +
> + return timeout_ms;
> +}

This reads the config value into a static local that gets cached for the
rest of the program run, and there's no way to invalidate the cache. I
think that should be OK, as we would never hit the ref code until we've
actually initialized the repository, at which point we're fairly well
committed.

(I'm a little gun-shy because I used a similar lazy-load pattern for
core.sharedrepository a while back, and those corner cases bit us. But
there it was heavily used by git-init, which wants to "switch" repos
after having loaded some values).

> @@ -573,7 +588,9 @@ static int write_pseudoref(const char *pseudoref, const 
> unsigned char *sha1,
>   strbuf_addf(, "%s\n", sha1_to_hex(sha1));
>  
>   filename = git_path("%s", pseudoref);
> - fd = hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
> + fd = hold_lock_file_for_update_timeout(, filename,
> +LOCK_DIE_ON_ERROR,
> +get_files_ref_lock_timeout_ms());
>   if (fd < 0) {
>   strbuf_addf(err, "could not open '%s' for writing: %s",
>   filename, strerror(errno));

The rest of the patch looks obviously correct to me. The one thing I
didn't audit for was whether there were any calls that _should_ be
changed that you missed. But I'm pretty sure based on our previous
off-list discussions that you just did that audit.

Obviously new ref-locking calls would want to use the timeout variant
here, too, and we need to remember to do so. But I don't think there
should be many, as most sites should be going through create_reflock()
or lock_raw_ref(), 

Re: [RFC 0/3] imap-send curl tunnelling support

[Jeff King]

On Thu, Aug 24, 2017 at 04:02:19PM +0200, Daniel Stenberg wrote:

> On Thu, 24 Aug 2017, Jeff King wrote:
> 
> > > I opened a bug upstream and they already fixed this.
> > > https://github.com/curl/curl/pull/1820
> > 
> > Cool! That's much faster than I had expected. :)
> 
> Your wish is our command! =)

Oh good. While I have you here, have you given any thought to a curl
handle that has two half-duplex file descriptors, rather than a single
full-duplex socket? That would let us tunnel over pipes rather than
worrying about the portability of socketpair().

I suspect it would be quite complicated, because I imagine that lots of
internal bits of curl assume there's a single descriptor.

>  / daniel.haxx.se (who landed the IMAP PREAUTH fix in curl)

Don't you land most of the fixes in curl? :)

-Peff

Re: [RFC 0/3] imap-send curl tunnelling support

[Jeff King]

On Thu, Aug 24, 2017 at 04:15:04PM +0200, Nicolas Morey-Chaisemartin wrote:

> >> 1) There does not seem to be an easy/clean workaround for the lack of 
> >> socketpair on windows.
> >> Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
> >> means creating a socket file somewhere which pulls a lot of potential
> >> issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)
> > Even if you create a non-anonymous socket and connect to both ends, I'm
> > not sure how it works to pass that to the spawned child. IIRC, our
> > run_command emulation cannot pass arbitrary descriptors to the child
> > processes (but I don't know the details of why that is the case, or if
> > there are windows-specific calls we could be making to work around it).
> Well as long as we can map it on a fd, the dup2 trickery should allow to 
> remap whatever solution we pick to stdin/stdout.
> Could this code be put in a #ifndef WINDOWS ?

Good point. So yeah, in theory you could emulate socketpair() with a
temporary path to do the rendezvous. Just bind/listen/accept in a
non-blocking way, then connect() from the same process, then close() the
listener and delete the socket path.

Of course that doesn't work if you don't have AF_UNIX in the first
place. You could always do the same trick with TCP sockets over the
loopback, but now you get the added bonus of wondering whether whoever
connected is the other half of your process. ;)

I dunno. I am well out of my range of Windows knowledge, and I don't
have a system to test on to determine whether my suggestions are going
completely off the deep end.

-Peff

Re: splitting off shell test framework

[Jeff King]

On Thu, Aug 24, 2017 at 12:23:05AM +0100, Adam Spiers wrote:

> > [1] I actually keep a local archive and convert public-inbox URLs into
> > local requests that I view in mutt.
> 
> Sounds like a neat trick - any scripts / config worth sharing?

It's probably too specific to my setup, but here it is anyway.

I keep the archive in a normal maildir, which I index with the mairix
tool (you could do the same thing with maildir-utils or notmuch). New
messages get put into the maildir by mutt's auto-move feature after I've
read them.

Then I have a few keys bound in mutt:

  macro pager,index M 'gmane'
  macro pager,index B 'rmairix-gmane'

(you can tell from the names that these predate public-inbox entirely).

The "gmane" script just opens a browser pointing to the current message
in the archive, from which I then 'y'ank the URL into the clipboard or
pasting. The network round-trip was necessary for gmane, since the
article ids were not predictable. For public-inbox, it's not necessary,
but old habits die hard (and it's a nice cross-check that the link
you're sending out isn't broken).

Here's that script:

-- >8 --
#!/bin/sh

mid() {
  exec webview "http://public-inbox.org/git/$1;
}

article() {
  exec webview "http://public-inbox.org/git/?q=gmane:$1;
}

find_mid() {
  perl -ne 'if(/^message-id:\s*<([^>]+)>/i) { print $1, "\n"; exit 0 }'
}

case "$#" in
  0) id=`find_mid`
 case "$id" in
   "") echo >&2 "fatal: unable to extract message-id from stdin"; exit 1 ;;
*) mid "$id" ;;
 esac
 ;;
  1) case "$1" in
   *@*) mid "$1" ;;
 *) article "$1" ;;
 esac
 ;;
  *) echo >&2 "fatal: don't know how to handle $# arguments"; exit 100 ;;
esac
-- 8< --

The "webview" command is just a personal wrapper that decides which
browser to use. You could replace it with "firefox" or "chromium" or
whatever.

The "rmairix-gmane" script picks out gmane/public-inbox references and
re-opens them in mutt. It looks like this:

-- >8 --
#!/usr/bin/env perl
use URI;
use URI::Escape;

if (@ARGV) {
  show_gmane_article($_) foreach @ARGV;
}
else {
  while(<>) {
if (m{http://[^/]*gmane.org/[-\w/.]+}) {
  show_gmane_url($&);
}
if (m{https?://public-inbox.org/git/([^/]+)}) {
  show_mid(uri_unescape($1));
}
  }
}
exit 0;

sub show_mid {
  system("rmairix -t m:" . quotemeta(shift) . " ) {
chomp;
my ($nr, $mid) = split / /, $_, 2;
return $mid if $nr == $want;
  }
}

sub extract_article {
  my @path = URI->new(shift)->path_segments;

  # first one is always empty in absolute URL
  shift @path unless length($path[0]);

  # group is always next
  my $group = shift @path;

  # and then look for numbers starting from the back. E.g.,
  # focus=N for threads, or just "N" for articles
  while (@path) {
local $_ = pop @path;
return ($group, $&) if /\d+/;
  }

  return ($group, undef);
}
-- 8< --

The two extra bits you'd need are:

  - the ~/.gmane-to-mid.gz mapping. I don't remember if I made this
myself or stole it from one that Eric posted. I'm happy to share if
anybody wants it.

  - rmairix is a personal wrapper around mairix that ssh's to my imap
server to do the search and then starts mutt on the result.

Naturally I also use it for general queries like

  rmairix -t f:peff sanitize thread

I hope that helps.  I suspect it may be more useful to people as
inspiration and not as running code. I'm happy to answer any questions
or give any guidance I can.

-Peff

Re: [RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

Le 24/08/2017 à 15:53, Jeff King a écrit :
> On Thu, Aug 24, 2017 at 10:00:47AM +0200, Nicolas Morey-Chaisemartin wrote:
>
>>> Yes, I agree. I was hoping when we started this discussion that we were
>>> more ready to switch to curl-by-default. But sadly, that isn't close to
>>> being the case. But hopefully we can at least end up with logic that
>>> lets us use it in the easy cases (no tunneling) and falls back in the
>>> harder ones.
>> I opened a bug upstream and they already fixed this.
>> https://github.com/curl/curl/pull/1820
> Cool! That's much faster than I had expected. :)
>
>> At least bleeding edge curl user should be able to use this.
>> I'm not sure where to go with these patches now.
>>
>> 1) There does not seem to be an easy/clean workaround for the lack of 
>> socketpair on windows.
>> Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
>> means creating a socket file somewhere which pulls a lot of potential
>> issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)
> Even if you create a non-anonymous socket and connect to both ends, I'm
> not sure how it works to pass that to the spawned child. IIRC, our
> run_command emulation cannot pass arbitrary descriptors to the child
> processes (but I don't know the details of why that is the case, or if
> there are windows-specific calls we could be making to work around it).
Well as long as we can map it on a fd, the dup2 trickery should allow to remap 
whatever solution we pick to stdin/stdout.
Could this code be put in a #ifndef WINDOWS ?

>
>> 2) The PREAUTH support won't largely be available  for a while (curl,
>> release, distro, etc.)
>> - If this is the main use case, it does not make much sense to puch
>> curl; tunneling support without this. I could push the code and only
>> enable the curl tunneling for the next curl release ?
>>   Meaning no one (or close to no one) would use this until some later
>>   This also means very little testing (apart from mine) until the next
>> curl version gets widely available
>> - If this is not the main case (or at least the non PREAUTH is
>> important enough), it would make sense to get this changes in.
>>   But it would probably need some more to code to either fallback to
>> legacy mode when curl failed (due to PREAUTH) or detect PREAUTH and
>> directly use the legacy mode.
> It seems like we should be able to hit the cases that we know work out
> of the box, and just hold back the default for the others. Like:
>
>   static int use_curl_auto(void)
>   {
>   #ifndef USE_CURL_FOR_IMAP_SEND
>   /* not built; we cannot use it */
>   return 0;
>   #else
>   if (srvc->tunnel) {
>   #if LIBCURL_VERSION < ...
>   /* no preauth support */
>   return 0;
>   #else
>   return 1;
>   #endif /* LIBCURL_VERSION < ... */
>   }
>   ... other checks go here ...
>   #endif /* USE_CURL */
>   }
>
>   ...
>   int use_curl = -1; /* auto */
>   ... set use_curl to 0/1 from --curl/--no-curl command line */
>   if (use_curl < 0)
>   use_curl = use_curl_auto();
>
> I'm not sure what other cases are left. But over time we'd hope that
> use_curl_auto() would shrink to just "return 1", at which point
> everybody is using it (and we can drop the fallback code).
>

This code works but I'm not that confortable getting code into master that will 
have been pretty much untested (I doubt there are many git pu/next user that 
run the bleeding edge curl on their setup)
and that may just break down once curl gets updated.
It has only been tested using the example line from imap-send man page which is 
a tiny coverage and I'm sure there are some IMAP server with funky 
interpreation of the standard out there (who said Exchange?)

Nicolas

Re: [PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog

[Richard Maw]

On Wed, Aug 23, 2017 at 07:37:00PM +0700, Nguyễn Thái Ngọc Duy wrote:
> refs/bisect is unfortunately per-worktree, so we need to look in
> per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
> current iterator only goes through per-repo logs/refs.
> 
> Use merge iterator to walk two ref stores at the same time and pick
> per-worktree refs from the right iterator.
> 
> PS. Note the unsorted order of for_each_reflog in the test. This is
> supposed to be OK, for now. If we enforce order on for_each_reflog()
> then some more work will be required.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c  | 59 
> +--
>  t/t1407-worktree-ref-store.sh | 30 ++
>  2 files changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5cca55510b..d4d22882ef 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2055,23 +2046,63 @@ static struct ref_iterator_vtable 
> files_reflog_iterator_vtable = {

> +static struct ref_iterator *files_reflog_iterator_begin(struct ref_store 
> *ref_store)
> +{
> + struct files_ref_store *refs =
> + files_downcast(ref_store, REF_STORE_READ,
> +"reflog_iterator_begin");
> +
> + if (!strcmp(refs->gitdir, refs->gitcommondir)) {
> + return reflog_iterator_begin(ref_store, refs->gitcommondir);
> + } else {
> + return merge_ref_iterator_begin(
> + reflog_iterator_begin(ref_store, refs->gitdir),
> + reflog_iterator_begin(ref_store, refs->gitcommondir),
> + reflog_iterator_select, refs);
> + }
> +}
> +
>  /*
>   * If update is a direct update of head_ref (the reference pointed to
>   * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.

Whilst trying to use ref backends to implement ref namespaces
one of the issues I had was that namespaced bisect refs weren't being found.

This turned out to be for the same reason their reflogs weren't found,
that by only iterating through the commondir refs it was missing the gitdir ones
and that it only worked normally because of a special case in loose_fill_ref_dir


/*
 * Manually add refs/bisect, which, being per-worktree, might
 * not appear in the directory listing for refs/ in the main
 * repo.
 */
if (!strcmp(dirname, "refs/")) {
int pos = search_ref_dir(dir, "refs/bisect/", 12);

if (pos < 0) {
struct ref_entry *child_entry = create_dir_entry(
dir->cache, "refs/bisect/", 12, 1);
add_entry_to_dir(dir, child_entry);
}
}

If files_ref_iterator_begin was made to use a merged or overlay ref iterator too
then this special case could be removed and bisecting in a namespaced workspace
would work.

I've yet to work out whether namespaced bisect refs makes sense,
but that's a problem for the next time I have time to work on it :).

Re: [RFC 0/3] imap-send curl tunnelling support

[Daniel Stenberg]

On Thu, 24 Aug 2017, Jeff King wrote:

I opened a bug upstream and they already fixed this. 
https://github.com/curl/curl/pull/1820


Cool! That's much faster than I had expected. :)


Your wish is our command! =)

--

 / daniel.haxx.se (who landed the IMAP PREAUTH fix in curl)

Re: [RFC 0/3] imap-send curl tunnelling support

[Jeff King]

On Thu, Aug 24, 2017 at 10:00:47AM +0200, Nicolas Morey-Chaisemartin wrote:

> > Yes, I agree. I was hoping when we started this discussion that we were
> > more ready to switch to curl-by-default. But sadly, that isn't close to
> > being the case. But hopefully we can at least end up with logic that
> > lets us use it in the easy cases (no tunneling) and falls back in the
> > harder ones.
>
> I opened a bug upstream and they already fixed this.
> https://github.com/curl/curl/pull/1820

Cool! That's much faster than I had expected. :)

> At least bleeding edge curl user should be able to use this.
> I'm not sure where to go with these patches now.
> 
> 1) There does not seem to be an easy/clean workaround for the lack of 
> socketpair on windows.
> Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
> means creating a socket file somewhere which pulls a lot of potential
> issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)

Even if you create a non-anonymous socket and connect to both ends, I'm
not sure how it works to pass that to the spawned child. IIRC, our
run_command emulation cannot pass arbitrary descriptors to the child
processes (but I don't know the details of why that is the case, or if
there are windows-specific calls we could be making to work around it).

> 2) The PREAUTH support won't largely be available  for a while (curl,
> release, distro, etc.)
> - If this is the main use case, it does not make much sense to puch
> curl; tunneling support without this. I could push the code and only
> enable the curl tunneling for the next curl release ?
>   Meaning no one (or close to no one) would use this until some later
>   This also means very little testing (apart from mine) until the next
> curl version gets widely available
> - If this is not the main case (or at least the non PREAUTH is
> important enough), it would make sense to get this changes in.
>   But it would probably need some more to code to either fallback to
> legacy mode when curl failed (due to PREAUTH) or detect PREAUTH and
> directly use the legacy mode.

It seems like we should be able to hit the cases that we know work out
of the box, and just hold back the default for the others. Like:

  static int use_curl_auto(void)
  {
  #ifndef USE_CURL_FOR_IMAP_SEND
/* not built; we cannot use it */
return 0;
  #else
if (srvc->tunnel) {
  #if LIBCURL_VERSION < ...
/* no preauth support */
return 0;
  #else
return 1;
  #endif /* LIBCURL_VERSION < ... */
}
... other checks go here ...
  #endif /* USE_CURL */
  }

  ...
  int use_curl = -1; /* auto */
  ... set use_curl to 0/1 from --curl/--no-curl command line */
  if (use_curl < 0)
  use_curl = use_curl_auto();

I'm not sure what other cases are left. But over time we'd hope that
use_curl_auto() would shrink to just "return 1", at which point
everybody is using it (and we can drop the fallback code).

-Peff

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Jeff King]

On Thu, Aug 24, 2017 at 12:27:52AM -0700, Jacob Keller wrote:

> > For the sneaker-net case, you are much better off generating a single
> > pack and then using "split" and "cat" to reconstruct it on the other end
> > Not that I think we should go into such detail in the manpage, but I
> > have to wonder if --max-pack-size has outlived its usefulness. The only
> > use case I can think of is a filesystem that cannot hold files larger
> > than N bytes.
> 
> Is it possible to detect on the file system that we can't store files
> that large, and remove the option, while enabling it only when we
> detect the filesystem is unable to store large files?

I'm not sure how easy it would be to do such a check. But even if it
was, I'm not sure that buys us much. We'd still carry the code. We could
in theory remove the option, simplifying the interface. But that removes
the possibility of somebody wanting to stage the smaller packfiles on a
more capable filesystem in preparation for moving them to the
more-limited one.

-Peff

Re: [BUG] rebase -i with only empty commits

[Phillip Wood]

On 23/08/17 15:40, Johannes Schindelin wrote:
> 
> These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe
> you want to do that, too?
> 
> Ciao,
> Dscho
> 

This is slightly off topic but when I was preparing the patches for [1]
I noticed a couple of potential bugs with rebase --keep-empty that I
haven't got around to doing anything about.

1 - If 'rebase --keep-empty' stops for a conflict resolution then it
cannot resume. This is because it uses cherry-pick rather than
format-patch/am and does not create $GIT_DIR/rebase-apply so there is no
saved rebase state for continue to use. In any case the --continue code
does not have the cherry-pick special case for --keep-empty that the
startup code does. I think this could be fixed by using an implicit
interactive rebase.

2 - The opt-spec allows '--no-keep-empty' but as far as I could see that
option is never checked for in the rebase code.

Best Wishes

Phillip


[1]
https://public-inbox.org/git/20170726102720.15274-1-phillip.w...@talktalk.net/

Re: [RFC PATCH 0/5] Add option to autostage changes when continuing a rebase

[Phillip Wood]

On 22/08/17 16:54, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>>> In other words, instead of
>>>
>>> git add -u && git rebase --continue
>>>
>>> you would want a quicker way to say
>>>
>>> git rebase --continue $something_here 
>>
>> Exactly
>> ...
>> rebase --continue -a
>>
>> behaves like commit -a in that it commits all updated tracked files and
>> does not take pathspecs.
> 
> Hmph, but what 'a' in "commit -a" wants to convey is "all", and in
> the context of "rebase" we want to avoid saying "all".  A user can
> be confused into thinking "all" refers to "all subsequent commits" 
> not "all paths conflicted".
> 
> Besides, with the $something_here option, the user is explicitly
> telling us that the user finisished the resolution of conflicts left
> in the working tree.  There is nothing "auto" about it.
> 
>> Did you have any further thoughts on what checks if any this new option
>> should make to avoid staging obviously unresolved files?
> 
> The more I think about this, the more I am convinced that it is a
> very bad idea to give such a $something_here option only to "rebase".
> 
> The standard flow for any operation that could stop in the middle
> because the command needs help from the user with conflicts that
> cannot be mechanically resolved is
> 
>  (1) write out conflicted state in the working tree, and mark these
>  paths in the index by leaving them in higher stages
>  (i.e. "ls-files -u" would list them);
> 
>  (2) the user edits them and marks the ones that are now resolved;
> 
>  (3) the user tells us to "continue".
> 
> and this is not limited to "rebase".  "cherry-pick", "am", and
> "revert" all share the above, and even "merge" (which is a single
> commit operation to which "continue" in its original meaning---to
> tell us the user is done with this step and wants us to go on
> processing further commits or patches---does not quite apply) does.
> 
> And "rebase" is an oddball in that it is the only command that we
> could deduce which files in the working tree should be "add"ed, i.e.
> "all the files that are different from HEAD".  All others allow
> users to begin in a dirty working tree (they only require the index
> to be clean), 

Are you sure about that, the second sentence of the cherry-pick man page
is "This requires your working tree to be clean (no modifications from
the HEAD commit).". If you pass '--no-commit' then this is relaxed so
that the index can differ from HEAD but it is not clear from the man
page if the working tree can differ from the index (potentially that
could give different conflicts in the index and working tree). I'm not
sure what a rebase that does not commit changes would look like.

> so your 
> 
>   git [cherry-pick|am|revert] --continue $something_here
> 
> cannot be "git add -u && git [that command] --continue".  Blindly
> taking any and all files that have modifications from HEAD will
> produce a wrong result.
> 
> You cannot even sanely solve that by first recording the paths that
> are conflicted before giving control back to the user and limit the
> "add" to them, i.e. doing "git add" only for paths that appear in
> "git ls-files -u" output would not catch additional changes the was
> needed in files that did not initially conflict (i.e. "evil merge"
> will have to modify a file that was not involved in the mechanical
> part of the conflict), because out of the files that are dirty in
> the working tree, some are evil merge resolutions and others are
> ones that were dirty from the beginning.

I wonder if git could record the state of the working tree in a
temporary index just before it exits with conflicts. Then when the user
has resolved the conflicts

git [cherry-pick|am|revert] --continue $something_here

could do something like

{ GIT_INDEX_FILE=$TMP_INDEX git diff-files --name-only &&
git ls-files -u } | git update-index --remove --stdin

to stage the files that the user has edited. This would not catch the
case where a new untracked file has been created that should be added to
the conflict resolution, but I think it is reasonable to expect the user
to manually add new files in this case. It would also not add an
unchanged file whose changes should be part of the conflict resolution
but I can't think of a sensible case where the user might want that at
the moment.

> 
> The only reason "git rebase" can get away without having to worry
> about all of the above is because it insists that the working tree
> is clean before it begins.  In the ideal world, however, we would
> want to lift that and allow it to start in a working tree that has
> an unfinished local modification that does not interfere with the
> rebase, just like all other operations that could stop upon a
> conflict. 

It could be expensive to check that the local modifications will not
interfere with the rebase - wouldn't it have to look at all the files
touched by each commit before starting? What do cherry-pick and am do

[PATCH] Fix branch renaming not updating HEADs correctly

[Nguyễn Thái Ngọc Duy]

There are two bugs that sort of work together and cause problems. Let's
start with one in replace_each_worktree_head_symref.

Before fa099d2322 (worktree.c: kill parse_ref() in favor of
refs_resolve_ref_unsafe() - 2017-04-24), this code looks like this:

if (strcmp(oldref, worktrees[i]->head_ref))
continue;
set_worktree_head_symref(...);

After fa099d2322, it is possible that head_ref can be NULL. However, the
updated code takes the wrong exit. In the error case (NULL head_ref), we
should "continue;" to the next worktree. The updated code makes us
_skip_ "continue;" and update HEAD anyway.

The NULL head_ref is triggered by the second bug in add_head_info (in
the same commit). With the flag RESOLVE_REF_READING, resolve_ref_unsafe()
will abort if it cannot resolve the target ref. For orphan checkouts,
HEAD always points to an unborned branch, resolving target ref will
always fail. Now we have NULL head_ref. Now we always update HEAD.

Correct the logic in replace_ function so that we don't accidentally
update HEAD on error. As it turns out, correcting the logic bug above
breaks branch renaming completely, thanks to the second bug.

"git branch -[Mm]" does two steps (on a normal checkout, no orphan!):

- rename the branch on disk (e.g. refs/heads/abc to refs/heads/def)
- update HEAD if it points to the branch being renamed.

At the second step, since the branch pointed to by HEAD (e.g. "abc") no
longer exists on disk, we run into a temporary orphan checkout situation
that has been just corrected to _not_ update HEAD. But we need to update
HEAD since it's not actually an orphan checkout. We need to update HEAD
to move out of that orphan state.

Correct add_head_info(), remove RESOLVE_REF_READING flag. With the flag
gone, we should always return good "head_ref" in orphan checkouts (either
temporary or permanent). With good head_ref, things start to work again.

Noticed-by: Nish Aravamudan 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 branch.c  |  5 +++--
 t/t3200-branch.sh | 13 +
 worktree.c|  2 +-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index 36541d05cd..86360d36b3 100644
--- a/branch.c
+++ b/branch.c
@@ -357,8 +357,9 @@ int replace_each_worktree_head_symref(const char *oldref, 
const char *newref,
 
if (worktrees[i]->is_detached)
continue;
-   if (worktrees[i]->head_ref &&
-   strcmp(oldref, worktrees[i]->head_ref))
+   if (!worktrees[i]->head_ref)
+   continue;
+   if (strcmp(oldref, worktrees[i]->head_ref))
continue;
 
refs = get_worktree_ref_store(worktrees[i]);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd37ac47c5..902cb87ea8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -162,6 +162,19 @@ test_expect_success 'git branch -M baz bam should add 
entries to .git/logs/HEAD'
grep "^0\{40\}.*$msg$" .git/logs/HEAD
 '
 
+test_expect_success 'git branch -M should leave orphaned HEAD alone' '
+   git init orphan &&
+   (
+   cd orphan &&
+   test_commit initial &&
+   git checkout --orphan lonely &&
+   grep lonely .git/HEAD &&
+   test_path_is_missing .git/refs/head/lonely &&
+   git branch -M master mistress &&
+   grep lonely .git/HEAD
+   )
+'
+
 test_expect_success 'resulting reflog can be shown by log -g' '
oid=$(git rev-parse HEAD) &&
cat >expect <<-EOF &&
diff --git a/worktree.c b/worktree.c
index e28ffbeb09..c0c5a2b373 100644
--- a/worktree.c
+++ b/worktree.c
@@ -30,7 +30,7 @@ static void add_head_info(struct worktree *wt)
 
target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
 "HEAD",
-RESOLVE_REF_READING,
+0,
 wt->head_sha1, );
if (!target)
return;
-- 
2.11.0.157.gd943d85

Re: [RFC PATCH 0/7] Implement ref namespaces as a ref storage backend

[Richard Maw]

On Tue, Aug 15, 2017 at 10:13:22AM -0700, Junio C Hamano wrote:
> Richard Maw  writes:
> 
> > This is not my first attempt to improve the git namespace handling in git.
> > I tried last year, but it took me so long that all the ref handling code 
> > changed
> > and I would have had to start from scratch.
> >
> > Fortunately the pluggable ref backends work provided an easier starting 
> > point.
> 
> Yeah, I also made an ultra-brief foray into ref backends a few weeks
> ago, and found that Michael did an excellent job identifying the
> building blocks backends may want to implement differently and
> abstracting out major parts of the ref processing.  I also hit some
> of the same issues you mention, e.g. "HEAD" and other funny refs.
> 
> I do suspect that the current GIT_NAMESPACE thing may have outlived
> its usefulness and with the pluggable ref backend thing in place, we
> may want to redesign how support for multiple views into the same
> repository is done.  I do not have a need for such a thing myself,
> but I am glad somebody is looking into it ;-)

It was great to be able to get something mostly working this time around,
which would not have been possible without the pluggable ref backends.

I've no intention of giving up just yet,
though it'll be a while before I can devote significant time to it.

I'll be keeping an eye on the refdb backend.
If it in the process fixes the issues I'd been having that'd be so much better
since I've not got the time or community standing to champion big changes.

Re: sequencer status

[Nicolas Morey-Chaisemartin]

Le 24/08/2017 à 11:43, Matthieu Moy a écrit :
> Christian Couder  writes:
>
>> It is displaying the steps that have already been performed, right?
>> I wonder if people might want more about the current step (but maybe
>> that belongs to `git status`) or perhaps the not yet performed states
>> (and maybe even a way to edit the todo list?)
> Note that 'git status' is already doing this, but shows only 2 items of
> each:
>
> $ git status
> interactive rebase in progress; onto 3772427
> Last commands done (2 commands done):
>pick a48812c some commit title
>exec false
> Next commands to do (2 remaining commands):
>pick 9d7835d some other commit
>pick c0e0fa8 one more subject
>   (use "git rebase --edit-todo" to view and edit)
> You are currently editing a commit while rebasing branch 'master' on 
> '3772427'.
> ...
>
> The idea was that 2 lines of context is often sufficient, and doesn't
> eat too much screen space so it makes sense to show it by default.
>
> I think it makes sense to have another command that shows the whole
> sequence, but perhaps it could also be just an option for "git status".
>
> Cheers,
>

But this is only for interactive rebase.
It might be worth adding a parameter for this, but I'd also like to see this 
feature for all rebase/cp/revert

Nicolas

Re: sequencer status

[Matthieu Moy]

Christian Couder  writes:

> It is displaying the steps that have already been performed, right?
> I wonder if people might want more about the current step (but maybe
> that belongs to `git status`) or perhaps the not yet performed states
> (and maybe even a way to edit the todo list?)

Note that 'git status' is already doing this, but shows only 2 items of
each:

$ git status
interactive rebase in progress; onto 3772427
Last commands done (2 commands done):
   pick a48812c some commit title
   exec false
Next commands to do (2 remaining commands):
   pick 9d7835d some other commit
   pick c0e0fa8 one more subject
  (use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch 'master' on '3772427'.
...

The idea was that 2 lines of context is often sufficient, and doesn't
eat too much screen space so it makes sense to show it by default.

I think it makes sense to have another command that shows the whole
sequence, but perhaps it could also be just an option for "git status".

Cheers,

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH 2/2] treewide: correct several "up-to-date" to "up to date"

[Martin Ågren]

On 24 August 2017 at 05:51, STEVEN WHITE
 wrote:
> On Wed, Aug 23, 2017 at 10:49 AM, Martin Ågren  wrote:
>> Follow the Oxford style, which says to use "up-to-date" before the noun,
>> but "up to date" after it. Don't change plumbing (specifically
>> send-pack.c, but transport.c (git push) also has the same string).

> These corrections all look great (English-language-wise). :)

Great. Thanks for checking.

Problems in Default merge message

[Ulrich Windl]

Hi!

I'd like to report an old-time bug in git, regarding the default message 
visible for a "git merge --no-commit" (at the later commit, of course):

* When merging a branch, the first name is put in single quotes, while the 
second is not.

* There is no blank line after the first one

* there are two blank lines further down

I'll attach a sample default merge file as found in the editor.

I suggest to put both branches in single quotes, and to either remove the two 
blank lines, or make them comment lines (for visual separation)

Regards,
Ulrich





merge-message
Description: Binary data

Re: Undocumented change in `git branch -M` behavior

[Duy Nguyen]

On Thu, Aug 24, 2017 at 3:13 AM, Nish Aravamudan
 wrote:
> Hello,
>
> Hopefully, I've got this right -- I noticed a change in behavior in git
> with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in
> an orphaned branch, -M ends up moving HEAD to the new branch name,
> clobbering the working tree.

Thanks for the report. I think I see what's going on. I need some more
time to come up with a patch but yes this is definitely a bug (and
caused by my commit)
-- 
Duy

Re: sequencer status

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 19:57, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> Two questions:
>> - Could this be a candidate for contrib/ ?
>> - Would it be interesting to add the relevant code to sequencer.c
>> so that all sequencer based commands could have a --status option
> I actually think we would want a "git sequencer" command, which can
> be fed an arbitrary instruction sheet created by a third-party and
> told to "run" it.  A new command $cmd that wants to rewrite history
> (like "rebase -i", "cherry-pick A..B", etc. do) can only concentrate
> on preparing the sequence of instructions and then internally invoke
> "git sequencer run" until it gives the control back to the end user.
> When the user tells $cmd to continue, it can relay that request to
> "git sequencer continue" under the hood.  
> Once its use is established, it might be even possible to let users
> run "git sequencer continue", bypassing frontends for individual
> commands, e.g. "git cherry-pick --continue", etc., but I do not know
> if that is generally a good idea or not.  In any case, having such a
> front-end will help third-party scripts that want to build a custom
> workflow using the sequecing machinery we have.
>
> And in such a world, we would need "git sequencer status" command
> to give us where in a larger sequence of instrutions we are.  
>
> So I tend to think this should be part of the core, not contrib/,
> and should become part of a new command "git sequencer".

I like your idea. I'm not sure I have the bandwidth to do this (by myself at 
least).
If someone (hopefully more familiar with the sequencer code than me) wants to 
work on this, I'd gladly help.

Nicolas

Re: [git-for-windows] Re: Revision resolution for remote-helpers?

[Mike Hommey]

On Tue, Aug 22, 2017 at 10:15:20PM +0200, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 18 Aug 2017, Jonathan Nieder wrote:
> 
> > Mike Hommey wrote[1]:
> > > On Fri, Aug 18, 2017 at 03:06:37PM -0700, Jonathan Nieder wrote:
> > >> Mike Hommey wrote:
> > 
> > >>> The reason for the :: prefix is that it matches the ::
> > >>> prefix used for remote helpers.
> > >>>
> > >>> Now, there are a few caveats:
> > [...]
> > >>> - msys likes to completely fuck up command lines when they contain ::.
> > >>>   For remote helpers, the alternative that works is
> > >>>   :///etc.
> > >>
> > >> Hm --- is there a bug already open about this (e.g. in the Git for
> > >> Windows project or in msys) where I can read more?
> > >
> > > It's entirely an msys problem. Msys has weird rules to translate between
> > > unix paths and windows paths on the command line, and botches everything
> > > as a consequence. That's by "design".
> > >
> > > http://www.mingw.org/wiki/Posix_path_conversion
> > >
> > > (Particularly, see the last two entries)
> > >
> > > That only happens when calling native Windows programs from a msys
> > > shell.
> > 
> > Cc-ing the Git for Windows mailing list as an FYI.
> > 
> > I have faint memories that some developers on that project have had to
> > delve deep into Msys path modification rules.  It's possible they can
> > give us advice (e.g. about :: having been a bad choice of
> > syntax in the first place :)).
> 
> I think it is safe to assume that :: is not part of any Unix-y path. That
> is why the MSYS2 runtime does not try to play games with it by converting
> it to a Windows path.
> 
> (And indeed, I just tested this, an argument of the form
> "a::file://b/x/y/z" is not converted to a "Windows path")

Note that there are people out there using msys, *and* git for windows,
although I don't know if such people exist outside Mozilla.

Mike

Re: sequencer status

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 18:40, Christian Couder a écrit :
> Hi,
>
> On Wed, Aug 23, 2017 at 10:10 AM, Nicolas Morey-Chaisemartin
>  wrote:
>> Hi,
>>
>> I've created a small tool to display the current sequencer status.
>> It mimics what Magit does to display what was done and what is left to do.
>>
>> As someone who often rebase large series of patches (also works with am, 
>> revert, cherry-pick), I really needed the feature and use this daily.
> Yeah, many people use the interactive rebase a lot so I think it could
> be very interesting.
>
>> It's available here:
>> https://github.com/nmorey/git-sequencer-status
>> SUSE and Fedora packages are available here:
>> https://build.opensuse.org/package/show/home:NMoreyChaisemartin:git-tools/git-sequencer-status
>>
>> It's not necessary very robust yet. Most of the time I use simple rebase so 
>> there are a lot of untested corner cases.
>>
>> Here is an example output:
>> $ sequencer-status
>> # Interactive rebase: master onto rebase_conflict
>> exectrue
>> picke54d993 Fourth commit
>> exectrue
>> *pick   b064629 Third commit
>> exectrue
>> pick174c933 Second commit
>> onto773cb23 Alternate second
> It is displaying the steps that have already been performed, right?
> I wonder if people might want more about the current step (but maybe
> that belongs to `git status`) or perhaps the not yet performed states
> (and maybe even a way to edit the todo list?)

Yes it is displaying all steps.
The line beginning by '*' is the current step.

Trying to "guess" what is happening on the current step is quite hard. Between 
conflict, empty commits, stopped for amending and other, it's a lot of cases to 
handle.
I'd rather have git-status deal with it (and you get your standard log/error 
fro your rebase/cp/am/revert command too).
The idea here is really to find out where you are in your operation sequence.

I've had a 700 patch series to reapply on a different subtree. Took me 3 days. 
This script was quite handy. (Also depressing as you can see how much work left 
there is).

Also if you feel it's missing something you need, I'm accepting PR on github ;)

>> Two questions:
>> - Could this be a candidate for contrib/ ?
> It seems to me that these days we don't often add new tools to contrib/.
>
>> - Would it be interesting to add the relevant code to sequencer.c so that 
>> all sequencer based commands could have a --status option ?
> Yeah, it's probably better if it's integrated in git, either as a
> --status option in some commands, or perhaps as an option of `git
> status`.

I'll have a look at what can be done.

Thanks

Nicolas

Re: What does 'git blame --before -- ' supposed to mean?

[Kevin Daudt]

On Mon, Aug 21, 2017 at 12:15:59AM +0530, Kaartic Sivaraam wrote:
> Hello all,
> 
> I tried to do a 'git blame' on a file and wanted to see the blame
> before a particular revision of that file. I initially didn't know that
> you could achieve that with,
> 
> $ git blame  
> 
> I thought I need to pass the revision as a parameter so I tried (before
> looking into the documentation) the command found in the subject. It
> worked but to my surprise it had the same result as,
> 
> $ git blame -- 
> 
> I was confused and came to know from the documentation that blame
> doesn't have any '--before' option. That was even more surprising. Why
> does blame accept an option which it doesn't identify? Shouldn't it
> have warned that it doesn't accept the '--before' option? I guess it
> should not accept it because it confuses the user a lot as the could
> make it hard time for him to identify the issue.
> 
> 'git blame' doesn't seem to be the only command that accepts options
> not specified in the documentation there's 'git show' for it's company,
> 
> $ git show --grep 'regex'
> 
> But the good thing with the above command is it behaves as expected. I
> suspect this should be documented, anyway.
> 
> Thoughts ?
> 
> -- 
> Kaartic

Git blame takes options that are fed to git rev-list, to limit the
commits being taken into account for blaming.

The man page shows "[--since=]", which is one such option, but
before is valid as well.

git blame -h shows:

 are documented in git-rev-list(1) 

and man git-blame shows under specifying ranges (emphasis mine): 

 When you are not interested in changes older than version v2.6.18,
 or changes older than 3 weeks, *you can use revision range
 specifiers similar to git rev-list*:

So these options are not documented under git blame, but git rev-list.

Perhaps the synopsis of man git-blame could be expanded so that that
it's clear it accepts rev-list options.

Kevin

[RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 23:43, Jeff King a écrit :
> On Mon, Aug 21, 2017 at 09:34:19AM +0200, Nicolas Morey-Chaisemartin wrote:
>
 It appears curl do not support the PREAUTH tag.
>>> Too bad. IMHO preauth is the main reason to use a tunnel in the first
>>> place.
>> It shouldn't be too hard to add support for this in curl.
>> If it's the main usecase, it'll simply means the curl tunnelling
>> should be disabled by default for older curl (in this case, meaning
>> every version until it gets supported) versions.
> Yes, I agree. I was hoping when we started this discussion that we were
> more ready to switch to curl-by-default. But sadly, that isn't close to
> being the case. But hopefully we can at least end up with logic that
> lets us use it in the easy cases (no tunneling) and falls back in the
> harder ones.
>
> -Peff
I opened a bug upstream and they already fixed this.
https://github.com/curl/curl/pull/1820

At least bleeding edge curl user should be able to use this.
I'm not sure where to go with these patches now.

1) There does not seem to be an easy/clean workaround for the lack of 
socketpair on windows.
Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it means 
creating a socket file somewhere which pulls a lot of potential issues (where 
to put it ? Post-mortem cleanup ? Parallel imap-send ?)

2) The PREAUTH support won't largely be available  for a while (curl, release, 
distro, etc.)
- If this is the main use case, it does not make much sense to puch curl; 
tunneling support without this. I could push the code and only enable the curl 
tunneling for the next curl release ?
  Meaning no one (or close to no one) would use this until some later
  This also means very little testing (apart from mine) until the next curl 
version gets widely available
- If this is not the main case (or at least the non PREAUTH is important 
enough), it would make sense to get this changes in.
  But it would probably need some more to code to either fallback to legacy 
mode when curl failed (due to PREAUTH) or detect PREAUTH and directly use the 
legacy mode.

Nicolas

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Jacob Keller]

On Wed, Aug 23, 2017 at 2:22 PM, Jeff King  wrote:
> On Tue, Aug 22, 2017 at 12:56:25PM -0700, Junio C Hamano wrote:
>
>>  - There should be an update to say max-pack-size is not something
>>normal users would ever want.
>
> Agreed.
>
>> diff --git a/Documentation/git-pack-objects.txt 
>> b/Documentation/git-pack-objects.txt
>> index 8973510a41..3aa6234501 100644
>> --- a/Documentation/git-pack-objects.txt
>> +++ b/Documentation/git-pack-objects.txt
>> @@ -108,9 +108,13 @@ base-name::
>>   is taken from the `pack.windowMemory` configuration variable.
>>
>>  --max-pack-size=::
>> - Maximum size of each output pack file. The size can be suffixed with
>> + In unusual scenarios, you may not be able to create files
>> + larger than certain size on your filesystem, and this option
>> + can be used to tell the command to split the output packfile
>> + into multiple independent packfiles and what the maximum
>> + size of each packfile is. The size can be suffixed with
>>   "k", "m", or "g". The minimum size allowed is limited to 1 MiB.
>> - If specified, multiple packfiles may be created, which also
>> + This option
>>   prevents the creation of a bitmap index.
>>   The default is unlimited, unless the config variable
>>   `pack.packSizeLimit` is set.
>
> I wonder if it is worth mentioning the other downside: that the sum of
> the split packfiles may be substantially larger than a single packfile
> would be (due to lost delta opportunities between the split packs).
>
> For the sneaker-net case, you are much better off generating a single
> pack and then using "split" and "cat" to reconstruct it on the other end
> Not that I think we should go into such detail in the manpage, but I
> have to wonder if --max-pack-size has outlived its usefulness. The only
> use case I can think of is a filesystem that cannot hold files larger
> than N bytes.
>
> -Peff

Is it possible to detect on the file system that we can't store files
that large, and remove the option, while enabling it only when we
detect the filesystem is unable to store large files?

Thanks,
Jake

Re: [PATCH v3 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:28, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> Now that curl is enable by default,
> s/enable//; 
>
> But it is unclear what the above really means.  You certainly do not
> mean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use
> libcurl and makes Git fail to build without it, but the above sounds
> as if that were the case.
>
>> use the curl implementation
>> for imap too.
> The Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND
> to YesPlease when the version of cURL we have is recent enough, I
> think.  So I am not sure what you want to add with this change.
It did but apart from allowing the compilation of the code and enabling the 
--curl option to do something, it had no impact on the default runtime.
>> The goal is to validate feature parity between the legacy and
>> the curl implementation, deprecate thee legacy implementation
> s/thee/the/;

Yes this is confusing.
In the current state, even if build against a curl version supporting imap, 
curl is not used by default at runtime (unless OpenSSL was not available).
This patch changes this behavior.

>
>> later on and in the long term, hopefully drop it altogether.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> Hmph, the patch itself is also confusing.
>
>> diff --git a/imap-send.c b/imap-send.c
>> index a74d011a9..58c191704 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -35,11 +35,11 @@ typedef void *SSL;
>>  #include "http.h"
>>  #endif
>>  
>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>> -/* only available option */
>> +#if defined(USE_CURL_FOR_IMAP_SEND)
>> +/* Always default to curl if it's available. */
>>  #define USE_CURL_DEFAULT 1
> The original says "we want to use CURL, if Makefile tells us to
> *AND* if Makefile tells us not to use OpenSSL", which does not make
> much sense to me.  I wonder if the original is buggy and should have
> been using "|| defined(NO_OPENSSL)" there instead.  
I think the idea for this was that curl should be used when curl is available 
and OpenSSL is not (curl being the only solution for secured authentication in 
this case)

>
> Perhaps that is the bug you are fixing with this patch, and all the
> talk about curl is default we saw above is a red herring?  If that
> is the case, then instead of removing, turning "&&" into "||" may be
> a better fix.  I dunno.

As said before, the goal of this patch is to enable curl by default at runtime 
when it has been "enabled" at compile time.
I'll reword

Is something like this clearer ?
Author: Nicolas Morey-Chaisemartin 
Date:   Sun Aug 6 21:30:15 2017 +0200

    imap-send: use curl by default when possible
   
    Set curl as the runtime default when it is available.
    When linked against older curl versions (< 7_34_0) or without curl,
    use the legacy imap implementation.
   
    The goal is to validate feature parity between the legacy and
    the curl implementation, deprecate the legacy implementation
    later on and in the long term, hopefully drop it altogether.
   
    Signed-off-by: Nicolas Morey-Chaisemartin 

Re: [PATCH v3 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:12, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> curl_append_msgs_to_imap always returned 0, whether curl failed or not.
>> Return a proper status so git imap-send will exit with an error code
>> if womething wrong happened.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index b2d0b849b..09f29ea95 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct 
>> imap_server_conf *server,
>>  curl_easy_cleanup(curl);
>>  curl_global_cleanup();
>>  
>> -return 0;
>> +return res == CURLE_OK;
>>  }
>>  #endif
> Wait a bit.  Did you mean "res != CURLE_OK"?  If we got an OK, we
> want to return 0 as success, because the value we return from here
> is returned by cmd_main() as-is to main() and to the outside world,
> no?
>
>


Good catch. I remember testing this out but I messed up somewhere along the 
line.

Nicolas

Re: [PATCH] Retry acquiring reference locks for 100ms

[Michael Haggerty]

On Wed, Aug 23, 2017 at 11:55 PM, Junio C Hamano  wrote:
> Michael Haggerty  writes:
>
>> The philosophy of reference locking has been, "if another process is
>> changing a reference, then whatever I'm trying to do to it will
>> probably fail anyway because my old-SHA-1 value is probably no longer
>> current". But this argument falls down if the other process has locked
>> the reference to do something that doesn't actually change the value
>> of the reference, such as `pack-refs` or `reflog expire`. There
>> actually *is* a decent chance that a planned reference update will
>> still be able to go through after the other process has released the
>> lock.
>
> The reason why these 'read-only' operations take locks is because
> they want to ensure that other people will not mess with the
> anchoring points of the history they base their operation on while
> they do their work, right?

In the case of `pack-refs`, after it makes packed versions of the
loose references, it needs to lock each loose reference before pruning
it, so that it can verify in a non-racy way that the loose reference
still has the same value as the one it just packed.

In the case of `reflog expire`, it locks the reference because that
implies a lock on the reflog file, which it needs to rewrite. (Reflog
files don't have their own locks.) Otherwise it could inadvertently
overwrite a new reflog entry that is added by another process while it
is rewriting the file.

>> So when trying to lock an individual reference (e.g., when creating
>> "refs/heads/master.lock"), if it is already locked, then retry the
>> lock acquisition for approximately 100 ms before giving up. This
>> should eliminate some unnecessary lock conflicts without wasting a lot
>> of time.
>>
>> Add a configuration setting, `core.filesRefLockTimeout`, to allow this
>> setting to be tweaked.
>
> I suspect that this came from real-life needs of a server operator.
> What numbers should I be asking to justify this change? ;-)
>
> "Without this change, 0.4% of pushes used to fail due to losing the
> race against periodic GC, but with this, the rate went down to 0.2%,
> which is 50% improvement!" or something like that?

We've had a patch like this deployed to our servers for quite some
time, so I don't remember too accurately. But I think we were seeing
maybe 10-50 such errors every day across our whole infrastructure
(we're talking like literally one in a million updates). That number
went basically to zero after retries were added.

It's also not particularly serious when the race happens: the
reference update is rejected, but it is rejected cleanly.

So it's definitely a rare race, and probably only of any interest at
all on a high-traffic Git server. OTOH the cure is pretty simple, so
it seems worth fixing.

Michael

Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:13, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 34 --
>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 09f29ea95..448a4a0b3 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
>> imap_cmd *cmd, const cha
>>  return 0;
>>  }
>>  
>> +static void server_fill_credential(struct imap_server_conf *srvc, struct 
>> credential *cred)
>> +{
>> +if (srvc->user && srvc->pass)
>> +return;
>> +
>> +cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>> +cred->host = xstrdup(srvc->host);
>> +
>> +cred->username = xstrdup_or_null(srvc->user);
>> +cred->password = xstrdup_or_null(srvc->pass);
>> +
>> +credential_fill(cred);
>> +
>> +if (!srvc->user)
>> +srvc->user = xstrdup(cred->username);
>> +if (!srvc->pass)
>> +srvc->pass = xstrdup(cred->password);
>> +}
>> +
> This looks straight-forward code movement.  The only thing that
> makes me wonder is if this is "server".  The existing naming of the
> variables screams at me that this is not "server" but is "service".

I read srvc as server conf not service.
But I can change the name if you prefer

Nicolas