Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509

[Jeff King]

On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:

> diff --git a/transport-helper.c b/transport-helper.c
> index 3640804..68a4e30 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1202,7 +1202,7 @@ static int udt_do_read(struct unidirectional_transfer 
> *t)
> return 0;   /* No space for more. */
> 
> transfer_debug("%s is readable", t->src_name);
> -   bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> +   bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> errno != EINTR) {
> error_errno("read(%s) failed", t->src_name);

After this patch, I don't think we can ever see any of those errno
values again, as xread() will automatically retry in such a case.

I think that's OK. In the code before your patch, udt_do_read() would
return 0 in such a case, giving the caller the opportunity to do
something besides simply retry the read. But the only caller is
udt_copy_task_routine(), which would just loop anyway.  It may be worth
mentioning that in the commit message.

So your patch is OK.  But we should probably clean up on top, like the
patch below (on top of yours; though note your patch was whitespace
corrupted; the tabs were converted to spaces).

-- >8 --
Subject: [PATCH] transport-helper: drop read/write errno checks

Since we use xread() and xwrite() here, EINTR, EAGAIN, and
EWOULDBLOCK retries are already handled for us, and we will
never see these errno values ourselves. We can drop these
conditions entirely, making the code easier to follow.

Signed-off-by: Jeff King 
---
 transport-helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index d48be722a5..fc49567ac4 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1208,8 +1208,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
 
transfer_debug("%s is readable", t->src_name);
bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
-   if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
-   errno != EINTR) {
+   if (bytes < 0) {
error_errno("read(%s) failed", t->src_name);
return -1;
} else if (bytes == 0) {
@@ -1236,7 +1235,7 @@ static int udt_do_write(struct unidirectional_transfer *t)
 
transfer_debug("%s is writable", t->dest_name);
bytes = xwrite(t->dest, t->buf, t->bufuse);
-   if (bytes < 0 && errno != EWOULDBLOCK) {
+   if (bytes < 0) {
error_errno("write(%s) failed", t->dest_name);
return -1;
} else if (bytes > 0) {
-- 
2.16.0.rc1.446.g4cb7d89c69

Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509

[Jeff King]

On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:

> This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than
> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> was the only place outside of wrapper.c.

For my own curiosity, what is SSIZE_MAX on your platform? BUFFERSIZE is
only 64k. Do you really have 16-bit size_t?

I wondered if you would also need to set MAX_IO_SIZE, but it looks like
we default it to SSIZE_MAX.

-Peff

RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509

[Randall S. Becker]

On January 11, 2018 12:40 AM, I wrote:
> Subject: [PATCH] Replaced read with xread in transport-helper.c to fix
> SSIZE_MAX overun in t5509
> 
> This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than
> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> was the only place outside of wrapper.c.
> 
> Signed-off-by: Randall S. Becker 
> ---
>  transport-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/transport-helper.c b/transport-helper.c
> index 3640804..68a4e30 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1202,7 +1202,7 @@ static int udt_do_read(struct
> unidirectional_transfer *t)
> return 0;   /* No space for more. */
> 
> transfer_debug("%s is readable", t->src_name);
> -   bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> +   bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> errno != EINTR) {
> error_errno("read(%s) failed", t->src_name);

This fixes all known breaks except 3 on NonStop down from 229, so I'm thinking 
it's worth it. A high fix-to-bytes-changed ratio 

Cheers,
Randall

[PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509

[Randall S. Becker]

This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than
BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
was the only place outside of wrapper.c.

Signed-off-by: Randall S. Becker 
---
 transport-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 3640804..68a4e30 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1202,7 +1202,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
return 0;   /* No space for more. */

transfer_debug("%s is readable", t->src_name);
-   bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
+   bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
errno != EINTR) {
error_errno("read(%s) failed", t->src_name);
--
2.8.5.23.g6fa7ec3

Re: [PATCH] bisect: debug: convert struct object to object_id

[brian m. carlson]

On Tue, Jan 09, 2018 at 08:03:56PM +0900, Yasushi SHOJI wrote:
> The commit f2fd0760f62e79609fef7bfd7ecebb002e8e4ced converted struct
> object to object_id but a debug function show_list(), which is
> ifdef'ed to noop, in bisect.c wasn't.
> 
> So fix it.

Thanks.  This is obviously the right change to make.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 26/26] remote-curl: implement connect-half-duplex command

[Brandon Williams]

On 01/10, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:28 -0800
> Brandon Williams  wrote:
> 
> > +static size_t proxy_in(void *ptr, size_t eltsize,
> > +  size_t nmemb, void *buffer_)
> 
> OK, I managed to look at the Curl stuff in more detail.
> 
> I know that these parameter names are what remote_curl.c has been using
> for its callbacks, but I find them confusing (in particular, some Curl
> documentation rightly refer to the 1st parameter as a buffer, and the
> 4th parameter is actually userdata). Also, according to the Curl
> documentation, the type of the first parameter is "char *". Could we
> change the type of the first parameter to "char *", and the name of the
> fourth parameter either to "proxy_state_" or "userdata"?

Sounds good, I'll make the change.

> 
> > +{
> > +   size_t max = eltsize * nmemb;
> > +   struct proxy_state *p = buffer_;
> > +   size_t avail = p->request_buffer.len - p->pos;
> > +
> > +   if (!avail) {
> > +   if (p->seen_flush) {
> > +   p->seen_flush = 0;
> > +   return 0;
> > +   }
> > +
> > +   strbuf_reset(>request_buffer);
> > +   switch (packet_reader_read(>reader)) {
> > +   case PACKET_READ_EOF:
> > +   die("error reading request from parent process");
> 
> This should say "BUG:", I think. I'm not sure what the best way of
> explaining it is, but basically connect_half_duplex is supposed to
> ensure (by peeking) that there is no EOF when proxy_in() is called.

This wouldn't necessarily be a bug if the parent dies early for some
reason though right?

> 
> > +   case PACKET_READ_NORMAL:
> > +   packet_buf_write_len(>request_buffer, p->reader.line,
> > +p->reader.pktlen);
> > +   break;
> > +   case PACKET_READ_DELIM:
> > +   packet_buf_delim(>request_buffer);
> > +   break;
> > +   case PACKET_READ_FLUSH:
> > +   packet_buf_flush(>request_buffer);
> > +   p->seen_flush = 1;
> > +   break;
> > +   }
> > +   p->pos = 0;
> > +   avail = p->request_buffer.len;
> > +   }
> > +
> > +   if (max < avail)
> > +   avail = max;
> > +   memcpy(ptr, p->request_buffer.buf + p->pos, avail);
> > +   p->pos += avail;
> > +   return avail;
> 
> Thanks, this looks correct. I wish that the Curl API had a way for us to
> say "here are 4 more bytes, and that is all" instead of us having to
> make a note (p->seen_flush) to remember to return 0 on the next call,
> but that's the way it is.
> 
> > +}
> > +static size_t proxy_out(char *ptr, size_t eltsize,
> > +   size_t nmemb, void *buffer_)
> 
> Add a blank line before proxy_out. Also, same comment as proxy_in()
> about the function signature.

I'll change this function too.

> 
> > +{
> > +   size_t size = eltsize * nmemb;
> > +   struct proxy_state *p = buffer_;
> > +
> > +   write_or_die(p->out, ptr, size);
> > +   return size;
> > +}
> > +
> > +static int proxy_post(struct proxy_state *p)
> > +{
> > +   struct active_request_slot *slot;
> > +   struct curl_slist *headers = http_copy_default_headers();
> > +   int err;
> > +
> > +   headers = curl_slist_append(headers, p->hdr_content_type);
> > +   headers = curl_slist_append(headers, p->hdr_accept);
> > +   headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
> > +
> > +   slot = get_active_slot();
> > +
> > +   curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
> > +   curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
> > +   curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
> > +   curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> 
> I looked at the Curl documentation for CURLOPT_HTTPHEADER and
> curl_easy_setopt doesn't consume the argument here (in fact, it asks us
> to keep "headers" around), so it might be possible to just generate the
> headers once in proxy_state_init().

Yeah I'll go ahead and do that, it'll make the post function a bit
cleaner too.

> 
> > +
> > +   /* Setup function to read request from client */
> > +   curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);
> > +   curl_easy_setopt(slot->curl, CURLOPT_READDATA, p);
> > +
> > +   /* Setup function to write server response to client */
> > +   curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);
> > +   curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);
> > +
> > +   err = run_slot(slot, NULL);
> > +
> > +   if (err != HTTP_OK)
> > +   err = -1;
> 
> This seems to mean that we cannot have two requests in flight at the
> same time even while there is no response (from the fact that we have a
> HTTP status code after returning from run_slot()).
> 
> I thought that git fetch over HTTP uses the two-requests-in-flight
> optimization that it also does over other protocols like SSH, but I see
> that that code path 

git svn clone of messy repository

[Jason Greenbaum]

Hi,

I'm in the process of using git svn to migrate several repos over to
git and one repo, in particular, has a very challenging format.
During the migration, I would also like to reorganize the repo.  It
looks something like this in svn:

myrepo
  trunk
project_of_interest
other_project1
other_project2
…
  branches
FF-1.0
  project_of_interest
  other_project1
  other_project2
  …
   FF-1.1
  project_of_interest
  other_project1
  other_project2
  …

There is also a 'tags' directory at the toplevel, but that's not
important to illustrate the issue I'm having.  What I would like to do
is to migrate ONLY the folder called 'project_of_interest' and
preserve it's trunk, branches, and tags.  I'm not sure this is
possible directly with git svn clone, but here is what I've tried:

git svn clone \
--authors-file=$AUTHORS_FILE \
--prefix="" \
--trunk=trunk/project_of_interest \
--branches=branches/FF-1.0/project_of_interest \
--branches=branches/FF-1.1/project_of_interest \
svn://my-svn-server/myrepo \
project_of_interest.git

The trunk seems to become the 'master' branch just fine, but my svn
branches are not pulled down.  I'm not sure I have the syntax right or
if this is even possible without first reorganizing the svn repo in
place, updating the .git/config file, or by some other means.  Any
help would be much appreciated.  I'm happy to provide more info as
needed.

Thanks,

Jason

Re: [PATCH 00/26] protocol version 2

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:02 -0800
> Brandon Williams  wrote:
> 
> >  * Introduce a new remote-helper command 'connect-half-duplex' which is
> >implemented by remote-curl (the http remote-helper).  This allows for a
> >client to establish a half-duplex connection and use remote-curl as a 
> > proxy
> >to wrap requests in http before sending them to the remote end and
> >unwrapping the responses and sending them back to the client's stdin.
> 
> I'm not sure about the "half-duplex" name - it is half-duplex in that
> each side must terminate their communications with a flush, but not
> half-duplex in that request-response pairs can overlap each other (e.g.
> during negotation during fetch - there is an optimization in which the
> client tries to keep two requests pending at a time). I think that the
> idea we want to communicate is that requests and responses are always
> packetized, stateless, and always happen as a pair.
> 
> I wonder if "stateless-connect" is a better keyword - it makes sense to
> me (once described) that "stateless" implies that the client sends
> everything the server needs at once (thus, in a packet), the server
> sends everything the client needs back at once (thus, in a packet), and
> then the client must not assume any state-storing on the part of the
> server or transport.

I like that name much better, I think I'll change it to use
'stateless-connect'.  Thanks :)


-- 
Brandon Williams

Re: Test failure for v2.16.0-rc0 on cygwin

[Ramsay Jones]

On 04/01/18 20:55, Johannes Schindelin wrote:
> On Tue, 2 Jan 2018, Ramsay Jones wrote:
[snip]
>> Also, when logged-in remotely it fails consistently, when logged-in
>> directly it passes consistently. :-D
> 
> You are most likely hitting cmd.exe at some point there. In cmd.exe, there
> are some restrictions that are inherited by spawned processes AFAIU. For
> example, the current directory cannot be a UNC path.
> 
> You are most likely running the interactive Cygwin session in MinTTY? Then
> you do not get those restrictions. If you start Cygwin in a cmd.exe
> window, you should see the exact same test failures again.

I actually don't see a difference when starting cygwin from a cmd.exe, it
passes just fine. The interactive cygwin session(s), either directly, or
most often via the X-server (with ssh-agent in between!), all have their
id's and group membership look like:

  $ who
  $ id
  uid=1001(ramsay) gid=513(None) 
groups=513(None),545(Users),4(INTERACTIVE),66049(CONSOLE 
LOGON),11(Authenticated Users),15(This Organization),113(Local 
account),66048(LOCAL),262154(NTLM Authentication),401408(Medium Mandatory Level)
  $

However, when remotely logged-in over shh, it looks like:

  $ who -H
  NAME LINE TIME COMMENT
  ramsay   pty2 2018-01-02 19:48 (192.168.1.2)
  $ id
  uid=1001(ramsay) gid=513(None) groups=513(None),11(Authenticated 
Users),66048(LOCAL),66049(CONSOLE LOGON),4(INTERACTIVE),15(This 
Organization),545(Users),0(root),405504(High Mandatory Level)
  $

So, when remotely logged-in, we have:

  Additional groups: 0(root), 405504(High Mandatory Level)

  Missing groups: 113(Local account), 262154(NTLM Authentication),
  401408(Medium Mandatory Level)

I haven't thought too much about what that means ...

After reading this[1], I have been meaning to try setting the
'LocalAccountTokenFilterPolicy' registry variable mentioned in
that article, to see if that would make any difference. I haven't
found the time yet ... :-D

ATB,
Ramsay Jones


[1] http://www.tomsitpro.com/articles/windows-10-administrative-shares,2-47.html

Re: [PATCH v2] t3900: add some more quotes

[Johannes Schindelin]

Hi Beat,

On Wed, 10 Jan 2018, Beat Bolli wrote:

> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index 9e4e694d9..dc00db87b 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
>  '
>  
>  test_expect_success 'UTF-8 invalid characters refused' '
> - test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> + test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&

It is probably worth quoting the dollar characters, too, as pointed out by
Hannes Sixt: we want to interpolate the value only when needed, just in
case that the HOME variable contains double quotes.

Ciao,
Dscho

Re: [PATCH v2] t3900: add some more quotes

[Junio C Hamano]

Beat Bolli  writes:

> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> handler, two files are deleted which must be quoted individually.
>
> Signed-off-by: Beat Bolli 
> ---
>
> Diff to v1:
>
> s/hander/handler/ in the message.
> ...

OK, but that forgets to fix a more important issue raised in the
discussion, no?

Here is what I ended up queuing in the meantime.  Thanks.

-- >8 --
From: Beat Bolli 
Date: Wed, 10 Jan 2018 10:58:32 +0100
Subject: [PATCH] t3900: add some more quotes

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
command, two files are deleted which must be quoted individually.

[jc: with \$HOME in the test_when_finished command quoted, as
pointed out by j6t].

Signed-off-by: Beat Bolli 
Helped-by: Johannes Sixt 
Signed-off-by: Junio C Hamano 
---
 t/t3900-i18n-commit.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d93..b92ff95977 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
 '
 
 test_expect_success 'UTF-8 invalid characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
echo "UTF-8 characters" >F &&
printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
>"$HOME/invalid" &&
@@ -49,7 +49,7 @@ test_expect_success 'UTF-8 invalid characters refused' '
 '
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
rm -f "$HOME/stderr" "$HOME/invalid" &&
echo "UTF-8 overlong" >F &&
printf "\340\202\251ommit message\n\nThis is not a space:\300\240\n" \
@@ -59,7 +59,7 @@ test_expect_success 'UTF-8 overlong sequences rejected' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
echo "UTF-8 non-character 1" >F &&
printf "Commit message\n\nNon-character:\364\217\277\276\n" \
>"$HOME/invalid" &&
@@ -68,7 +68,7 @@ test_expect_success 'UTF-8 non-characters refused' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
echo "UTF-8 non-character 2." >F &&
printf "Commit message\n\nNon-character:\357\267\220\n" \
>"$HOME/invalid" &&
-- 
2.16.0-rc1-187-g8dee184084

Re: [PATCH v5 8/9] sequencer: try to commit without forking 'git commit'

[Johannes Schindelin]

Hi,

On Wed, 10 Jan 2018, Jonathan Nieder wrote:

> Phillip Wood wrote:
> 
> > From: Phillip Wood 
> >
> > If the commit message does not need to be edited then create the
> > commit without forking 'git commit'. Taking the best time of ten runs
> > with a warm cache this reduces the time taken to cherry-pick 10
> > commits by 27% (from 282ms to 204ms), and the time taken by 'git
> > rebase --continue' to pick 10 commits by 45% (from 386ms to 212ms) on
> > my computer running linux. Some of greater saving for rebase is
> > because it no longer wastes time creating the commit summary just to
> > throw it away.
> 
> Neat!  Dmitry Torokhov (cc-ed) noticed[1] that this causes the
> prepare-commit-msg hook not to be invoked, which I think is
> unintentional.  Should we check for such a hook and take the slowpath
> when it is present?

We could also easily recreate the functionality:

if (find_hook("pre-commit")) {
struct argv_array hook_env = ARGV_ARRAY_INIT;

argv_array_pushf(_env, "GIT_INDEX_FILE=%s",
get_index_file());
argv_array_push(_env, "GIT_EDITOR=:");
ret = run_hook_le(hook_env.argv, "pre-commit", NULL);
argv_array_clear(_env);
}

(This assumes that the in-process try_to_commit() is only called if the
commit message is not to be edited interactively, which is currently the
case.)

Ciao,
Dscho

[PATCH v2] t3900: add some more quotes

[Beat Bolli]

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
handler, two files are deleted which must be quoted individually.

Signed-off-by: Beat Bolli 
---

Diff to v1:

s/hander/handler/ in the message.

 t/t3900-i18n-commit.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d9..dc00db87b 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
 '
 
 test_expect_success 'UTF-8 invalid characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
echo "UTF-8 characters" >F &&
printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
>"$HOME/invalid" &&
@@ -49,7 +49,7 @@ test_expect_success 'UTF-8 invalid characters refused' '
 '
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
rm -f "$HOME/stderr" "$HOME/invalid" &&
echo "UTF-8 overlong" >F &&
printf "\340\202\251ommit message\n\nThis is not a space:\300\240\n" \
@@ -59,7 +59,7 @@ test_expect_success 'UTF-8 overlong sequences rejected' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
echo "UTF-8 non-character 1" >F &&
printf "Commit message\n\nNon-character:\364\217\277\276\n" \
>"$HOME/invalid" &&
@@ -68,7 +68,7 @@ test_expect_success 'UTF-8 non-characters refused' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
echo "UTF-8 non-character 2." >F &&
printf "Commit message\n\nNon-character:\357\267\220\n" \
>"$HOME/invalid" &&
-- 
2.15.0.rc1.299.gda03b47c3

Re: [PATCH 1/2] cat-file doc: document that -e will return some output

[Junio C Hamano]

Junio C Hamano  writes:

> Ævar Arnfjörð Bjarmason   writes:
>
>> The -e option added in 7950571ad7 ("A few more options for
>> git-cat-file", 2005-12-03) has always errored out with message on
>> stderr saying that the provided object is malformed, currently:
>>
>> $ git cat-file -e malformed; echo $?
>> fatal: Not a valid object name malformed
>> 128
>>
>> A careful reader of this documentation would be mislead into thinking
>> the could write:
>>
>> if ! git cat-file -e "$object" [...]
>
> It is arguable if such a reader is careful or careless.  I'd rather drop
> s/careful // there ;-)

Actually the phrasing around here was a bit strange, and I ended up
rewriting a bit more.

cat-file doc: document that -e will return some output

The -e option added in 7950571ad7 ("A few more options for
git-cat-file", 2005-12-03) has always errored out with message on
stderr saying that the provided object is malformed, like this:

$ git cat-file -e malformed; echo $?
fatal: Not a valid object name malformed
128

A reader of this documentation may be misled into thinking that

if ! git cat-file -e "$object" [...]

as opposed to:

if ! git cat-file -e "$object" 2>/dev/null [...]

is sufficient to implement a truly silent test that checks whether
some arbitrary $object string was both valid, and pointed to an
object that exists.

Signed-off-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Junio C Hamano 

Re: What's cooking in git.git (Jan 2018, #02; Tue, 9)

[Jeff Hostetler]

On 1/10/2018 2:57 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


On 1/9/2018 6:33 PM, Junio C Hamano wrote:

--
[Cooking]


* jh/fsck-promisors (2017-12-08) 10 commits

[...]


* jh/partial-clone (2017-12-08) 13 commits

[...]

Parts 2 and 3 of partial clone have been simmering
for a while now.  I was wondering if there were any
more comments or questions on them.  I don't recall
any existing issues.


Me neither.

I do not mind merging them to 'next' during the feature freeze, but
we won't be merging them to 'master' soon anyway, and I'd like to
see us concentrate more on finding and fixing regressions on the
'master' front for now.


I didn't want to rush this -- just check to see if there was
any thing that I should focus on while waiting for 2.16 to ship
and settle down.

Leave this where it is now if you want and we can merge it in later.

Thanks
Jeff

 

Re: [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Junio C Hamano]

Prathamesh Chavan  writes:

> Thanks for pointing out that we can introduce the flag 
> REMOVE_DIR_KEEP_TOPLEVEL
> which solves the issue. And for the case where no directory exists: we
> create an empty
> directory.Since this won't be similar to what happens in the shell
> script, this change
> can be included in a saperate patch as an imporvement.

Exactly.  The way the shell script does it is to _always_ honor
user's umask and recreate the directory, so before that separate
improvement, tweaking "mode" based on the returned value from an
extra lstat() is an unneeded change of behaviour.  Just passing 0777
and let mkdir() take the umask into account to come up with the
final permission bits is more in line with the original scripted
version, I would think.

Thanks.

Re: [PATCH 1/2] cat-file doc: document that -e will return some output

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> The -e option added in 7950571ad7 ("A few more options for
> git-cat-file", 2005-12-03) has always errored out with message on
> stderr saying that the provided object is malformed, currently:
>
> $ git cat-file -e malformed; echo $?
> fatal: Not a valid object name malformed
> 128
>
> A careful reader of this documentation would be mislead into thinking
> the could write:
>
> if ! git cat-file -e "$object" [...]

It is arguable if such a reader is careful or careless.  I'd rather drop
s/careful // there ;-)

> As opposed to:
>
> if ! git cat-file -e "$object" 2>/dev/null [...]
>
> To check whether some arbitrary $object string was both valid, and
> pointed to an object that exists.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  Documentation/git-cat-file.txt | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Re: [PATCH] t3900: add some more quotes

[Junio C Hamano]

Jeff King  writes:

> Yeah. One of the reasons for both of the errors in this thread is the
> nested double-quoting. Using single quotes is awkward because we're
> already using them to delimit the whole snippet.  I've often wondered if
> our tests would be more readable taking the snippet over stdin.
> Something like:
> +test_expect_success 'UTF-8 invalid characters refused' - <<\EOT
> + test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&
> ...
> -'
> +EOT
> 
> +# Read from stdin into the variable given in $1.
> +test_read_to_eof () {
> + # Bash's "read" is sufficiently flexible that we can skip the extra
> + # process.
> + if test -n "$BASH_VERSION"
> + then
> + # 64k should be enough for anyone...
> + read -N 65536 -r "$1"
> + else
> + # command substitution eats trailing whitespace, so we add
> + # and then remove a non-whitespace character.
> + eval "$1=\$(cat; printf x)"
> + eval "$1=\${$1%x}"
> + fi
> +}

Yuck.  Hacky but nice.

I think this will make it easier to read but I suspect here text
would use temporary files and may slow things down a bit?  I do not
know if it is even measurable, though.

> +
>  test_expect_failure () {
>   test_start_
>   test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
>   test "$#" = 2 ||
>   error "bug in the test script: not 2 or 3 parameters to 
> test-expect-failure"
> + if test "$2" = "-"
> + then
> + test_read_to_eof test_block
> + set -- "$1" "$test_block"
> + fi
>   test_verify_prereq
>   export test_prereq
>   if ! test_skip "$@"
> @@ -416,6 +437,11 @@ test_expect_success () {
>   test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
>   test "$#" = 2 ||
>   error "bug in the test script: not 2 or 3 parameters to 
> test-expect-success"
> + if test "$2" = "-"
> + then
> + test_read_to_eof test_block
> + set -- "$1" "$test_block"
> + fi
>   test_verify_prereq
>   export test_prereq
>   if ! test_skip "$@"

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Junio C Hamano]

Dmitry Torokhov  writes:

> Right, so it looks like the master works well, it is next(?) branch
> that is troublesome (apparently we pack experimental internally?).
>
> I bisected it down to:
>
> commit 356ee4659bb551cd9464b317d691827276752c2d (refs/bisect/bad)
> Author: Phillip Wood 
> Date:   Fri Nov 24 11:07:57 2017 +
>
>sequencer: try to commit without forking 'git commit'
> ...
>
> With this commit the hook is not being run unless I specify '-e' flag
> to cherry-pick.

Thanks.

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Martin Fick]

On Wednesday, January 10, 2018 02:39:13 PM Derrick Stolee 
wrote:
> On 1/10/2018 1:25 PM, Martin Fick wrote:
> > On Sunday, January 07, 2018 01:14:41 PM Derrick Stolee
> > 
> > wrote:
> >> This RFC includes a new way to index the objects in
> >> multiple packs using one file, called the multi-pack
> >> index (MIDX).
> > 
> > ...
> > 
> >> The main goals of this RFC are:
> >> 
> >> * Determine interest in this feature.
> >> 
> >> * Find other use cases for the MIDX feature.
> > 
> > My interest in this feature would be to speed up fetches
> > when there is more than one large pack-file with many of
> > the same objects that are in other pack-files.   What
> > does your MIDX design do when it encounters multiple
> > copies of the same object in different pack files? 
> > Does it index them all, or does it keep a single copy?
> 
> The MIDX currently keeps only one reference to each
> object. Duplicates are dropped during writing. (See the
> care taken in commit 04/18 to avoid duplicates.) Since
> midx_sha1_compare() does not use anything other than the
> OID to order the objects, there is no decision being made
> about which pack is "better". The MIDX writes the first
> copy it finds and discards the others.

This would likely speed things up then, even if the chosen 
objects are suboptimal.

> It would not be difficult to include a check in
> midx_sha1_compare() to favor one packfile over another
> based on some measurement (size? mtime?). Since this
> would be a heuristic at best, I left it out of the
> current patch.

Yeah, I didn't know what heuristic to use either, I tended 
to think that the bigger pack-file would be valuable because 
it is more likely to share deltas with other objects in that 
pack, so more easy to send them.  However, that is likely 
only true during clones or other large fetches when we want 
most objects.  During small "update" fetches, the newer 
packs might be better?

I also thought that objects in alternates should be 
considered less valuable for my use case, however in the 
github fork use case, the alternates might be more valuable?

So yes heuristics, and I don't know what is best.  Perhaps 
some config options could be used to set heuristics like 
this.  Whatever the heuristics are, since they would be a 
part of the MIDX packing process it would be easy to change.  
This assumes that keeping only one copy in the index is the 
right thing.  The question would be, what if we need 
different heuristics for different operations?  Would it make 
sense to have multiple MIDX files covering the same packs 
then, one for fetch, one for merge...?

> > In our Gerrit instance (Gerrit uses jgit), we have
> > multiple copies of the linux kernel repos linked
> > together via the alternatives file mechanism.
> 
> GVFS also uses alternates for sharing packfiles across
> multiple copies of the repo. The MIDX is designed to
> cover all packfiles in the same directory, but is not
> designed to cover packfiles in multiple alternates;
> currently, each alternate would need its own MIDX file.
> Does that cause issues with your setup?

No, since the other large packfiles are all in other repos 
(alternates).  Is there a reason the MIDX would not want to 
cover the alternates?  If you don't then you would seemingly 
loose any benefits of the MIDX when you have alternates in 
use.

...
> > It would be nice if this use case could be improved with
> > MIDX.  To do so, it seems that it would either require
> > that MIDX either only put "the best" version of an
> > object (i.e. pre-select which one to use), or include
> > the extra information to help make the selection
> > process of which copy to use (perhaps based on the
> > operation being performed) fast.
> 
> I'm not sure if there is sufficient value in storing
> multiple references to the same object stored in multiple
> packfiles. There could be value in carefully deciding
> which copy is "best" during the MIDX write, but during
> read is not a good time to make such a decision. It also
> increases the size of the file to store multiple copies.

Yes, I am not sure either, it would be good to have input 
from experts here.

> > This also leads me to ask, what other additional
> > information (bitmaps?) for other operations, besides
> > object location, might suddenly be valuable in an index
> > that potentially points to multiple copies of objects? 
> > Would such information be appropriate in MIDX, or would
> > it be better in another index?
> 
> For applications to bitmaps, it is probably best that we
> only include one copy of each object. Otherwise, we need
> to include extra bits in the bitmaps for those copies
> (when asking "is this object in the bitmap?").

Agreed.  Would the MIDX potentially pave the way to create 
multi-pack bitmaps?

> Thanks for the context with Gerrit's duplicate object
> problem. I'll try to incorporate it in to the design
> document (commit 01/18) for the v1 patch.

FYI, this is not a typical Gerrit thing, it is 

information required

[From Amir Khadov]

-- 
Thanks for your last email response to me.
The information required should include the following-:
Your full names
Your address
Telephone number
Your private email
Occupation
Age
This is to enable my further discussion with you in confidence.
Best regards and wishes to you.
Mohammad Amir Khadov
NB: Please reply to:

am...@postaxte.com

am...@pobox.sk

Re: [PATCH v5 8/9] sequencer: try to commit without forking 'git commit'

[Jonathan Nieder]

Hi,

Phillip Wood wrote:

> From: Phillip Wood 
>
> If the commit message does not need to be edited then create the
> commit without forking 'git commit'. Taking the best time of ten runs
> with a warm cache this reduces the time taken to cherry-pick 10
> commits by 27% (from 282ms to 204ms), and the time taken by 'git
> rebase --continue' to pick 10 commits by 45% (from 386ms to 212ms) on
> my computer running linux. Some of greater saving for rebase is
> because it no longer wastes time creating the commit summary just to
> throw it away.

Neat!  Dmitry Torokhov (cc-ed) noticed[1] that this causes the
prepare-commit-msg hook not to be invoked, which I think is
unintentional.  Should we check for such a hook and take the slowpath
when it is present?

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/cakdakrquj1hfkeckjur2op+8c1i+zr36o-+aryif4ufas_z...@mail.gmail.com/

Re: [PATCH 0/2] the cat-file -e option doesn't work as documented

[Junio C Hamano]

Junio C Hamano  writes:

> I am kind of confused.  
>
> When the doc there says "no output", I read it as "no output", and
> no other restriction (like suppressing an error diagnosis, which is
> not even sent to the standard output stream).

Ah, OK, so you were saying with 1/2 that "output" could mean
"something sent to standard error stream" as well and can be helped
with a more explicit clarification.  Makes sense to me (sort-of).

Re: [PATCH v2 1/2] Doc/gitsubmodules: make some changes to improve readability and syntax

[Stefan Beller]

On Tue, Jan 9, 2018 at 10:49 PM, Kaartic Sivaraam
 wrote:
> * Only mention porcelain commands in examples
>
> * Split a sentence for better readability
>
> * Add missing apostrophes
>
> * Clearly specify the advantages of using submodules
>
> * Avoid abbreviations
>
> * Use "Git" consistently
>
> * Improve readability of certain lines
>
> * Clarify when a submodule is considered active
>
> Helped-by: Eric Sunshine 
> Helped-by: Stefan Beller 
> Signed-off-by: Kaartic Sivaraam 
> ---

Thanks for sending it in one patch,
Stefan

>  Documentation/gitsubmodules.txt | 93 
> +++--
>  1 file changed, 72 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
> index 46cf120f6..ce2369c2d 100644
> --- a/Documentation/gitsubmodules.txt
> +++ b/Documentation/gitsubmodules.txt
> @@ -36,8 +36,8 @@ The `gitlink` entry contains the object name of the commit 
> that the
>  superproject expects the submodule’s working directory to be at.
>
>  The section `submodule.foo.*` in the `.gitmodules` file gives additional
> -hints to Gits porcelain layer such as where to obtain the submodule via
> -the `submodule.foo.url` setting.
> +hints to Git's porcelain layer. For example, the `submodule.foo.url`
> +setting specifies where to obtain the submodule.
>
>  Submodules can be used for at least two different use cases:
>
> @@ -51,18 +51,21 @@ Submodules can be used for at least two different use 
> cases:
>
>  2. Splitting a (logically single) project into multiple
> repositories and tying them back together. This can be used to
> -   overcome current limitations of Gits implementation to have
> +   overcome current limitations of Git's implementation to have
> finer grained access:
>
> -* Size of the git repository:
> +* Size of the Git repository:
>In its current form Git scales up poorly for large repositories 
> containing
>content that is not compressed by delta computation between trees.
> -  However you can also use submodules to e.g. hold large binary assets
> -  and these repositories are then shallowly cloned such that you do not
> +  For example, you can use submodules to hold large binary assets
> +  and these repositories can be shallowly cloned such that you do not
>have a large history locally.
>  * Transfer size:
>In its current form Git requires the whole working tree present. It
>does not allow partial trees to be transferred in fetch or clone.
> +  If the project you work on consists of multiple repositories tied
> +  together as submodules in a superproject, you can avoid fetching the
> +  working trees of the repositories you are not interested in.
>  * Access control:
>By restricting user access to submodules, this can be used to implement
>read/write policies for different users.
> @@ -73,9 +76,10 @@ The configuration of submodules
>  Submodule operations can be configured using the following mechanisms
>  (from highest to lowest precedence):
>
> - * The command line for those commands that support taking submodule specs.
> -   Most commands have a boolean flag '--recurse-submodules' whether to
> -   recurse into submodules. Examples are `ls-files` or `checkout`.
> + * The command line for those commands that support taking submodules
> +   as part of their pathspecs. Most commands have a boolean flag
> +   `--recurse-submodules` which specify whether to recurse into submodules.
> +   Examples are `grep` and `checkout`.
> Some commands take enums, such as `fetch` and `push`, where you can
> specify how submodules are affected.
>
> @@ -87,8 +91,8 @@ Submodule operations can be configured using the following 
> mechanisms
>  For example an effect from the submodule's `.gitignore` file
>  would be observed when you run `git status --ignore-submodules=none` in
>  the superproject. This collects information from the submodule's working
> -directory by running `status` in the submodule, which does pay attention
> -to its `.gitignore` file.
> +directory by running `status` in the submodule while paying attention
> +to the `.gitignore` file of the submodule.
>  +
>  The submodule's `$GIT_DIR/config` file would come into play when running
>  `git push --recurse-submodules=check` in the superproject, as this would
> @@ -97,20 +101,20 @@ remotes are configured in the submodule as usual in the 
> `$GIT_DIR/config`
>  file.
>
>   * The configuration file `$GIT_DIR/config` in the superproject.
> -   Typical configuration at this place is controlling if a submodule
> -   is recursed into at all via the `active` flag for example.
> +   Git only recurses into active submodules (see 'ACTIVE SUBMODULES'
> +   section below).

The section below is capitalized differently?


>  +
>  If the submodule is not yet initialized, 

Re: [PATCH 0/2] the cat-file -e option doesn't work as documented

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> The -e option to cat-file will emit output, after promising not to.
>
> We should take either 1/2 or 2/2, but not both. I'm partial to just
> documenting the existing behavior and dropping 2/2, it's useful to
> know if you passed in something that didn't look like a SHA-1.
>
> But if others disagree we can drop 1/2 and take 2/2. Up to you.
>
> Ævar Arnfjörð Bjarmason (2):
>   cat-file doc: document that -e will return some output
>   cat-file: -e should not emit output on stderr
>
>  Documentation/git-cat-file.txt | 7 ---
>  builtin/cat-file.c | 8 ++--
>  t/t1006-cat-file.sh| 7 +++
>  3 files changed, 17 insertions(+), 5 deletions(-)

I am kind of confused.  

When the doc there says "no output", I read it as "no output", and
no other restriction (like suppressing an error diagnosis, which is
not even sent to the standard output stream).

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

[Johannes Schindelin]

Hi Duy,

On Wed, 10 Jan 2018, Duy Nguyen wrote:

> On Mon, Jan 08, 2018 at 01:25:04PM +0100, Johannes Schindelin wrote:
> > I agree that it would make a ton of sense to use a proper, portable test
> > framework written in pure, portable C.
> > 
> > However, this ship has long sailed, hasn't it?
> 
> If you meant converting the whole test suite, oh yeah that's not gonna
> happen. But it's still possible to have some tests written in C.

True.

> I played a bit with this. The assumption is if it's agreed that we can
> get something bare bone (but functional) in then we could start having
> more and more C-based unit tests in future and also improve the C
> framework to be on par with shell one on the side.

We can also start with something small that does not contend to replace
the entire test suite, and evolve from there as we have time.

Your initial patch looks very good, I will give it a cursory review (only
cursory because we are technically in a feature-freeze...)

> diff --git a/Makefile b/Makefile
> index 2a81ae22e9..567387b558 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -644,6 +644,7 @@ X =
>  
>  PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>  
> +TEST_PROGRAMS_NEED_X += test-3071-wildmatch

I guess I can always work on unifying those gazillion of test executables
into a single one later.

For testing purposes, I have to bundle them (BusyBox-based MinGit is
supposed to be stand-alone, yet I want to test it to verify that it works
even if it ships only a subset of Git for Windows), and they dominate the
payload of any prerelease, as you can see e.g. here:
https://github.com/git-for-windows/git/releases/tag/v2.16.0-rc1.windows.1

> diff --git a/t/helper/test-3071-wildmatch.c b/t/helper/test-3071-wildmatch.c
> new file mode 100644
> index 00..24a657202d
> --- /dev/null
> +++ b/t/helper/test-3071-wildmatch.c
> @@ -0,0 +1,273 @@
> +#include "cache.h"
> +#include "test-lib.h"
> +
> +struct match_input {
> + int expect_true;
> + const char *text;
> + const char *pattern;
> +};
> +
> +static struct match_input match_tests[] = {
> + /* Basic wildmatch features */
> + { 1, "foo", "foo" },
> + { 0, "foo", "bar" },
> + { 1, "", "" },

These patterns share the "magic-ness" of Ævar's test cases... although
your version is certainly more concise.

BTW IIRC Ævar explicitly said that he needs to use `ls-files` in order to
test the interaction with the index, so that would probably take a little
bit more work.

> diff --git a/t/t3071-wildmatch.sh b/t/t3071-wildmatch.sh
> new file mode 100755
> index 00..6e83b4d684
> --- /dev/null
> +++ b/t/t3071-wildmatch.sh
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +
> +exec helper/test-3071-wildmatch t3071-wildmatch "$@"

Should it not be `exec test-3071-wildmatch "${0%.sh}" "$@"`?

> diff --git a/test-lib.c b/test-lib.c
> new file mode 100644
> index 00..8e8b7cd6df
> --- /dev/null
> +++ b/test-lib.c
> @@ -0,0 +1,97 @@
> [...]

Lots of good stuff in there. Definitely a good start.

Ciao,
Dscho

Re: [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

On Wed, Jan 10, 2018 at 2:54 AM, Junio C Hamano  wrote:
> Prathamesh Chavan  writes:
>
>> The same mechanism is used even for porting this submodule
>> subcommand, as used in the ported subcommands till now.
>> The function cmd_deinit in split up after porting into four
>> functions: module_deinit(), for_each_listed_submodule(),
>> deinit_submodule() and deinit_submodule_cb().
>>
>> Mentored-by: Christian Couder 
>> Mentored-by: Stefan Beller 
>> Signed-off-by: Prathamesh Chavan 
>> ---
>>  builtin/submodule--helper.c | 153 
>> 
>>  git-submodule.sh|  55 +---
>>  2 files changed, 154 insertions(+), 54 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index dd7737acd..54b0e46fc 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -20,6 +20,7 @@
>>  #define OPT_QUIET (1 << 0)
>>  #define OPT_CACHED (1 << 1)
>>  #define OPT_RECURSIVE (1 << 2)
>> +#define OPT_FORCE (1 << 3)
>>
>>  typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
>> void *cb_data);
>> @@ -908,6 +909,157 @@ static int module_sync(int argc, const char **argv, 
>> const char *prefix)
>>   return 0;
>>  }
>>
>> +struct deinit_cb {
>> + const char *prefix;
>> + unsigned int flags;
>> +};
>> +#define DEINIT_CB_INIT { NULL, 0 }
>> +
>> +static void deinit_submodule(const char *path, const char *prefix,
>> +  unsigned int flags)
>> +{
>> + const struct submodule *sub;
>> + char *displaypath = NULL;
>> + struct child_process cp_config = CHILD_PROCESS_INIT;
>> + struct strbuf sb_config = STRBUF_INIT;
>> + char *sub_git_dir = xstrfmt("%s/.git", path);
>> + mode_t mode = 0777;
>> +
>> + sub = submodule_from_path(_oid, path);
>> +
>> + if (!sub || !sub->name)
>> + goto cleanup;
>> +
>> + displaypath = get_submodule_displaypath(path, prefix);
>> +
>> + /* remove the submodule work tree (unless the user already did it) */
>> + if (is_directory(path)) {
>> + struct stat st;
>> + /*
>> +  * protect submodules containing a .git directory
>> +  * NEEDSWORK: automatically call absorbgitdirs before
>> +  * warning/die.
>> +  */
>
> I guess that you mean "instead of dying, automatically call absorb
> and (possibly) warn"?  That sounds like a sensible improvement.
>
>> + if (is_directory(sub_git_dir))
>> + die(_("Submodule work tree '%s' contains a .git "
>> +   "directory use 'rm -rf' if you really want "
>> +   "to remove it including all of its history"),
>
> This changes the message text by removing () around "use ... history",
> which I do not think you intended to do.
>
>> +   displaypath);
>> +
>> + if (!(flags & OPT_FORCE)) {
>> + struct child_process cp_rm = CHILD_PROCESS_INIT;
>> + cp_rm.git_cmd = 1;
>> + argv_array_pushl(_rm.args, "rm", "-qn",
>> +  path, NULL);
>> +
>> + if (run_command(_rm))
>> + die(_("Submodule work tree '%s' contains local 
>> "
>> +   "modifications; use '-f' to discard 
>> them"),
>> +   displaypath);
>> + }
>> +
>> + if (!lstat(path, )) {
>
> What is this if statement doing here?  It does not make sense,
> especially without an 'else' clause on the other side, at least to
> me.
>
> At this point in the flow, the code has already determined that path
> is a directory above before starting to check if it has ".git/"
> immediately below it, or trying to run "git rm" in the dry run mode
> to see if it yields an error, so at this point lstat() should
> succeed (and would say it is a directory).  I would sort-of
> understand it if this "if()" has an "else" clause to act on an
> error, but that is not something the original does not do, so I am
> not sure if it belongs to a "rewrite to C" patch.
>
>> + struct strbuf sb_rm = STRBUF_INIT;
>> + const char *format;
>> +
>> + strbuf_addstr(_rm, path);
>> +
>> + if (!remove_dir_recursively(_rm, 0))
>> + format = _("Cleared directory '%s'\n");
>> + else
>> + format = _("Could not remove submodule work 
>> tree '%s'\n");
>> +
>> + if (!(flags & OPT_QUIET))
>> + printf(format, displaypath);
>> +
>> + mode = st.st_mode;
>> +
>> + strbuf_release(_rm);
>> +

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Junio C Hamano]

Jeff King  writes:

> To be clear, which approach are we talking about? I think there are
> three options:
>
>   1. The user tells us not to bother computing real ahead/behind values.
>  We always say "same" or "not the same".
>
>   2. The user tells us not to bother computing ahead/behind values
>  with more effort than N. After traversing N commits without getting
>  an answer, we say "same" or "not the same". But we may sometimes
>  give a real answer if we found it within N.
>
>   3. The user tells us not to spend more effort than N. After traversing
>  N commits we try to make some partial statement based on
>  generations (or commit timestamps as a proxy for them).
>
> I agree that (3) is probably not going to be useful enough in the
> general case to merit the implementation effort and confusion. But is
> there anything wrong with (2)?

I agree (3) would not be all that interesting.  Offhand I do not see
a problem with (2).  I think with "real" in your "sometimes give a
real answer" you meant to say that we limit our answers to just one
three ("same", "not the same", "ahead/behind by exactly N/M") and I
think it is a good choice that is easy to explain.

We might be able to say things other than these three, namely,
"ahead by no more than N, behind by no more than M", but I do not
know if that is useful or merely more confusing than it's worth.

Re: [ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

Johannes Schindelin  writes:

> No, the only thing that changed was the introduction of Git::Packet (and
> t0021/*.perl uses that). And that Perl module is not yet installed.

Ahh, that is the difference among other users of split(/:/,
$ENV{GITPERLLIB}).  Scripts other than 0021 may be using installed
ones and using the wrong separator does not affect them.

> Granted, we tested incorrect versions of those modules, but this late in
> the -rc phase, I would prefer to be cautious. People might be relying on
> the current bug.

People meaning those who run "make test"?

In any case, the patch to 0021 as posted sounds like the most
conservative thing to do at this point, even though we would
definitely want to revisit it and clean it up after the release.  As
long as the reason why only 0021 wants the special case while others
are OK is understood, I am OK with the patch ;-)

Thanks.

Re: What's cooking in git.git (Jan 2018, #02; Tue, 9)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Tue, Jan 09 2018, Junio C. Hamano jotted:
>
>> * ab/wildmatch-tests (2018-01-04) 7 commits
>>   (merged to 'next' on 2018-01-09 at 09f0b84098)
>>  + wildmatch test: create & test files on disk in addition to in-memory
>>  + wildmatch test: perform all tests under all wildmatch() modes
>>  + wildmatch test: remove dead fnmatch() test code
>>  + wildmatch test: use a paranoia pattern from nul_match()
>>  + wildmatch test: don't try to vertically align our output
>>  + wildmatch test: use more standard shell style
>>  + wildmatch test: indent with tabs, not spaces
>>
>>  More tests for wildmatch functions.
>>
>>  Will cook in 'next'.
>
> Please don't merge it down for now.

Oops, the above entry in the "What's cooking" report is totally
bogus.  It is not merged to 'next' yet, and 09f0b84098 does not
exist in the public history of the project.

What happened was that I rewound 'next' after making a trial merge
before pushing the result out.  I updated the "What's cooking" report
while the trial merge was in 'next', which I should have redone.

>> * ab/perf-grep-threads (2018-01-04) 1 commit
>>   (merged to 'next' on 2018-01-09 at 8fe1d71894)
>>  + perf: amend the grep tests to test grep.threads
>>
>>  More perf tests for threaded grep
>>
>>  Will cook in 'next'.
>
> Re: the concern raised in xmqqa7xsaqki@gitster.mtv.corp.google.com I
> think it makes sense to just document (and I can do that if you agree)
> that:
>
> test_expect_success SOME_PREREQ,$SOME_OTHER_PREREQ,$ANOTHER_ONE [...]
>
> Will work as far as prereqs goes even though the variables might be
> empty. It's much less verbose than the proposed alternative, and easy to
> support.

OK.

>>  Will [cook in|merge to] 'next'.
>
> Refresh my memory, that means merge down post-2.16.0 at this point,
> right?

After -rc1, anything merged to 'next' will by default stay there
until the final.  Hotfixes will be merged to 'next' and (unless I
forget) will be marked as "will merge to 'master'" when that
happens, so that we will have them in the final.

Re: What's cooking in git.git (Jan 2018, #02; Tue, 9)

[Junio C Hamano]

Jeff Hostetler  writes:

> On 1/9/2018 6:33 PM, Junio C Hamano wrote:
>> --
>> [Cooking]
>>
>>
>> * jh/fsck-promisors (2017-12-08) 10 commits
> [...]
>
>> * jh/partial-clone (2017-12-08) 13 commits
> [...]
>
> Parts 2 and 3 of partial clone have been simmering
> for a while now.  I was wondering if there were any
> more comments or questions on them.  I don't recall
> any existing issues.

Me neither.

I do not mind merging them to 'next' during the feature freeze, but
we won't be merging them to 'master' soon anyway, and I'd like to
see us concentrate more on finding and fixing regressions on the
'master' front for now.

Re: [PATCH] t3900: add some more quotes

[Jeff King]

On Wed, Jan 10, 2018 at 08:02:09PM +0100, Johannes Sixt wrote:

> > diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> > index 9e4e694d9..dc00db87b 100755
> > --- a/t/t3900-i18n-commit.sh
> > +++ b/t/t3900-i18n-commit.sh
> > @@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
> >   '
> >   test_expect_success 'UTF-8 invalid characters refused' '
> > -   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> > +   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
> 
> Should that not better be
> 
>   test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""
> 
> i.e., delay the expansion of $HOME to protect against double-quotes in the
> path?

Yeah. One of the reasons for both of the errors in this thread is the
nested double-quoting. Using single quotes is awkward because we're
already using them to delimit the whole snippet.  I've often wondered if
our tests would be more readable taking the snippet over stdin.
Something like:

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d93..09ad4d8878 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -39,14 +39,14 @@ test_expect_success 'UTF-16 refused because of NULs' '
test_must_fail git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
 '
 
-test_expect_success 'UTF-8 invalid characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+test_expect_success 'UTF-8 invalid characters refused' - <<\EOT
+   test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&
echo "UTF-8 characters" >F &&
printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
>"$HOME/invalid" &&
git commit -a -F "$HOME/invalid" 2>"$HOME"/stderr &&
test_i18ngrep "did not conform" "$HOME"/stderr
-'
+EOT
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2a06..be8a47d304 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -391,11 +391,32 @@ test_verify_prereq () {
error "bug in the test script: '$test_prereq' does not look like a 
prereq"
 }
 
+# Read from stdin into the variable given in $1.
+test_read_to_eof () {
+   # Bash's "read" is sufficiently flexible that we can skip the extra
+   # process.
+   if test -n "$BASH_VERSION"
+   then
+   # 64k should be enough for anyone...
+   read -N 65536 -r "$1"
+   else
+   # command substitution eats trailing whitespace, so we add
+   # and then remove a non-whitespace character.
+   eval "$1=\$(cat; printf x)"
+   eval "$1=\${$1%x}"
+   fi
+}
+
 test_expect_failure () {
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to 
test-expect-failure"
+   if test "$2" = "-"
+   then
+   test_read_to_eof test_block
+   set -- "$1" "$test_block"
+   fi
test_verify_prereq
export test_prereq
if ! test_skip "$@"
@@ -416,6 +437,11 @@ test_expect_success () {
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to 
test-expect-success"
+   if test "$2" = "-"
+   then
+   test_read_to_eof test_block
+   set -- "$1" "$test_block"
+   fi
test_verify_prereq
export test_prereq
if ! test_skip "$@"

Re: [PATCH/RFC] add--interactive: ignore all internal submodule changes

[Stefan Beller]

On Wed, Jan 10, 2018 at 3:06 AM, Nguyễn Thái Ngọc Duy  wrote:
> For 'add -i' and 'add -p' the only action we can take on a dirty
> submodule entry (from the superproject perspective) is its SHA-1. The
> content changes inside do not matter, at least until interactive add has
> --recurse-submodules or something.
>
> Ignore all dirty changes to reduce the questions 'add -i' and 'add -p'
> throw at the user when submodules are dirty.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  $DAYJOB started to use submodules and this annoys me so much when I
>  use 'git add -p'. I'm neither very familiar with add--interactive nor
>  submodules code but this seems to work. Hopefully it's a correct
>  change.

I would think this fixes your problem and it looks correct.

However I wonder about some subtle detail:
the "dirty" setting will ignore anything inside the submodule, and
only pay attention to the delta in gitlinks between HEAD and index.

Maybe we'd want to have a mode "dirty-except-submodule-HEAD",
which would ignore all submodule worktree changes, but if its HEAD
is different than the gitlink in the superproject index or HEAD, such that
checking out a different revision inside the submodule is not lost
when staging things in the superproject for a new commit?

>
>  git-add--interactive.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 28b325d754..964c3a7542 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -262,7 +262,7 @@ sub list_modified {
> }
> }
>
> -   for (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), 
> @ARGV)) {
> +   for (run_cmd_pipe(qw(git diff-files --ignore-submodules=dirty 
> --numstat --summary --raw --), @ARGV)) {
> if (($add, $del, $file) =
> /^([-\d]+)  ([-\d]+)(.*)/) {
> $file = unquote_path($file);
> --
> 2.15.1.600.g899a5f85c6
>

Re: [PATCH] t3900: add some more quotes

[Junio C Hamano]

Johannes Sixt  writes:

>> test_expect_success 'UTF-8 invalid characters refused' '
>> -test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
>> +test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
>
> Should that not better be
>
>   test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""
>
> i.e., delay the expansion of $HOME to protect against double-quotes in
> the path?

Yes ;-)

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Derrick Stolee]

On 1/10/2018 1:25 PM, Martin Fick wrote:

On Sunday, January 07, 2018 01:14:41 PM Derrick Stolee
wrote:

This RFC includes a new way to index the objects in
multiple packs using one file, called the multi-pack
index (MIDX).

...

The main goals of this RFC are:

* Determine interest in this feature.

* Find other use cases for the MIDX feature.

My interest in this feature would be to speed up fetches
when there is more than one large pack-file with many of the
same objects that are in other pack-files.   What does your
MIDX design do when it encounters multiple copies of the
same object in different pack files?  Does it index them all,
or does it keep a single copy?


The MIDX currently keeps only one reference to each object. Duplicates 
are dropped during writing. (See the care taken in commit 04/18 to avoid 
duplicates.) Since midx_sha1_compare() does not use anything other than 
the OID to order the objects, there is no decision being made about 
which pack is "better". The MIDX writes the first copy it finds and 
discards the others.


It would not be difficult to include a check in midx_sha1_compare() to 
favor one packfile over another based on some measurement (size? 
mtime?). Since this would be a heuristic at best, I left it out of the 
current patch.



In our Gerrit instance (Gerrit uses jgit), we have multiple
copies of the linux kernel repos linked together via the
alternatives file mechanism.


GVFS also uses alternates for sharing packfiles across multiple copies 
of the repo. The MIDX is designed to cover all packfiles in the same 
directory, but is not designed to cover packfiles in multiple 
alternates; currently, each alternate would need its own MIDX file. Does 
that cause issues with your setup?



   These repos have many different
references (mostly Gerrit change references), but they share
most of the common objects from the mainline.  I have found
that during a large fetch such as a clone, jgit spends a
significant amount of extra time by having the extra large
pack-files from the other repos visible to it, usually around
an extra minute per instance of these (without them, the
clone takes around 7mins).  This adds up easily with a few
repos extra repos, it can almost double the time.

My investigations have shown that this is due to jgit
searching each of these pack files to decide which version of
each object to send.  I don't fully understand its selection
criteria, however if I shortcut it to just pick the first
copy of an object that it finds, I regain my lost time.  I
don't know if git suffers from a similar problem?  If git
doesn't suffer from this then it likely just uses the first
copy of an object it finds (which may not be the best object
to send?)

It would be nice if this use case could be improved with
MIDX.  To do so, it seems that it would either require that
MIDX either only put "the best" version of an object (i.e.
pre-select which one to use), or include the extra
information to help make the selection process of which copy
to use (perhaps based on the operation being performed)
fast.


I'm not sure if there is sufficient value in storing multiple references 
to the same object stored in multiple packfiles. There could be value in 
carefully deciding which copy is "best" during the MIDX write, but 
during read is not a good time to make such a decision. It also 
increases the size of the file to store multiple copies.



This also leads me to ask, what other additional information
(bitmaps?) for other operations, besides object location,
might suddenly be valuable in an index that potentially
points to multiple copies of objects?  Would such
information be appropriate in MIDX, or would it be better in
another index?


For applications to bitmaps, it is probably best that we only include 
one copy of each object. Otherwise, we need to include extra bits in the 
bitmaps for those copies (when asking "is this object in the bitmap?").


Thanks for the context with Gerrit's duplicate object problem. I'll try 
to incorporate it in to the design document (commit 01/18) for the v1 patch.


Thanks,
-Stolee

Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set

[Stefan Beller]

On Wed, Jan 10, 2018 at 11:26 AM, Brandon Williams  wrote:
> On 01/10, Stefan Beller wrote:
>> On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams  wrote:
>> > At first when i read this I was under the impression that the whole
>> > environment was going to be printed out...but i now realize that this
>> > tracing  will only print out the delta's or the additions to the
>> > environment that the child will see.  Maybe this should be called out
>> > more clearly in the commit message?
>>
>> It only adds newly set variables, I wonder why deletions are noisy?
>> I could not find an example of a removal of environment variables
>> specific to submodules that would be noisy.
>
> Deletions are noisy because we append local_repo_env anytime we kick
> off a child process (ok maybe not all the time, but a lot of the time)
> which is just a list of ~15 deletions.

Oh, I see. I'm fine with that.

Though to further the discussion, maybe we want to check if such a variable
is set and if so actually show the deletion? I have the impression that most
of the variables are not set, and yet we unconditionally delete them, "just to
be sure". Maybe we'd only want to delete them if we actually have them
set in the superproject, and then we can show the diff in env vars
more faithful?

Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set

[Brandon Williams]

On 01/10, Stefan Beller wrote:
> On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams  wrote:
> > At first when i read this I was under the impression that the whole
> > environment was going to be printed out...but i now realize that this
> > tracing  will only print out the delta's or the additions to the
> > environment that the child will see.  Maybe this should be called out
> > more clearly in the commit message?
> 
> It only adds newly set variables, I wonder why deletions are noisy?
> I could not find an example of a removal of environment variables
> specific to submodules that would be noisy.

Deletions are noisy because we append local_repo_env anytime we kick
off a child process (ok maybe not all the time, but a lot of the time)
which is just a list of ~15 deletions.

-- 
Brandon Williams

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Dmitry Torokhov]

On Tue, Jan 9, 2018 at 6:24 PM, Junio C Hamano  wrote:
>
> Dmitry Torokhov  writes:
>
> >> I had prepare-commit-msg hook that would scrub "Patchwork-ID: " tags
> >> form commit messages and would update input mailing list patchwork to
> >> mark corresponding patches as "accepted" when I cherry pick form
> >> WIP/review queue into branches that I publish, but that recently stopped
> >> working if I do a simple cherry-pick.
> >
> > This seems like a regression, at least for my use case. Unfortunately
> > my mail seems to get lost in the mailing list noise...
>
> Possibly.  Can you bisect to see which commit broke things for you?
> That would allow people who know what they themselves broke better
> than I do to take a look ;-)

Right, so it looks like the master works well, it is next(?) branch
that is troublesome (apparently we pack experimental internally?).

I bisected it down to:

commit 356ee4659bb551cd9464b317d691827276752c2d (refs/bisect/bad)
Author: Phillip Wood 
Date:   Fri Nov 24 11:07:57 2017 +

   sequencer: try to commit without forking 'git commit'

   If the commit message does not need to be edited then create the
   commit without forking 'git commit'. Taking the best time of ten runs
   with a warm cache this reduces the time taken to cherry-pick 10
   commits by 27% (from 282ms to 204ms), and the time taken by 'git
   rebase --continue' to pick 10 commits by 45% (from 386ms to 212ms) on
   my computer running linux. Some of greater saving for rebase is
   because it no longer wastes time creating the commit summary just to
   throw it away.

   The code to create the commit is based on builtin/commit.c. It is
   simplified as it doesn't have to deal with merges and modified so that
   it does not die but returns an error to make sure the sequencer exits
   cleanly, as it would when forking 'git commit'

   Even when not forking 'git commit' the commit message is written to a
   file and CHERRY_PICK_HEAD is created unnecessarily. This could be
   eliminated in future. I hacked up a version that does not write these
   files and just passed an strbuf (with the wrong message for fixup and
   squash commands) to do_commit() but I couldn't measure any significant
   time difference when running cherry-pick or rebase. I think
   eliminating the writes properly for rebase would require a bit of
   effort as the code would need to be restructured.

   Signed-off-by: Phillip Wood 
   Signed-off-by: Junio C Hamano 

With this commit the hook is not being run unless I specify '-e' flag
to cherry-pick.

Thanks.

-- 
Dmitry

Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set

[Stefan Beller]

On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams  wrote:
> On 01/10, Nguyễn Thái Ngọc Duy wrote:
>> Occasionally submodule code could execute new commands with GIT_DIR set
>> to some submodule. GIT_TRACE prints just the command line which makes it
>> hard to tell that it's not really executed on this repository.

Speaking of GIT_DIR, we may also want to print the dir that the command is
executed in as the GIT_DIR for submodules usually is only ".git" assuming the
run-command struct set the .dir to the submodules worktree. I think I had a
patch for printing the cwd of a process on the list once upon a time, but
I am unable to find it again. Maybe we'd want to include the cwd of a spawned
process if it differs from the current process?

>>
>> Print env variables in this case. Note that the code deliberately ignore
>> variables unsetting because there are so many of them (to keep git
>> environment clean for the next process) and really hard to read.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  A minor thing that I ignored in this patch is quoting the environment
>>  variables. For me it's meh. Perhaps I should just dumb the env
>>  without quoting at all.
>
> A patch like this would have been very helpful to me on some previous
> occasions, so thanks for writing it up.

The patch I mentioned above (very similar though less powerful than this one)
was carried by locally for some time, so I definitely see value in this patch.
Thanks for writing it!

>
>>
>>  run-command.c |  3 ++-
>>  trace.c   | 38 +++---
>>  trace.h   | 18 +++---
>>  3 files changed, 52 insertions(+), 7 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 31fc5ea86e..235367087d 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)
>>   cmd->err = fderr[0];
>>   }
>>
>> - trace_argv_printf(cmd->argv, "trace: run_command:");
>> + trace_env_argv_printf(cmd->env, cmd->argv, "trace: run_command:");
>> +
>>   fflush(NULL);
>>
>>  #ifndef GIT_WINDOWS_NATIVE
>> diff --git a/trace.c b/trace.c
>> index b7530b51a9..d8967b66e8 100644
>> --- a/trace.c
>> +++ b/trace.c
>> @@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int 
>> line, struct trace_key *key,
>>   print_trace_line(key, );
>>  }
>>
>> +static void concatenate_env(struct strbuf *dst, const char *const *env)
>> +{
>> + int i;
>> +
>> + /* Copy into destination buffer. */
>> + strbuf_grow(dst, 255);
>> + for (i = 0; env[i]; ++i) {
>> + /*
>> +  * the main interesting is setting new vars
>> +  * e.g. GIT_DIR, ignore the unsetting to reduce noise.
>> +  */
>
> I think you're missing a word, maybe:
>   'The main interesting part is setting new vars'
>
>> + if (!strchr(env[i], '='))
>> + continue;
>> + strbuf_addch(dst, ' ');
>> + sq_quote_buf(dst, env[i]);
>> + }
>
> At first when i read this I was under the impression that the whole
> environment was going to be printed out...but i now realize that this
> tracing  will only print out the delta's or the additions to the
> environment that the child will see.  Maybe this should be called out
> more clearly in the commit message?

It only adds newly set variables, I wonder why deletions are noisy?
I could not find an example of a removal of environment variables
specific to submodules that would be noisy.

Stefan

Re: [PATCH] t3900: add some more quotes

[Johannes Sixt]

Am 10.01.2018 um 10:58 schrieb Beat Bolli:

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
hander, two files are deleted which must be quoted individually.

Signed-off-by: Beat Bolli 
---
  t/t3900-i18n-commit.sh | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d9..dc00db87b 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
  '
  
  test_expect_success 'UTF-8 invalid characters refused' '

-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&


Should that not better be

test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""

i.e., delay the expansion of $HOME to protect against double-quotes in 
the path?


-- Hannes

[BUG] Breakage in t5509 # 2 from read(stdin) EINVAL

[Randall S. Becker]

Hi All,

Here’s the situation. In the NonStop port, since time immemorial, we’ve had
a breakage in 5509 that I’ve finally had the chance to track down. The error
report at the breakage is:

./trash directory.t5509-fetch-push-namespaces/original: GIT_TRACE=true
GIT_PACKET_TRACE=true GIT_TRANSLOOP_DEBUG=true git push pushee-namespaced
master
11:55:02.512020 trace: built-in: git 'push' 'pushee-namespaced' 'master'
11:55:02.600895 trace: run_command: 'git-remote-ext' 'pushee-namespaced'
'git --namespace=namespace %s ../pushee'
11:55:02.610439 trace: built-in: git 'remote-ext' 'pushee-namespaced' 'git
--namespace=namespace %s ../pushee'
11:55:02.612643 trace: run_command: 'git' '--namespace=namespace'
'receive-pack' '../pushee'
11:55:02.622553 trace: built-in: git 'receive-pack' '../pushee'
Transfer loop debugging: stdin is readable
error: read(stdin) failed: Invalid function argument
Transfer loop debugging: remote input is readable
error: read(remote input) failed: Invalid function argument
error: Git to program copy process failed
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

transport-helper.c uses read instead of wrapper.c’s xread to do its work. It
looks like when the buffer size calculation is done passed into read, we’re
getting a very large value, more than SSIZE_MAX. In udt_do_read:

bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);

generates EINVAL. This may be because unidirectional_transfer.bufuse is
size_t instead of ssize_t. I can try to fix this a number of ways, but it
seems that xread is likely the way to go. udt_do_write already uses xwrite.

I’m looking to fix on or after 2.13.5 (7234152).

Opinions?

Thanks,
Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(2112884442)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

RE: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

On January 10, 2018 1:16 PM, Johannes Sixt wrote:
> Am 10.01.2018 um 01:12 schrieb Randall S. Becker:
> > On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> > I'm encountering strange warnings, while looking into the details of what
> test t0001 fails in spots. These include:
> > #24 warning: templates not found x00...[lots of 0
> > deleted...] which exceeds 2K, but that's just
> content, right, and not causing an apparent breakage.
> 
> This warning occurs also on Linux and Windows. I think it is by design and
> not something to be fixed.
> 
> >
> > # 34. Admittedly it was shorter than 2K, but there is something weird in 
> > this
> path that I can't find, causing a failure out of fts_read from gnulib.
> > Initialized empty Git repository in /home/ituglib/randall/git/t/trash
> > directory.t0001-
> init/123456789abcdef/123456789abcdef/123456789abcdef/1
> >
> 23456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123
> 4567
> > 89abcdef/newdir/.git/
> > rm: fts_read failed: No such file or directory
> >
> > This error is coming from some of shell utilities (in this case rm)
> > used in the test rather than git code itself.
> 
> So, the problem is not in the git executable. This does not warrant a change
> in the build process, yet.
> 
> >  While well within the
> > supported path length of the operating system/platform (1K), there is
> > an acknowledged issue that is causing breakage when paths get large
> > enough (even only this large, unfortunately). We're at 221 breaks out
> > of 12982-ish, which is good, but have to otherwise visually check each
> > breakage until the fts_read problem is resolved - I know what the
> > issue is, but I don't have the auth to resolve it, so waiting on HPE
> > platform development for that. Of course, manually patching that many
> > breaks is equally unwieldy, so I'm willing to tolerate not having this
> > patch applied at this time.
> 
> Let me propose a different workaround. In my build on Windows, I inject a
> few blind spots in the test suite using GIT_SKIP_TESTS for cases where I do
> not have time to find a fix. It looks like this:
> 
> diff --git a/t/Makefile b/t/Makefile
> index 96317a35f4..fd8b18c3c0 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -103,3 +103,19 @@ perf:
>   $(MAKE) -C perf/ all
> 
>  .PHONY: pre-clean $(T) aggregate-results clean valgrind perf
> +
> +# see
> +https://public-inbox.org/git/alpine.DEB.2.21.1.1710260008270.37495@virt
> +ualbox/ # the suggested solution is for MSYS2; don't have time to fix
> +this for MSYS GIT_SKIP_TESTS += t0021.1[5-9] t0021.2[0-6] # special
> +file names GIT_SKIP_TESTS += t1300.14[02] # GIT_SSH_COMMAND with args
> +forwarded incompletely via git clone to test_fake_ssh GIT_SKIP_TESTS +=
> +t5601.5[01] # unknown failure in shallow submodule test GIT_SKIP_TESTS
> ++= t7406.46 # mktemp missing?
> +GIT_SKIP_TESTS += t7610.22
> +export GIT_SKIP_TESTS
> +
> +NO_SVN_TESTS=SkipThem
> +export NO_SVN_TESTS
> --
> 
> Build the list of test cases that do not pass, until the test suite runs 
> through.
> Then start fixing the cases.
> 
> It is not foolproof, but very effective in keeping the focus on new cases. You
> have to run tests with 'make' so that the variable is picked up. Also, when
> somebody adds new tests in front of the mentioned cases, the numbers
> must be adjusted.

I can live with this. Thanks for your advice. Let's hold off on applying my 
approach.

Cheers,
Randall

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Martin Fick]

On Sunday, January 07, 2018 01:14:41 PM Derrick Stolee 
wrote:
> This RFC includes a new way to index the objects in
> multiple packs using one file, called the multi-pack
> index (MIDX).
...
> The main goals of this RFC are:
> 
> * Determine interest in this feature.
> 
> * Find other use cases for the MIDX feature.

My interest in this feature would be to speed up fetches 
when there is more than one large pack-file with many of the 
same objects that are in other pack-files.   What does your 
MIDX design do when it encounters multiple copies of the 
same object in different pack files?  Does it index them all, 
or does it keep a single copy?

In our Gerrit instance (Gerrit uses jgit), we have multiple 
copies of the linux kernel repos linked together via the 
alternatives file mechanism.  These repos have many different 
references (mostly Gerrit change references), but they share 
most of the common objects from the mainline.  I have found 
that during a large fetch such as a clone, jgit spends a 
significant amount of extra time by having the extra large 
pack-files from the other repos visible to it, usually around 
an extra minute per instance of these (without them, the 
clone takes around 7mins).  This adds up easily with a few 
repos extra repos, it can almost double the time.

My investigations have shown that this is due to jgit 
searching each of these pack files to decide which version of 
each object to send.  I don't fully understand its selection 
criteria, however if I shortcut it to just pick the first 
copy of an object that it finds, I regain my lost time.  I 
don't know if git suffers from a similar problem?  If git 
doesn't suffer from this then it likely just uses the first 
copy of an object it finds (which may not be the best object 
to send?)

It would be nice if this use case could be improved with 
MIDX.  To do so, it seems that it would either require that 
MIDX either only put "the best" version of an object (i.e. 
pre-select which one to use), or include the extra 
information to help make the selection process of which copy 
to use (perhaps based on the operation being performed) 
fast.

This also leads me to ask, what other additional information 
(bitmaps?) for other operations, besides object location, 
might suddenly be valuable in an index that potentially 
points to multiple copies of objects?  Would such 
information be appropriate in MIDX, or would it be better in 
another index?

Thanks,

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Johannes Sixt]

Am 10.01.2018 um 01:12 schrieb Randall S. Becker:
> On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> I'm encountering strange warnings, while looking into the details of what 
> test t0001 fails in spots. These include:
> #24 warning: templates not found x00...[lots of 0 
> deleted...]
> which exceeds 2K, but that's just content, right, and not causing an apparent 
> breakage.

This warning occurs also on Linux and Windows. I think it is by design
and not something to be fixed.

> 
> # 34. Admittedly it was shorter than 2K, but there is something weird in this 
> path that I can't find, causing a failure out of fts_read from gnulib.
> Initialized empty Git repository in /home/ituglib/randall/git/t/trash 
> directory.t0001-init/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/newdir/.git/
> rm: fts_read failed: No such file or directory
> 
> This error is coming from some of shell utilities (in this case rm)
> used in the test rather than git code itself.

So, the problem is not in the git executable. This does not warrant a
change in the build process, yet.

>  While well within the
> supported path length of the operating system/platform (1K), there is
> an acknowledged issue that is causing breakage when paths get large
> enough (even only this large, unfortunately). We're at 221 breaks out
> of 12982-ish, which is good, but have to otherwise visually check
> each breakage until the fts_read problem is resolved - I know what
> the issue is, but I don't have the auth to resolve it, so waiting on
> HPE platform development for that. Of course, manually patching that
> many breaks is equally unwieldy, so I'm willing to tolerate not
> having this patch applied at this time.

Let me propose a different workaround. In my build on Windows, I inject
a few blind spots in the test suite using GIT_SKIP_TESTS for cases where
I do not have time to find a fix. It looks like this:

diff --git a/t/Makefile b/t/Makefile
index 96317a35f4..fd8b18c3c0 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,3 +103,19 @@ perf:
$(MAKE) -C perf/ all
 
 .PHONY: pre-clean $(T) aggregate-results clean valgrind perf
+
+# see 
https://public-inbox.org/git/alpine.DEB.2.21.1.1710260008270.37495@virtualbox/
+# the suggested solution is for MSYS2; don't have time to fix this for MSYS
+GIT_SKIP_TESTS += t0021.1[5-9] t0021.2[0-6]
+# special file names
+GIT_SKIP_TESTS += t1300.14[02]
+# GIT_SSH_COMMAND with args forwarded incompletely via git clone to 
test_fake_ssh
+GIT_SKIP_TESTS += t5601.5[01]
+# unknown failure in shallow submodule test
+GIT_SKIP_TESTS += t7406.46
+# mktemp missing?
+GIT_SKIP_TESTS += t7610.22
+export GIT_SKIP_TESTS
+
+NO_SVN_TESTS=SkipThem
+export NO_SVN_TESTS
-- 

Build the list of test cases that do not pass, until the test suite runs
through. Then start fixing the cases.

It is not foolproof, but very effective in keeping the focus on new
cases. You have to run tests with 'make' so that the variable is picked
up. Also, when somebody adds new tests in front of the mentioned cases,
the numbers must be adjusted.

-- Hannes

Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set

[Brandon Williams]

On 01/10, Nguyễn Thái Ngọc Duy wrote:
> Occasionally submodule code could execute new commands with GIT_DIR set
> to some submodule. GIT_TRACE prints just the command line which makes it
> hard to tell that it's not really executed on this repository.
> 
> Print env variables in this case. Note that the code deliberately ignore
> variables unsetting because there are so many of them (to keep git
> environment clean for the next process) and really hard to read.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  A minor thing that I ignored in this patch is quoting the environment
>  variables. For me it's meh. Perhaps I should just dumb the env
>  without quoting at all.

A patch like this would have been very helpful to me on some previous
occasions, so thanks for writing it up.

> 
>  run-command.c |  3 ++-
>  trace.c   | 38 +++---
>  trace.h   | 18 +++---
>  3 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 31fc5ea86e..235367087d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)
>   cmd->err = fderr[0];
>   }
>  
> - trace_argv_printf(cmd->argv, "trace: run_command:");
> + trace_env_argv_printf(cmd->env, cmd->argv, "trace: run_command:");
> +
>   fflush(NULL);
>  
>  #ifndef GIT_WINDOWS_NATIVE
> diff --git a/trace.c b/trace.c
> index b7530b51a9..d8967b66e8 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int line, 
> struct trace_key *key,
>   print_trace_line(key, );
>  }
>  
> +static void concatenate_env(struct strbuf *dst, const char *const *env)
> +{
> + int i;
> +
> + /* Copy into destination buffer. */
> + strbuf_grow(dst, 255);
> + for (i = 0; env[i]; ++i) {
> + /*
> +  * the main interesting is setting new vars
> +  * e.g. GIT_DIR, ignore the unsetting to reduce noise.
> +  */

I think you're missing a word, maybe:
  'The main interesting part is setting new vars'

> + if (!strchr(env[i], '='))
> + continue;
> + strbuf_addch(dst, ' ');
> + sq_quote_buf(dst, env[i]);
> + }

At first when i read this I was under the impression that the whole
environment was going to be printed out...but i now realize that this
tracing  will only print out the delta's or the additions to the
environment that the child will see.  Maybe this should be called out
more clearly in the commit message?

> +}
> +
>  static void trace_argv_vprintf_fl(const char *file, int line,
> +   const char *const *env,
> const char **argv, const char *format,
> va_list ap)
>  {
> @@ -157,6 +176,9 @@ static void trace_argv_vprintf_fl(const char *file, int 
> line,
>  
>   strbuf_vaddf(, format, ap);
>  
> + if (env)
> + concatenate_env(, env);
> +
>   sq_quote_argv(, argv, 0);
>   print_trace_line(_default_key, );
>  }
> @@ -214,7 +236,16 @@ void trace_argv_printf(const char **argv, const char 
> *format, ...)
>  {
>   va_list ap;
>   va_start(ap, format);
> - trace_argv_vprintf_fl(NULL, 0, argv, format, ap);
> + trace_argv_vprintf_fl(NULL, 0, NULL, argv, format, ap);
> + va_end(ap);
> +}
> +
> +void trace_env_argv_printf(const char *const *env, const char **argv,
> +const char *format, ...)
> +{
> + va_list ap;
> + va_start(ap, format);
> + trace_argv_vprintf_fl(NULL, 0, env, argv, format, ap);
>   va_end(ap);
>  }
>  
> @@ -251,12 +282,13 @@ void trace_printf_key_fl(const char *file, int line, 
> struct trace_key *key,
>   va_end(ap);
>  }
>  
> -void trace_argv_printf_fl(const char *file, int line, const char **argv,
> +void trace_argv_printf_fl(const char *file, int line,
> +   const char *const *env, const char **argv,
> const char *format, ...)
>  {
>   va_list ap;
>   va_start(ap, format);
> - trace_argv_vprintf_fl(file, line, argv, format, ap);
> + trace_argv_vprintf_fl(file, line, env, argv, format, ap);
>   va_end(ap);
>  }
>  
> diff --git a/trace.h b/trace.h
> index 88055abef7..830d9dcd19 100644
> --- a/trace.h
> +++ b/trace.h
> @@ -34,6 +34,10 @@ extern void trace_printf_key(struct trace_key *key, const 
> char *format, ...);
>  __attribute__((format (printf, 2, 3)))
>  extern void trace_argv_printf(const char **argv, const char *format, ...);
>  
> +__attribute__((format (printf, 3, 4)))
> +extern void trace_env_argv_printf(const char * const*env, const char **argv,
> +   const char *format, ...);
> +
>  extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
>  
>  /* Prints elapsed time (in nanoseconds) if 

Please reply immediately,

[Mrs. Sarah Faith.]

Dear Friend,

I know that this message will look strange, surprising and probably 
unbelievable to you, but it is true and reality. I want to transfer the sum of: 
￡35,000,000 (Thirty Five Million British Pounds) to you. THIS IS NOT A JOKE, 
but after reading if it does not interest you please kindly delete it. I am 
contacting you by the Will of God. My name is: Mrs. Sarah  Faith, I am a 
British business woman specialized in mining of raw Gold in Africa; but now I 
am critically sick with esophageal cancer which has damaged almost all the 
cells in my body system and I will soon die according to my doctors.

My late husband died in an accident with our two daughters few years ago 
leaving me with our only son whom is just 12 years old and he is my most 
concern now as he is still a child and does not know anything about live and 
has nobody to take care of him after I am dead; because I and my late husband 
does not have any relatives, we both grew up in the orphanage home and got 
married under orphanage as orphans.

 So if I die now my innocent child would be left alone in this wicked world and 
I do not wish to send him to any orphanage home, I want him to grow up in the 
hands of an individual, not orphanage. Please, i am begging you in the name of 
God to sincerely accept my proposal; let me instruct my bank to wire transfer 
my fund worth the sum of: ￡35 MILLION POUNDS to your account in your country 
immediately, then you take my son to your home and raise him as your own son.

Please reply immediately so that we can further discuss the details fast before 
anything happens to me.

Yours Sincerely,
Mrs. Sarah  Faith.

Re: [PATCH 26/26] remote-curl: implement connect-half-duplex command

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:28 -0800
Brandon Williams  wrote:

> +static size_t proxy_in(void *ptr, size_t eltsize,
> +size_t nmemb, void *buffer_)

OK, I managed to look at the Curl stuff in more detail.

I know that these parameter names are what remote_curl.c has been using
for its callbacks, but I find them confusing (in particular, some Curl
documentation rightly refer to the 1st parameter as a buffer, and the
4th parameter is actually userdata). Also, according to the Curl
documentation, the type of the first parameter is "char *". Could we
change the type of the first parameter to "char *", and the name of the
fourth parameter either to "proxy_state_" or "userdata"?

> +{
> + size_t max = eltsize * nmemb;
> + struct proxy_state *p = buffer_;
> + size_t avail = p->request_buffer.len - p->pos;
> +
> + if (!avail) {
> + if (p->seen_flush) {
> + p->seen_flush = 0;
> + return 0;
> + }
> +
> + strbuf_reset(>request_buffer);
> + switch (packet_reader_read(>reader)) {
> + case PACKET_READ_EOF:
> + die("error reading request from parent process");

This should say "BUG:", I think. I'm not sure what the best way of
explaining it is, but basically connect_half_duplex is supposed to
ensure (by peeking) that there is no EOF when proxy_in() is called.

> + case PACKET_READ_NORMAL:
> + packet_buf_write_len(>request_buffer, p->reader.line,
> +  p->reader.pktlen);
> + break;
> + case PACKET_READ_DELIM:
> + packet_buf_delim(>request_buffer);
> + break;
> + case PACKET_READ_FLUSH:
> + packet_buf_flush(>request_buffer);
> + p->seen_flush = 1;
> + break;
> + }
> + p->pos = 0;
> + avail = p->request_buffer.len;
> + }
> +
> + if (max < avail)
> + avail = max;
> + memcpy(ptr, p->request_buffer.buf + p->pos, avail);
> + p->pos += avail;
> + return avail;

Thanks, this looks correct. I wish that the Curl API had a way for us to
say "here are 4 more bytes, and that is all" instead of us having to
make a note (p->seen_flush) to remember to return 0 on the next call,
but that's the way it is.

> +}
> +static size_t proxy_out(char *ptr, size_t eltsize,
> + size_t nmemb, void *buffer_)

Add a blank line before proxy_out. Also, same comment as proxy_in()
about the function signature.

> +{
> + size_t size = eltsize * nmemb;
> + struct proxy_state *p = buffer_;
> +
> + write_or_die(p->out, ptr, size);
> + return size;
> +}
> +
> +static int proxy_post(struct proxy_state *p)
> +{
> + struct active_request_slot *slot;
> + struct curl_slist *headers = http_copy_default_headers();
> + int err;
> +
> + headers = curl_slist_append(headers, p->hdr_content_type);
> + headers = curl_slist_append(headers, p->hdr_accept);
> + headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
> +
> + slot = get_active_slot();
> +
> + curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
> + curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
> + curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
> + curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);

I looked at the Curl documentation for CURLOPT_HTTPHEADER and
curl_easy_setopt doesn't consume the argument here (in fact, it asks us
to keep "headers" around), so it might be possible to just generate the
headers once in proxy_state_init().

> +
> + /* Setup function to read request from client */
> + curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);
> + curl_easy_setopt(slot->curl, CURLOPT_READDATA, p);
> +
> + /* Setup function to write server response to client */
> + curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);
> + curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);
> +
> + err = run_slot(slot, NULL);
> +
> + if (err != HTTP_OK)
> + err = -1;

This seems to mean that we cannot have two requests in flight at the
same time even while there is no response (from the fact that we have a
HTTP status code after returning from run_slot()).

I thought that git fetch over HTTP uses the two-requests-in-flight
optimization that it also does over other protocols like SSH, but I see
that that code path (fetch_git() in remote-curl.c) also uses run_slot()
indirectly, so maybe my assumption is wrong. Anyway, this is outside the
scope of this patch.

> +
> + curl_slist_free_all(headers);
> + return err;
> +}
> +
> +static int connect_half_duplex(const char *service_name)
> +{
> + struct discovery *discover;
> + struct proxy_state p;
> +
> + /*
> +  * Run the info/refs request 

Re: [git-for-windows] Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Sixt]

Am 10.01.2018 um 18:37 schrieb Johannes Schindelin:

On Tue, 9 Jan 2018, Junio C Hamano wrote:

Johannes Schindelin  writes:


diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index f1678851de9..470107248eb 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -31,7 +31,22 @@
  #
  
  use 5.008;

-use lib (split(/:/, $ENV{GITPERLLIB}));
+sub gitperllib {
+...
+   if ($ENV{GITPERLLIB} =~ /;/) {
+   return split(/;/, $ENV{GITPERLLIB});
+   }
+   return split(/:/, $ENV{GITPERLLIB});
+}


This cannot be the whole story for a few reasons.

  - In t/test-lib.sh we see this:


GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
export GITPERLLIB

If this part wants to split with ';', then the joining needs to
be done with ';' to match, no?


No.

It is a lot more complicated than that. As you know, on Linux there is
this implicit assumption that path lists are colon-separated. As a
consequence, Cygwin does the same (because it would be too hard to port
all those Linux/Unix projects to stop assuming colon-separated path lists,
right?)

This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for
Windows, too).

Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence
the MSYS2 runtime knows that it has to convert the path lists to Windows
paths separated by semicolons.

The next thing happening in our case is that the Perl script is called
from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl
interpreter) does *not* convert those path lists back to Unix-like paths
separated by colons.


But this is a bug in MSYS2, isn't it? The MSYS2 runtime should detect 
that it was not invoked by some other MSYS2 process. The MSYS2 startup 
sequence should assume in this case that the environment is 
Windows-style and convert to POSIX before it calls into perl's main().



And that's why the Unix shell script can happily construct the
colon-separated list, and the Perl script will *still* receive the
semicolon-separated version of it.


  - In addition to t0021, there are similar split with colon in 0202,
9000 and 9700, yet I am getting the feeling that you observed the
issue only in0021, to which I do not think of a good explanation
why.


Here is the good explanation: t0021 relies on a Perl package that is not
yet installed. t0202 relies on Git::I18N, of which there is a version
installed in Git for Windows' SDK. (I do not bother to slow down the test
runs by the Subversion tests, I always skip all of them, that's why t9*
does not matter to me.)


t0202 and the t9* cases are different because perl is invoked by bash 
directly (AFAICS), without a non-MSYS2 process between them. There is no 
difference when the path conversion is omitted in this case by design or 
due to a bug.


-- Hannes

Subject for default customers

[jsguerra7]

Dear Sir or Madam,

this is a default email message that gets sent out automatically in the 
licensed version.
This is written in English and will be send out to all people if not some other 
template might match (e.g. the *@*.de one).

As you may see here, you can use different tags like word1 to generate a 
different email each time.

You can also use the following things here:

 git@vger.kernel.org - gets replaced with the email of the person you are 
emailing to
 https://git-scm.com/community - the url where this email was found
 git-scm.com - the domain of the url where the email was found
 N/A - if extra data was collected it will be inserted here


With best Regards
Email Spider
---
Attention! Please never use our software to spam other people.

Re: [PATCH] t3900: add some more quotes

[Eric Sunshine]

On Wed, Jan 10, 2018 at 4:58 AM, Beat Bolli  wrote:
> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> hander, two files are deleted which must be quoted individually.

s/hander/handler/

> Signed-off-by: Beat Bolli 

Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Schindelin]

Hi Junio,

On Tue, 9 Jan 2018, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Johannes Schindelin  writes:
> >
> >> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> >> index f1678851de9..470107248eb 100644
> >> --- a/t/t0021/rot13-filter.pl
> >> +++ b/t/t0021/rot13-filter.pl
> >> @@ -31,7 +31,22 @@
> >>  #
> >>  
> >>  use 5.008;
> >> -use lib (split(/:/, $ENV{GITPERLLIB}));
> >> +sub gitperllib {
> >> +...
> >> +  if ($ENV{GITPERLLIB} =~ /;/) {
> >> +  return split(/;/, $ENV{GITPERLLIB});
> >> +  }
> >> +  return split(/:/, $ENV{GITPERLLIB});
> >> +}
> >
> > This cannot be the whole story for a few reasons.
> >
> >  - In t/test-lib.sh we see this:
> >
> >
> > GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
> >export GITPERLLIB
> >
> >If this part wants to split with ';', then the joining needs to
> >be done with ';' to match, no?
> >
> >  - In addition to t0021, there are similar split with colon in 0202,
> >9000 and 9700, yet I am getting the feeling that you observed the
> >issue only in0021, to which I do not think of a good explanation
> >why.
> 
> This somehow vaguely rang a bell, and I dug this thing up from the
> archive, [*1*] which ended like so:
> 
> >> In our C code, we have "#define PATH_SEP ';'", and encourage
> >> our code to be careful and use it.  Is there something
> >> similar for Perl scripts, I wonder.
> >>
> > We probably should find a better solution to allow this to
> > work with windows style paths...? I know that python provides
> > os.pathsep, but I haven't seen an equivalent for perl yet.
> >
> > The Env[1] core modules suggests using
> > $Config::Config{path_sep}[2]..  maybe we should be using this?
> 
> I was testing this recently on the Perl included with Git for
> Windows and it returns : for the path separator even though it's
> on Windows, so I don't think that would work. The Perl in Git
> for Windows seems to want UNIX-style inputs (something Dscho
> seemed to allude to in his response earlier.). I'm not sure why
> it's that way, but he probably knows.
> 
> Your initial response in this thread made it sound as if -rc1 is the
> only thing that changed, but looking at the differences between -rc0
> and -rc1, which does not touch t0021 or any other instances of
> "split(/:/, $ENV{GITPERLLIB})", I am wondering if it is possible
> that perhaps the way Perl is built for GfW has been changed recently
> and we can safely and sanely use $Config::Config{path_sep} (contrary
> to what was found in late Oct in the message quoted above) now?

No, the only thing that changed was the introduction of Git::Packet (and
t0021/*.perl uses that). And that Perl module is not yet installed.

Granted, we tested incorrect versions of those modules, but this late in
the -rc phase, I would prefer to be cautious. People might be relying on
the current bug.

OTOH I might be way too pessimistic and we should essentially replicate
that `sub gitperllib` trick in *all* of our Perl scripts.

What do you think?

Ciao,
Dscho

Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Schindelin]

Hi Junio,

On Tue, 9 Jan 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> > index f1678851de9..470107248eb 100644
> > --- a/t/t0021/rot13-filter.pl
> > +++ b/t/t0021/rot13-filter.pl
> > @@ -31,7 +31,22 @@
> >  #
> >  
> >  use 5.008;
> > -use lib (split(/:/, $ENV{GITPERLLIB}));
> > +sub gitperllib {
> > +...
> > +   if ($ENV{GITPERLLIB} =~ /;/) {
> > +   return split(/;/, $ENV{GITPERLLIB});
> > +   }
> > +   return split(/:/, $ENV{GITPERLLIB});
> > +}
> 
> This cannot be the whole story for a few reasons.
> 
>  - In t/test-lib.sh we see this:
> 
>
> GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
>export GITPERLLIB
> 
>If this part wants to split with ';', then the joining needs to
>be done with ';' to match, no?

No.

It is a lot more complicated than that. As you know, on Linux there is
this implicit assumption that path lists are colon-separated. As a
consequence, Cygwin does the same (because it would be too hard to port
all those Linux/Unix projects to stop assuming colon-separated path lists,
right?)

This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for
Windows, too).

Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence
the MSYS2 runtime knows that it has to convert the path lists to Windows
paths separated by semicolons.

The next thing happening in our case is that the Perl script is called
from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl
interpreter) does *not* convert those path lists back to Unix-like paths
separated by colons.

And that's why the Unix shell script can happily construct the
colon-separated list, and the Perl script will *still* receive the
semicolon-separated version of it.

>  - In addition to t0021, there are similar split with colon in 0202,
>9000 and 9700, yet I am getting the feeling that you observed the
>issue only in0021, to which I do not think of a good explanation
>why.

Here is the good explanation: t0021 relies on a Perl package that is not
yet installed. t0202 relies on Git::I18N, of which there is a version
installed in Git for Windows' SDK. (I do not bother to slow down the test
runs by the Subversion tests, I always skip all of them, that's why t9*
does not matter to me.)

Granted, that is a bug, and an old one at that: the test suite should test
what is in the current revision, not what happens to be installed into the
system locations.

But this late in the game, I am really uncomfortable with the idea to rush
through an equivalent fix to all Perl scripts: who knows what it breaks?

Ciao,
Dscho

Re: [PATCH] t3900: add some more quotes

[Johannes Schindelin]

Hi,

On Wed, 10 Jan 2018, Jeff King wrote:

> On Wed, Jan 10, 2018 at 10:58:32AM +0100, Beat Bolli wrote:
> 
> > In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> > were added to protect against spaces in $HOME. In the test_when_finished
> > hander, two files are deleted which must be quoted individually.
> 
> Doh. I can't believe I missed that when reading the patch.

D'oh, and I can't believe that I missed this when writing the patch.

Thanks, Beat!
Dscho

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Johannes Schindelin]

Hi Stefan,

On Tue, 9 Jan 2018, Stefan Beller wrote:

> On Tue, Jan 9, 2018 at 5:05 AM, Johannes Schindelin
>  wrote:
>
> > Just to throw this out there: --abbrev=8! would be one possible
> > convention to state "I want exactly 8 hex digits, don't bother
> > checking for uniqueness".
> >
> > Not sure how urgent such a feature is.
> 
> You seem to have spent some time on rebase, including lamenting on the
> in-expressiveness of the instruction sheet (c.f. "rebase a mergy history
> sucks")

Let's call it "todo list" again?

And yes, I spend a *lot* of time in rebase. It's part of my duties as Git
for Windows maintainer.

> And in that light, I'd like to propose a new naming scheme:
> 
> (a) assume that we "tag" HEAD at the start of the rebase
> (b) any abbreviation must be given as committish anchored to said ref:
> 
> pick REBASE_HEAD~1 commit subject
> pick REBASE_HEAD~2 distak the gostim
> pick REBASE_HEAD~3 Document foo
> pick REBASE_HEAD~4 Testing the star-gazer

I do not necessarily agree that this is better because it is valid only
locally, only in the current rebase (and you enter new problems because
you implicitly require REBASE_HEAD to be worktree-local lest you prevent
simultaneous rebases in related worktrees).

That prevents you from, say, chatting to your colleague and mentioning
that commit that you are uncertain about. Imagine asking "Hey Todd, do you
know what REBASE_HEAD~2 is all about?". Granted, if you ask about
"deadbeef" and it just so happens that this shortened name is ambiguous in
Todd's repository. But that is pretty unlikely, isn't it?

> And as we have the full descriptiveness of the committishs there, each
> commit can be described in a unique way using the graph relationship.  I
> am just throwing the name REBASE_HEAD out there to trigger some
> associations ("similar to FETCH_HEAD"), but I dislike the name.

Not only that. It would also be confusing to read after reordering...

> (c) this would not solve the problem of mergy history, yet. For that
> we'd need to introduce more keywords, that allow us to move around in
> the DAG, [...]

You probably missed my hint that I have a working solution for that.
Please have a look at

https://github.com/git/git/pull/447

Short version: for a commit topology like this:

A - B - C
  \   /
D

the generated list would look like this:

# branch D
pick 0123 A
label branch-point
pick 1234 D
label D

reset branch-point
pick 2345 B
merge 3456 D C

Ciao,
Dscho

Re: upstreaming https://github.com/cgwalters/git-evtag ?

[Santiago Torres]

> > push for hash-agnosticity. I don't know if git-evtag is hash agnostic,
> > but if it is not, then we have two transition plans to think about.
> 
> I don't think there's even a question here: Git has to transition off
> of SHA-1.
> 
> In that context, Stefan's comment is a welcome one: once we've
> transitioned off of SHA-1, having a separate evtag feature would make
> git more complicated without any benefit to match.  To put it another
> way, the gpgsig-sha256 field described in
> Documentation/technical/hash-function-transition.txt provides
> essentially the same functionality as an evtag.  What's missing is an
> implementation of it.
> 
> I'm happy to help in any way I can (reviews, advice, etc).

Same here, although I'm a bit swamped with other work... 

> 
> > Full disclosure, I published a "competing" solution a couple of years
> > ago[1] but, in my personal opinion, I think push certificates can
> > achieve the same security guarantees as my system with very little
> > changes.
> 
> Work to improve the usability of push certs would also be very very
> welcome.

I agree. I personally think that at least the sample hook work on here
would be a good candidate for this[1], although I don't know what's the
status of it. The way they are right now, they should at least warn when
push certificates are not enabled on the server side (i.e., there is no
hook to handle it).

> 
> Thanks and hope that helps,
> Jonathan

No, thanks to you :)

-Santiago.

[1] https://public-inbox.org/git/20171202091248.6037-1-r...@shikherverma.com/


signature.asc
Description: PGP signature

Re: How to use multiple SSH keys on Git exe (not bash)

[Sam Millman]

Aha, thanks, I'll go annoy them :P

On 10 January 2018 at 16:29, Randall S. Becker  wrote:
> OpenSSH generally. Other providers (and platform providers) exist as well. It 
> is hard to know which is really involved, but not git.
>
>> -Original Message-
>> From: Sam Millman [mailto:sam.mill...@gmail.com]
>> Sent: January 10, 2018 11:26 AM
>> To: Randall S. Becker 
>> Cc: Ævar Arnfjörð Bjarmason ; git@vger.kernel.org
>> Subject: Re: How to use multiple SSH keys on Git exe (not bash)
>>
>> Does the ssh.exe come from OpenSSH? I thought it was Git's implementation
>> of the SSH protocol
>>
>> On 10 January 2018 at 16:23, Randall S. Becker 
>> wrote:
>> > May I, with respect, ask you to take this to the OpenSSH email list?
>> (openssh-unix-...@mindrot.org)  I think the discussion better belongs there
>> and you're likely to get more detailed information from that team.
>> > Sincerely,
>> > Randall
>> >
>> >> -Original Message-
>> >> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
>> >> Behalf Of Sam Millman
>> >> Sent: January 10, 2018 11:03 AM
>> >> To: Ævar Arnfjörð Bjarmason 
>> >> Cc: git@vger.kernel.org
>> >> Subject: Re: How to use multiple SSH keys on Git exe (not bash)
>> >>
>> >> I actually played a bit more and got this:
>> >>
>> >> Host *
>> >> IdentityFile ~/.ssh/id_rsa_d
>> >> IdentityFile ~/.ssh/id_rsa
>> >>
>> >> Host bitbucket_1
>> >> User git
>> >> HostName bitbucket.org
>> >> IdentityFile ~/.ssh/id_rsa_d
>> >>
>> >> Host bitbucket_2
>> >> User git
>> >> HostName bitbucket.org
>> >> IdentityFile ~/.ssh/id_rsa
>> >>
>> >> And from basic testing it seems to actually work, it seems it is the
>> >> Host * that make sit work and it will actually iterate the keys and try 
>> >> them.
>> >>
>> >> Not sure why this is, any thoughts?
>> >>
>> >> On 10 January 2018 at 15:58, Ævar Arnfjörð Bjarmason
>> >> 
>> >> wrote:
>> >> >
>> >> > On Wed, Jan 10 2018, Sam Millman jotted:
>> >> >
>> >> >> I am trying, for the sake of PhpStorm, to get multiple SSH keys
>> >> >> working using git . exe, which means no GitBash.
>> >> >>
>> >> >> I can get the keys to work just fine with GitBash.
>> >> >>
>> >> >> I edited my .ssh/config to look like (I know this is incorrect):
>> >> >>
>> >> >> Host bitucket . org
>> >> >> IdentityFile ~/.ssh/id_rsa1
>> >> >>
>> >> >> Host bitbucket . org
>> >> >> IdentityFile ~/.ssh/id_rsa
>> >> >>
>> >> >>
>> >> >> And id_rsa1 works, I can actually pick from the other repo. But,
>> >> >> of course, id_rsa does not now.
>> >> >>
>> >> >> I change to:
>> >> >>
>> >> >> Host bitucket . org-dd
>> >> >> HostName bitbucket . org
>> >> >> IdentityFile ~/.ssh/id_rsa1
>> >> >>
>> >> >> Host bitbucket . org-sas
>> >> >> HostName bitbucket . org
>> >> >> IdentityFile ~/.ssh/id_rsa
>> >> >>
>> >> >> And now only id_rsa works.
>> >> >>
>> >> >> I also tried combining the two IdentityFile lines together like so
>> >> >> (for some reason):
>> >> >>
>> >> >> Host bitucket . org
>> >> >> IdentityFile ~/.ssh/id_rsa1
>> >> >> IdentityFile ~/.ssh/id_rsa
>> >> >>
>> >> >> I have even tried running ssh-agent . exe, adding id_rsa1 to that
>> >> >> and then running the git clone with no result.
>> >> >>
>> >> >> The weird thing is, I have two public keys as well and they both
>> >> >> load in the ssh . exe (they return errors about format), I just
>> >> >> cannot get my ssh . exe to work with multiple private keys.
>> >> >
>> >> > This might just be a special case of the problem of some hosting
>> >> > providers picking only the first key you provide, as described in
>> >> > this
>> >> > thread:
>> >> > https://public-inbox.org/git/20180103102840.27897-1-ava...@gmail.co
>> >> > m/
>> >> >
>> >> > If so, you either need to hack around this with ssh host aliases,
>> >> > or a custom GIT_SSH_COMMAND.
>> >> >
>> >> >> On 10 January 2018 at 15:29, Sam Millman 
>> >> wrote:
>> >> >>> I am trying, for the sake of PhpStorm, to get multiple SSH keys
>> >> >>> working using git . exe, which means no GitBash.
>> >> >>>
>> >> >>> I can get the keys to work just fine with GitBash.
>> >> >>>
>> >> >>> I edited my .ssh/config to look like (I know this is incorrect):
>> >> >>>
>> >> >>> Host bitucket . org
>> >> >>> IdentityFile ~/.ssh/id_rsa1
>> >> >>>
>> >> >>> Host bitbucket . org
>> >> >>> IdentityFile ~/.ssh/id_rsa
>> >> >>>
>> >> >>>
>> >> >>> And id_rsa1 works, I can actually pick from the other repo. But,
>> >> >>> of course, id_rsa does not now.
>> >> >>>
>> >> >>> I change to:
>> >> >>>
>> >> >>> Host bitucket . org-dd
>> >> >>> HostName bitbucket . org
>> >> >>> IdentityFile ~/.ssh/id_rsa1
>> >> >>>
>> >> >>> Host bitbucket . org-sas
>> >> >>> HostName bitbucket . org
>> >> >>> IdentityFile ~/.ssh/id_rsa
>> >> >>>
>> >> >>> And now only id_rsa works.
>> >> >>>
>> >> >>> I also tried combining the two IdentityFile lines together 

RE: How to use multiple SSH keys on Git exe (not bash)

[Randall S. Becker]

OpenSSH generally. Other providers (and platform providers) exist as well. It 
is hard to know which is really involved, but not git.

> -Original Message-
> From: Sam Millman [mailto:sam.mill...@gmail.com]
> Sent: January 10, 2018 11:26 AM
> To: Randall S. Becker 
> Cc: Ævar Arnfjörð Bjarmason ; git@vger.kernel.org
> Subject: Re: How to use multiple SSH keys on Git exe (not bash)
> 
> Does the ssh.exe come from OpenSSH? I thought it was Git's implementation
> of the SSH protocol
> 
> On 10 January 2018 at 16:23, Randall S. Becker 
> wrote:
> > May I, with respect, ask you to take this to the OpenSSH email list?
> (openssh-unix-...@mindrot.org)  I think the discussion better belongs there
> and you're likely to get more detailed information from that team.
> > Sincerely,
> > Randall
> >
> >> -Original Message-
> >> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
> >> Behalf Of Sam Millman
> >> Sent: January 10, 2018 11:03 AM
> >> To: Ævar Arnfjörð Bjarmason 
> >> Cc: git@vger.kernel.org
> >> Subject: Re: How to use multiple SSH keys on Git exe (not bash)
> >>
> >> I actually played a bit more and got this:
> >>
> >> Host *
> >> IdentityFile ~/.ssh/id_rsa_d
> >> IdentityFile ~/.ssh/id_rsa
> >>
> >> Host bitbucket_1
> >> User git
> >> HostName bitbucket.org
> >> IdentityFile ~/.ssh/id_rsa_d
> >>
> >> Host bitbucket_2
> >> User git
> >> HostName bitbucket.org
> >> IdentityFile ~/.ssh/id_rsa
> >>
> >> And from basic testing it seems to actually work, it seems it is the
> >> Host * that make sit work and it will actually iterate the keys and try 
> >> them.
> >>
> >> Not sure why this is, any thoughts?
> >>
> >> On 10 January 2018 at 15:58, Ævar Arnfjörð Bjarmason
> >> 
> >> wrote:
> >> >
> >> > On Wed, Jan 10 2018, Sam Millman jotted:
> >> >
> >> >> I am trying, for the sake of PhpStorm, to get multiple SSH keys
> >> >> working using git . exe, which means no GitBash.
> >> >>
> >> >> I can get the keys to work just fine with GitBash.
> >> >>
> >> >> I edited my .ssh/config to look like (I know this is incorrect):
> >> >>
> >> >> Host bitucket . org
> >> >> IdentityFile ~/.ssh/id_rsa1
> >> >>
> >> >> Host bitbucket . org
> >> >> IdentityFile ~/.ssh/id_rsa
> >> >>
> >> >>
> >> >> And id_rsa1 works, I can actually pick from the other repo. But,
> >> >> of course, id_rsa does not now.
> >> >>
> >> >> I change to:
> >> >>
> >> >> Host bitucket . org-dd
> >> >> HostName bitbucket . org
> >> >> IdentityFile ~/.ssh/id_rsa1
> >> >>
> >> >> Host bitbucket . org-sas
> >> >> HostName bitbucket . org
> >> >> IdentityFile ~/.ssh/id_rsa
> >> >>
> >> >> And now only id_rsa works.
> >> >>
> >> >> I also tried combining the two IdentityFile lines together like so
> >> >> (for some reason):
> >> >>
> >> >> Host bitucket . org
> >> >> IdentityFile ~/.ssh/id_rsa1
> >> >> IdentityFile ~/.ssh/id_rsa
> >> >>
> >> >> I have even tried running ssh-agent . exe, adding id_rsa1 to that
> >> >> and then running the git clone with no result.
> >> >>
> >> >> The weird thing is, I have two public keys as well and they both
> >> >> load in the ssh . exe (they return errors about format), I just
> >> >> cannot get my ssh . exe to work with multiple private keys.
> >> >
> >> > This might just be a special case of the problem of some hosting
> >> > providers picking only the first key you provide, as described in
> >> > this
> >> > thread:
> >> > https://public-inbox.org/git/20180103102840.27897-1-ava...@gmail.co
> >> > m/
> >> >
> >> > If so, you either need to hack around this with ssh host aliases,
> >> > or a custom GIT_SSH_COMMAND.
> >> >
> >> >> On 10 January 2018 at 15:29, Sam Millman 
> >> wrote:
> >> >>> I am trying, for the sake of PhpStorm, to get multiple SSH keys
> >> >>> working using git . exe, which means no GitBash.
> >> >>>
> >> >>> I can get the keys to work just fine with GitBash.
> >> >>>
> >> >>> I edited my .ssh/config to look like (I know this is incorrect):
> >> >>>
> >> >>> Host bitucket . org
> >> >>> IdentityFile ~/.ssh/id_rsa1
> >> >>>
> >> >>> Host bitbucket . org
> >> >>> IdentityFile ~/.ssh/id_rsa
> >> >>>
> >> >>>
> >> >>> And id_rsa1 works, I can actually pick from the other repo. But,
> >> >>> of course, id_rsa does not now.
> >> >>>
> >> >>> I change to:
> >> >>>
> >> >>> Host bitucket . org-dd
> >> >>> HostName bitbucket . org
> >> >>> IdentityFile ~/.ssh/id_rsa1
> >> >>>
> >> >>> Host bitbucket . org-sas
> >> >>> HostName bitbucket . org
> >> >>> IdentityFile ~/.ssh/id_rsa
> >> >>>
> >> >>> And now only id_rsa works.
> >> >>>
> >> >>> I also tried combining the two IdentityFile lines together like
> >> >>> so (for some
> >> >>> reason):
> >> >>>
> >> >>> Host bitucket . org
> >> >>> IdentityFile ~/.ssh/id_rsa1
> >> >>> IdentityFile ~/.ssh/id_rsa
> >> >>>
> >> >>> I have even tried running ssh-agent . exe, adding id_rsa1 to that
> >> >>> and then 

Re: How to use multiple SSH keys on Git exe (not bash)

[Sam Millman]

Does the ssh.exe come from OpenSSH? I thought it was Git's
implementation of the SSH protocol

On 10 January 2018 at 16:23, Randall S. Becker  wrote:
> May I, with respect, ask you to take this to the OpenSSH email list? 
> (openssh-unix-...@mindrot.org)  I think the discussion better belongs there 
> and you're likely to get more detailed information from that team.
> Sincerely,
> Randall
>
>> -Original Message-
>> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
>> Behalf Of Sam Millman
>> Sent: January 10, 2018 11:03 AM
>> To: Ævar Arnfjörð Bjarmason 
>> Cc: git@vger.kernel.org
>> Subject: Re: How to use multiple SSH keys on Git exe (not bash)
>>
>> I actually played a bit more and got this:
>>
>> Host *
>> IdentityFile ~/.ssh/id_rsa_d
>> IdentityFile ~/.ssh/id_rsa
>>
>> Host bitbucket_1
>> User git
>> HostName bitbucket.org
>> IdentityFile ~/.ssh/id_rsa_d
>>
>> Host bitbucket_2
>> User git
>> HostName bitbucket.org
>> IdentityFile ~/.ssh/id_rsa
>>
>> And from basic testing it seems to actually work, it seems it is the Host * 
>> that
>> make sit work and it will actually iterate the keys and try them.
>>
>> Not sure why this is, any thoughts?
>>
>> On 10 January 2018 at 15:58, Ævar Arnfjörð Bjarmason 
>> wrote:
>> >
>> > On Wed, Jan 10 2018, Sam Millman jotted:
>> >
>> >> I am trying, for the sake of PhpStorm, to get multiple SSH keys
>> >> working using git . exe, which means no GitBash.
>> >>
>> >> I can get the keys to work just fine with GitBash.
>> >>
>> >> I edited my .ssh/config to look like (I know this is incorrect):
>> >>
>> >> Host bitucket . org
>> >> IdentityFile ~/.ssh/id_rsa1
>> >>
>> >> Host bitbucket . org
>> >> IdentityFile ~/.ssh/id_rsa
>> >>
>> >>
>> >> And id_rsa1 works, I can actually pick from the other repo. But, of
>> >> course, id_rsa does not now.
>> >>
>> >> I change to:
>> >>
>> >> Host bitucket . org-dd
>> >> HostName bitbucket . org
>> >> IdentityFile ~/.ssh/id_rsa1
>> >>
>> >> Host bitbucket . org-sas
>> >> HostName bitbucket . org
>> >> IdentityFile ~/.ssh/id_rsa
>> >>
>> >> And now only id_rsa works.
>> >>
>> >> I also tried combining the two IdentityFile lines together like so
>> >> (for some reason):
>> >>
>> >> Host bitucket . org
>> >> IdentityFile ~/.ssh/id_rsa1
>> >> IdentityFile ~/.ssh/id_rsa
>> >>
>> >> I have even tried running ssh-agent . exe, adding id_rsa1 to that and
>> >> then running the git clone with no result.
>> >>
>> >> The weird thing is, I have two public keys as well and they both load
>> >> in the ssh . exe (they return errors about format), I just cannot get
>> >> my ssh . exe to work with multiple private keys.
>> >
>> > This might just be a special case of the problem of some hosting
>> > providers picking only the first key you provide, as described in this
>> > thread:
>> > https://public-inbox.org/git/20180103102840.27897-1-ava...@gmail.com/
>> >
>> > If so, you either need to hack around this with ssh host aliases, or a
>> > custom GIT_SSH_COMMAND.
>> >
>> >> On 10 January 2018 at 15:29, Sam Millman 
>> wrote:
>> >>> I am trying, for the sake of PhpStorm, to get multiple SSH keys
>> >>> working using git . exe, which means no GitBash.
>> >>>
>> >>> I can get the keys to work just fine with GitBash.
>> >>>
>> >>> I edited my .ssh/config to look like (I know this is incorrect):
>> >>>
>> >>> Host bitucket . org
>> >>> IdentityFile ~/.ssh/id_rsa1
>> >>>
>> >>> Host bitbucket . org
>> >>> IdentityFile ~/.ssh/id_rsa
>> >>>
>> >>>
>> >>> And id_rsa1 works, I can actually pick from the other repo. But, of
>> >>> course, id_rsa does not now.
>> >>>
>> >>> I change to:
>> >>>
>> >>> Host bitucket . org-dd
>> >>> HostName bitbucket . org
>> >>> IdentityFile ~/.ssh/id_rsa1
>> >>>
>> >>> Host bitbucket . org-sas
>> >>> HostName bitbucket . org
>> >>> IdentityFile ~/.ssh/id_rsa
>> >>>
>> >>> And now only id_rsa works.
>> >>>
>> >>> I also tried combining the two IdentityFile lines together like so
>> >>> (for some
>> >>> reason):
>> >>>
>> >>> Host bitucket . org
>> >>> IdentityFile ~/.ssh/id_rsa1
>> >>> IdentityFile ~/.ssh/id_rsa
>> >>>
>> >>> I have even tried running ssh-agent . exe, adding id_rsa1 to that
>> >>> and then running the git clone with no result.
>> >>>
>> >>> The weird thing is, I have two public keys as well and they both
>> >>> load in the ssh . exe (they return errors about format), I just
>> >>> cannot get my ssh . exe to work with multiple private keys.
>> >>>
>> >>> Has anyone got any ideas on how to solve this?
>

Re: What's cooking in git.git (Jan 2018, #02; Tue, 9)

[Jeff Hostetler]

On 1/9/2018 6:33 PM, Junio C Hamano wrote:

--
[Cooking]


* jh/fsck-promisors (2017-12-08) 10 commits

[...]


* jh/partial-clone (2017-12-08) 13 commits

[...]

Parts 2 and 3 of partial clone have been simmering
for a while now.  I was wondering if there were any
more comments or questions on them.  I don't recall
any existing issues.

Thanks
Jeff

RE: How to use multiple SSH keys on Git exe (not bash)

[Randall S. Becker]

May I, with respect, ask you to take this to the OpenSSH email list? 
(openssh-unix-...@mindrot.org)  I think the discussion better belongs there and 
you're likely to get more detailed information from that team.
Sincerely,
Randall

> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
> Behalf Of Sam Millman
> Sent: January 10, 2018 11:03 AM
> To: Ævar Arnfjörð Bjarmason 
> Cc: git@vger.kernel.org
> Subject: Re: How to use multiple SSH keys on Git exe (not bash)
> 
> I actually played a bit more and got this:
> 
> Host *
> IdentityFile ~/.ssh/id_rsa_d
> IdentityFile ~/.ssh/id_rsa
> 
> Host bitbucket_1
> User git
> HostName bitbucket.org
> IdentityFile ~/.ssh/id_rsa_d
> 
> Host bitbucket_2
> User git
> HostName bitbucket.org
> IdentityFile ~/.ssh/id_rsa
> 
> And from basic testing it seems to actually work, it seems it is the Host * 
> that
> make sit work and it will actually iterate the keys and try them.
> 
> Not sure why this is, any thoughts?
> 
> On 10 January 2018 at 15:58, Ævar Arnfjörð Bjarmason 
> wrote:
> >
> > On Wed, Jan 10 2018, Sam Millman jotted:
> >
> >> I am trying, for the sake of PhpStorm, to get multiple SSH keys
> >> working using git . exe, which means no GitBash.
> >>
> >> I can get the keys to work just fine with GitBash.
> >>
> >> I edited my .ssh/config to look like (I know this is incorrect):
> >>
> >> Host bitucket . org
> >> IdentityFile ~/.ssh/id_rsa1
> >>
> >> Host bitbucket . org
> >> IdentityFile ~/.ssh/id_rsa
> >>
> >>
> >> And id_rsa1 works, I can actually pick from the other repo. But, of
> >> course, id_rsa does not now.
> >>
> >> I change to:
> >>
> >> Host bitucket . org-dd
> >> HostName bitbucket . org
> >> IdentityFile ~/.ssh/id_rsa1
> >>
> >> Host bitbucket . org-sas
> >> HostName bitbucket . org
> >> IdentityFile ~/.ssh/id_rsa
> >>
> >> And now only id_rsa works.
> >>
> >> I also tried combining the two IdentityFile lines together like so
> >> (for some reason):
> >>
> >> Host bitucket . org
> >> IdentityFile ~/.ssh/id_rsa1
> >> IdentityFile ~/.ssh/id_rsa
> >>
> >> I have even tried running ssh-agent . exe, adding id_rsa1 to that and
> >> then running the git clone with no result.
> >>
> >> The weird thing is, I have two public keys as well and they both load
> >> in the ssh . exe (they return errors about format), I just cannot get
> >> my ssh . exe to work with multiple private keys.
> >
> > This might just be a special case of the problem of some hosting
> > providers picking only the first key you provide, as described in this
> > thread:
> > https://public-inbox.org/git/20180103102840.27897-1-ava...@gmail.com/
> >
> > If so, you either need to hack around this with ssh host aliases, or a
> > custom GIT_SSH_COMMAND.
> >
> >> On 10 January 2018 at 15:29, Sam Millman 
> wrote:
> >>> I am trying, for the sake of PhpStorm, to get multiple SSH keys
> >>> working using git . exe, which means no GitBash.
> >>>
> >>> I can get the keys to work just fine with GitBash.
> >>>
> >>> I edited my .ssh/config to look like (I know this is incorrect):
> >>>
> >>> Host bitucket . org
> >>> IdentityFile ~/.ssh/id_rsa1
> >>>
> >>> Host bitbucket . org
> >>> IdentityFile ~/.ssh/id_rsa
> >>>
> >>>
> >>> And id_rsa1 works, I can actually pick from the other repo. But, of
> >>> course, id_rsa does not now.
> >>>
> >>> I change to:
> >>>
> >>> Host bitucket . org-dd
> >>> HostName bitbucket . org
> >>> IdentityFile ~/.ssh/id_rsa1
> >>>
> >>> Host bitbucket . org-sas
> >>> HostName bitbucket . org
> >>> IdentityFile ~/.ssh/id_rsa
> >>>
> >>> And now only id_rsa works.
> >>>
> >>> I also tried combining the two IdentityFile lines together like so
> >>> (for some
> >>> reason):
> >>>
> >>> Host bitucket . org
> >>> IdentityFile ~/.ssh/id_rsa1
> >>> IdentityFile ~/.ssh/id_rsa
> >>>
> >>> I have even tried running ssh-agent . exe, adding id_rsa1 to that
> >>> and then running the git clone with no result.
> >>>
> >>> The weird thing is, I have two public keys as well and they both
> >>> load in the ssh . exe (they return errors about format), I just
> >>> cannot get my ssh . exe to work with multiple private keys.
> >>>
> >>> Has anyone got any ideas on how to solve this?

RE: How to use multiple SSH keys on Git exe (not bash)

[Randall S. Becker]

On January 10, 2018 11:01 AM Sam Millman wrote:
> That would mean I would need to change the case for a letter everytime I
> have a repo with a new key, that would mean I would be restricted to
> 12 client repos at a time :\, seems very hacky to me
> 
> On 10 January 2018 at 15:58, Randall S. Becker 
> wrote:
> > The ~/.ssh/config file is case sensitive by definition when it comes to Host
> and HostName. Try bitbucket.org for one and Bitbucket.org for another. You
> will have to change the remote URL accordingly to pick up the correct
> identity.

The combinatorics of it is Sum i=1 to n.(i) = 78.

You can also use BUtbucket.org, bUTbucket.org, etc., BUTbucket.org, etc., etc.

Hacky, maybe, but it has worked that way for a long time. Why do you need more 
than 78 identities? Bitbucket allows you to be on multiple projects with the 
same identity.

Re: How to use multiple SSH keys on Git exe (not bash)

[Sam Millman]

I actually played a bit more and got this:

Host *
IdentityFile ~/.ssh/id_rsa_d
IdentityFile ~/.ssh/id_rsa

Host bitbucket_1
User git
HostName bitbucket.org
IdentityFile ~/.ssh/id_rsa_d

Host bitbucket_2
User git
HostName bitbucket.org
IdentityFile ~/.ssh/id_rsa

And from basic testing it seems to actually work, it seems it is the
Host * that make sit work and it will actually iterate the keys and
try them.

Not sure why this is, any thoughts?

On 10 January 2018 at 15:58, Ævar Arnfjörð Bjarmason  wrote:
>
> On Wed, Jan 10 2018, Sam Millman jotted:
>
>> I am trying, for the sake of PhpStorm, to get multiple SSH keys
>> working using git . exe, which means no GitBash.
>>
>> I can get the keys to work just fine with GitBash.
>>
>> I edited my .ssh/config to look like (I know this is incorrect):
>>
>> Host bitucket . org
>> IdentityFile ~/.ssh/id_rsa1
>>
>> Host bitbucket . org
>> IdentityFile ~/.ssh/id_rsa
>>
>>
>> And id_rsa1 works, I can actually pick from the other repo. But, of
>> course, id_rsa does not now.
>>
>> I change to:
>>
>> Host bitucket . org-dd
>> HostName bitbucket . org
>> IdentityFile ~/.ssh/id_rsa1
>>
>> Host bitbucket . org-sas
>> HostName bitbucket . org
>> IdentityFile ~/.ssh/id_rsa
>>
>> And now only id_rsa works.
>>
>> I also tried combining the two IdentityFile lines together like so
>> (for some reason):
>>
>> Host bitucket . org
>> IdentityFile ~/.ssh/id_rsa1
>> IdentityFile ~/.ssh/id_rsa
>>
>> I have even tried running ssh-agent . exe, adding id_rsa1 to that and
>> then running the git clone with no result.
>>
>> The weird thing is, I have two public keys as well and they both load
>> in the ssh . exe (they return errors about format), I just cannot get
>> my ssh . exe to work with multiple private keys.
>
> This might just be a special case of the problem of some hosting
> providers picking only the first key you provide, as described in this
> thread:
> https://public-inbox.org/git/20180103102840.27897-1-ava...@gmail.com/
>
> If so, you either need to hack around this with ssh host aliases, or a
> custom GIT_SSH_COMMAND.
>
>> On 10 January 2018 at 15:29, Sam Millman  wrote:
>>> I am trying, for the sake of PhpStorm, to get multiple SSH keys working
>>> using git . exe, which means no GitBash.
>>>
>>> I can get the keys to work just fine with GitBash.
>>>
>>> I edited my .ssh/config to look like (I know this is incorrect):
>>>
>>> Host bitucket . org
>>> IdentityFile ~/.ssh/id_rsa1
>>>
>>> Host bitbucket . org
>>> IdentityFile ~/.ssh/id_rsa
>>>
>>>
>>> And id_rsa1 works, I can actually pick from the other repo. But, of course,
>>> id_rsa does not now.
>>>
>>> I change to:
>>>
>>> Host bitucket . org-dd
>>> HostName bitbucket . org
>>> IdentityFile ~/.ssh/id_rsa1
>>>
>>> Host bitbucket . org-sas
>>> HostName bitbucket . org
>>> IdentityFile ~/.ssh/id_rsa
>>>
>>> And now only id_rsa works.
>>>
>>> I also tried combining the two IdentityFile lines together like so (for some
>>> reason):
>>>
>>> Host bitucket . org
>>> IdentityFile ~/.ssh/id_rsa1
>>> IdentityFile ~/.ssh/id_rsa
>>>
>>> I have even tried running ssh-agent . exe, adding id_rsa1 to that and then
>>> running the git clone with no result.
>>>
>>> The weird thing is, I have two public keys as well and they both load in the
>>> ssh . exe (they return errors about format), I just cannot get my ssh . exe
>>> to work with multiple private keys.
>>>
>>> Has anyone got any ideas on how to solve this?

Re: How to use multiple SSH keys on Git exe (not bash)

[Sam Millman]

That would mean I would need to change the case for a letter everytime
I have a repo with a new key, that would mean I would be restricted to
12 client repos at a time :\, seems very hacky to me

On 10 January 2018 at 15:58, Randall S. Becker  wrote:
> On January 10, 2018 10:31 AM Sam Millman wrote:
>> I am trying, for the sake of PhpStorm, to get multiple SSH keys working using
>> git . exe, which means no GitBash.
>>
>> I can get the keys to work just fine with GitBash.
>>
>> I edited my .ssh/config to look like (I know this is incorrect):
>>
>> Host bitucket . org
>> IdentityFile ~/.ssh/id_rsa1
>>
>> Host bitbucket . org
>> IdentityFile ~/.ssh/id_rsa
>>
>>
>> And id_rsa1 works, I can actually pick from the other repo. But, of course,
>> id_rsa does not now.
>>
>> I change to:
>>
>> Host bitucket . org-dd
>> HostName bitbucket . org
>> IdentityFile ~/.ssh/id_rsa1
>>
>> Host bitbucket . org-sas
>> HostName bitbucket . org
>> IdentityFile ~/.ssh/id_rsa
>>
>> And now only id_rsa works.
>>
>> I also tried combining the two IdentityFile lines together like so (for some
>> reason):
>>
>> Host bitucket . org
>> IdentityFile ~/.ssh/id_rsa1
>> IdentityFile ~/.ssh/id_rsa
>>
>> I have even tried running ssh-agent . exe, adding id_rsa1 to that and then
>> running the git clone with no result.
>>
>> The weird thing is, I have two public keys as well and they both load in the
>> ssh . exe (they return errors about format), I just cannot get my ssh . exe 
>> to
>> work with multiple private keys.
>>
>> On 10 January 2018 at 15:29, Sam Millman  wrote:
>> > I am trying, for the sake of PhpStorm, to get multiple SSH keys
>> > working using git . exe, which means no GitBash.
>> >
>> > I can get the keys to work just fine with GitBash.
>> >
>> > I edited my .ssh/config to look like (I know this is incorrect):
>> >
>> > Host bitucket . org
>> > IdentityFile ~/.ssh/id_rsa1
>> >
>> > Host bitbucket . org
>> > IdentityFile ~/.ssh/id_rsa
>> >
>> >
>> > And id_rsa1 works, I can actually pick from the other repo. But, of
>> > course, id_rsa does not now.
>> >
>> > I change to:
>> >
>> > Host bitucket . org-dd
>> > HostName bitbucket . org
>> > IdentityFile ~/.ssh/id_rsa1
>> >
>> > Host bitbucket . org-sas
>> > HostName bitbucket . org
>> > IdentityFile ~/.ssh/id_rsa
>> >
>> > And now only id_rsa works.
>> >
>> > I also tried combining the two IdentityFile lines together like so
>> > (for some
>> > reason):
>> >
>> > Host bitucket . org
>> > IdentityFile ~/.ssh/id_rsa1
>> > IdentityFile ~/.ssh/id_rsa
>> >
>> > I have even tried running ssh-agent . exe, adding id_rsa1 to that and
>> > then running the git clone with no result.
>> >
>> > The weird thing is, I have two public keys as well and they both load
>> > in the ssh . exe (they return errors about format), I just cannot get
>> > my ssh . exe to work with multiple private keys.
>> >
>> > Has anyone got any ideas on how to solve this?
>
> The ~/.ssh/config file is case sensitive by definition when it comes to Host 
> and HostName. Try bitbucket.org for one and Bitbucket.org for another. You 
> will have to change the remote URL accordingly to pick up the correct 
> identity.
>
> Good luck,
> Randall
>
>

Re: How to use multiple SSH keys on Git exe (not bash)

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 10 2018, Sam Millman jotted:

> I am trying, for the sake of PhpStorm, to get multiple SSH keys
> working using git . exe, which means no GitBash.
>
> I can get the keys to work just fine with GitBash.
>
> I edited my .ssh/config to look like (I know this is incorrect):
>
> Host bitucket . org
> IdentityFile ~/.ssh/id_rsa1
>
> Host bitbucket . org
> IdentityFile ~/.ssh/id_rsa
>
>
> And id_rsa1 works, I can actually pick from the other repo. But, of
> course, id_rsa does not now.
>
> I change to:
>
> Host bitucket . org-dd
> HostName bitbucket . org
> IdentityFile ~/.ssh/id_rsa1
>
> Host bitbucket . org-sas
> HostName bitbucket . org
> IdentityFile ~/.ssh/id_rsa
>
> And now only id_rsa works.
>
> I also tried combining the two IdentityFile lines together like so
> (for some reason):
>
> Host bitucket . org
> IdentityFile ~/.ssh/id_rsa1
> IdentityFile ~/.ssh/id_rsa
>
> I have even tried running ssh-agent . exe, adding id_rsa1 to that and
> then running the git clone with no result.
>
> The weird thing is, I have two public keys as well and they both load
> in the ssh . exe (they return errors about format), I just cannot get
> my ssh . exe to work with multiple private keys.

This might just be a special case of the problem of some hosting
providers picking only the first key you provide, as described in this
thread:
https://public-inbox.org/git/20180103102840.27897-1-ava...@gmail.com/

If so, you either need to hack around this with ssh host aliases, or a
custom GIT_SSH_COMMAND.

> On 10 January 2018 at 15:29, Sam Millman  wrote:
>> I am trying, for the sake of PhpStorm, to get multiple SSH keys working
>> using git . exe, which means no GitBash.
>>
>> I can get the keys to work just fine with GitBash.
>>
>> I edited my .ssh/config to look like (I know this is incorrect):
>>
>> Host bitucket . org
>> IdentityFile ~/.ssh/id_rsa1
>>
>> Host bitbucket . org
>> IdentityFile ~/.ssh/id_rsa
>>
>>
>> And id_rsa1 works, I can actually pick from the other repo. But, of course,
>> id_rsa does not now.
>>
>> I change to:
>>
>> Host bitucket . org-dd
>> HostName bitbucket . org
>> IdentityFile ~/.ssh/id_rsa1
>>
>> Host bitbucket . org-sas
>> HostName bitbucket . org
>> IdentityFile ~/.ssh/id_rsa
>>
>> And now only id_rsa works.
>>
>> I also tried combining the two IdentityFile lines together like so (for some
>> reason):
>>
>> Host bitucket . org
>> IdentityFile ~/.ssh/id_rsa1
>> IdentityFile ~/.ssh/id_rsa
>>
>> I have even tried running ssh-agent . exe, adding id_rsa1 to that and then
>> running the git clone with no result.
>>
>> The weird thing is, I have two public keys as well and they both load in the
>> ssh . exe (they return errors about format), I just cannot get my ssh . exe
>> to work with multiple private keys.
>>
>> Has anyone got any ideas on how to solve this?

RE: How to use multiple SSH keys on Git exe (not bash)

[Randall S. Becker]

On January 10, 2018 10:31 AM Sam Millman wrote:
> I am trying, for the sake of PhpStorm, to get multiple SSH keys working using
> git . exe, which means no GitBash.
> 
> I can get the keys to work just fine with GitBash.
> 
> I edited my .ssh/config to look like (I know this is incorrect):
> 
> Host bitucket . org
> IdentityFile ~/.ssh/id_rsa1
> 
> Host bitbucket . org
> IdentityFile ~/.ssh/id_rsa
> 
> 
> And id_rsa1 works, I can actually pick from the other repo. But, of course,
> id_rsa does not now.
> 
> I change to:
> 
> Host bitucket . org-dd
> HostName bitbucket . org
> IdentityFile ~/.ssh/id_rsa1
> 
> Host bitbucket . org-sas
> HostName bitbucket . org
> IdentityFile ~/.ssh/id_rsa
> 
> And now only id_rsa works.
> 
> I also tried combining the two IdentityFile lines together like so (for some
> reason):
> 
> Host bitucket . org
> IdentityFile ~/.ssh/id_rsa1
> IdentityFile ~/.ssh/id_rsa
> 
> I have even tried running ssh-agent . exe, adding id_rsa1 to that and then
> running the git clone with no result.
> 
> The weird thing is, I have two public keys as well and they both load in the
> ssh . exe (they return errors about format), I just cannot get my ssh . exe to
> work with multiple private keys.
> 
> On 10 January 2018 at 15:29, Sam Millman  wrote:
> > I am trying, for the sake of PhpStorm, to get multiple SSH keys
> > working using git . exe, which means no GitBash.
> >
> > I can get the keys to work just fine with GitBash.
> >
> > I edited my .ssh/config to look like (I know this is incorrect):
> >
> > Host bitucket . org
> > IdentityFile ~/.ssh/id_rsa1
> >
> > Host bitbucket . org
> > IdentityFile ~/.ssh/id_rsa
> >
> >
> > And id_rsa1 works, I can actually pick from the other repo. But, of
> > course, id_rsa does not now.
> >
> > I change to:
> >
> > Host bitucket . org-dd
> > HostName bitbucket . org
> > IdentityFile ~/.ssh/id_rsa1
> >
> > Host bitbucket . org-sas
> > HostName bitbucket . org
> > IdentityFile ~/.ssh/id_rsa
> >
> > And now only id_rsa works.
> >
> > I also tried combining the two IdentityFile lines together like so
> > (for some
> > reason):
> >
> > Host bitucket . org
> > IdentityFile ~/.ssh/id_rsa1
> > IdentityFile ~/.ssh/id_rsa
> >
> > I have even tried running ssh-agent . exe, adding id_rsa1 to that and
> > then running the git clone with no result.
> >
> > The weird thing is, I have two public keys as well and they both load
> > in the ssh . exe (they return errors about format), I just cannot get
> > my ssh . exe to work with multiple private keys.
> >
> > Has anyone got any ideas on how to solve this?

The ~/.ssh/config file is case sensitive by definition when it comes to Host 
and HostName. Try bitbucket.org for one and Bitbucket.org for another. You will 
have to change the remote URL accordingly to pick up the correct identity.

Good luck,
Randall

Re: How to use multiple SSH keys on Git exe (not bash)

[Sam Millman]

I am trying, for the sake of PhpStorm, to get multiple SSH keys
working using git . exe, which means no GitBash.

I can get the keys to work just fine with GitBash.

I edited my .ssh/config to look like (I know this is incorrect):

Host bitucket . org
IdentityFile ~/.ssh/id_rsa1

Host bitbucket . org
IdentityFile ~/.ssh/id_rsa


And id_rsa1 works, I can actually pick from the other repo. But, of
course, id_rsa does not now.

I change to:

Host bitucket . org-dd
HostName bitbucket . org
IdentityFile ~/.ssh/id_rsa1

Host bitbucket . org-sas
HostName bitbucket . org
IdentityFile ~/.ssh/id_rsa

And now only id_rsa works.

I also tried combining the two IdentityFile lines together like so
(for some reason):

Host bitucket . org
IdentityFile ~/.ssh/id_rsa1
IdentityFile ~/.ssh/id_rsa

I have even tried running ssh-agent . exe, adding id_rsa1 to that and
then running the git clone with no result.

The weird thing is, I have two public keys as well and they both load
in the ssh . exe (they return errors about format), I just cannot get
my ssh . exe to work with multiple private keys.

On 10 January 2018 at 15:29, Sam Millman  wrote:
> I am trying, for the sake of PhpStorm, to get multiple SSH keys working
> using git . exe, which means no GitBash.
>
> I can get the keys to work just fine with GitBash.
>
> I edited my .ssh/config to look like (I know this is incorrect):
>
> Host bitucket . org
> IdentityFile ~/.ssh/id_rsa1
>
> Host bitbucket . org
> IdentityFile ~/.ssh/id_rsa
>
>
> And id_rsa1 works, I can actually pick from the other repo. But, of course,
> id_rsa does not now.
>
> I change to:
>
> Host bitucket . org-dd
> HostName bitbucket . org
> IdentityFile ~/.ssh/id_rsa1
>
> Host bitbucket . org-sas
> HostName bitbucket . org
> IdentityFile ~/.ssh/id_rsa
>
> And now only id_rsa works.
>
> I also tried combining the two IdentityFile lines together like so (for some
> reason):
>
> Host bitucket . org
> IdentityFile ~/.ssh/id_rsa1
> IdentityFile ~/.ssh/id_rsa
>
> I have even tried running ssh-agent . exe, adding id_rsa1 to that and then
> running the git clone with no result.
>
> The weird thing is, I have two public keys as well and they both load in the
> ssh . exe (they return errors about format), I just cannot get my ssh . exe
> to work with multiple private keys.
>
> Has anyone got any ideas on how to solve this?

Re: [PATCH 0/4] "Fast-"track Git GUI changes

[Johannes Schindelin]

Hi,

On Tue, 9 Jan 2018, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Johannes Schindelin  writes:
> >
> >> As it seems to be impossible to get the attention of the Git GUI
> >> maintainer these "days" (I have opened Pull Requests on October 16th
> >> 2016 that have not even been looked at), let's just side-step that
> >> contribution path which seems to be dormant.
> >
> > Good to see that finally somebody else steps up after I did the same
> > for a few times recently.
> >
> >> These fixes have been in Git for Windows for various amounts of time.
> >>
> >> Note: there are more Git GUI changes in Git for Windows, I only
> >> accumulated the ones I deem wort considering for inclusion into v2.16.0,
> >> still.
> >
> > Thanks.  I am not sure if it is too late for 2.16, as these are not
> > fixes for regression during this cycle, though.
> 
> Heh, I changed my mind.  
> 
> Just like I pretended to be interim maintainer of Git-GUI for the
> past two times, you are doing the same this time and I even agreed
> that it was a good thing that you volunteered to pretend as one.
> 
> So let's follow through the pretence to its conclusion and merge
> these directly to 'master'.

Thank you, fellow pretender.

Ciao,
Dscho

Re: merge-base --is-ancestor A B is unreasonably slow with unrelated history B

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 09 2018, Derrick Stolee jotted:

> On 1/9/2018 10:17 AM, Ævar Arnfjörð Bjarmason wrote:
>> This is a pathological case I don't have time to dig into right now:
>>
>>  git branch -D orphan;
>>  git checkout --orphan orphan &&
>>  git reset --hard &&
>>  touch foo &&
>>  git add foo &&
>>  git commit -m"foo" &&
>>  time git merge-base --is-ancestor master orphan
>>
>> This takes around 5 seconds on linux.git to return 1. Which is around
>> the same time it takes to run current master against the first commit in
>> linux.git:
>>
>>  git merge-base --is-ancestor 1da177e4c3f4 master
>>
>> This is obviously a pathological case, but maybe we should work slightly
>> harder on the RHS of and discover that it itself is an orphan commit.
>>
>> I ran into this while writing a hook where we'd like to do:
>>
>>  git diff $master...topic
>>
>> Or not, depending on if the topic is an orphan or just something
>> recently branched off, figured I could use --is-ancestor as on
>> optimization, and then discovered it's not much of an optimization.
>
> Ævar,
>
> This is the same performance problem that we are trying to work around
> with Jeff's "Add --no-ahead-behind to status" patch [1]. For commits
> that are far apart, many commits need to be parsed. I think the right
> solution is to create a serialized commit graph that stores the
> adjacency information of the commits and can create commit structs
> quickly. This requires storing the commit id, commit date, parents,
> and root tree id to satisfy the needs of parse_commit_gently(). Once
> the framework for this data is constructed, it is simple to add
> generation numbers to that data and start consuming them in other
> algorithms (by adding the field to 'struct commit').
>
> I'm working on such a patch right now, but it will be a few weeks
> before I'm ready.

Thanks, yes of course I forgot about not being able to rely on
timestamps at all, otherwise this check wouldn't need commit traversal
at all, we could simply note:

 1. A is older than B
 2. B is an orphan commit
 3. Therefore it's impossible that A is an ancestor of B

But we don't enforce any of this in the DAG, so now we need to do a full
traversal to make sure this B committed in 2018 doesn't precede some
commit added in 2005.

This has come up before related to various traversal optimizations,
e.g. [tag|branch] --contains.

I don't know the details of what you have in mind, but have you
considered a solution which involves either computing that all commits
in the repo have a timestamp later than their parents, or alternatively
declaring via some config mechanism that that's true of all (or a subset
of) commits?

E.g. for centrally hosted repos a pre-receive hook could be added to
assert that all incoming commits must have monotonically increasing
timestamps compared to preceding commits, and if that wasn't the case we
could declare that e.g. all commits added after 2019 would have that
property (after ensuring no commits had dates in the future).

It sounds like a subset of what you have in mind, so maybe it's
useless. However for many cases such as this one it should be faster to
almost instantaneously consult such config instead of some side-data we
need to maintain, but it's probably not worth the complexity.


> [1] v5 of --no-ahead-behind
> https://public-inbox.org/git/20180109185018.69164-1-...@jeffhostetler.com/T/#t
>
> [2] v4 of --no-ahead-behind
> https://public-inbox.org/git/nycvar.qro.7.76.6.1801091744540...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/T/#t

Re: How to view diff when doing `git push --force`

[KES]

> At this point, no, but in the future instead of --force use
> --force-with-lease=: where  is e.g. $(git
> rev-parse HEAD).

Thank you.

But I have found next command to see what were changed. 
For example if someone did forced push and I wanna see changes before merge 
remote

git log --graph --decorate --pretty=oneline --abbrev-commit --cherry-mark 
--boundary --left-right

But is there an option to view changes in amended commits?

See the last code post about unknown option  in the next answer :
https://stackoverflow.com/a/48149931/4632019


If there is no such option. Will it be useful to have it?

information required

[From Amir Khadov]

-- 
Thanks for your last email response to me.
The information required should include the following-:
Your full names
Your address
Telephone number
Your private email
Occupation
Age
This is to enable my further discussion with you in confidence.
Best regards and wishes to you.
Mohammad Amir Khadov
NB: Please reply to:

am...@postaxte.com

am...@pobox.sk

[PATCH 0/2] the cat-file -e option doesn't work as documented

[Ævar Arnfjörð Bjarmason]

The -e option to cat-file will emit output, after promising not to.

We should take either 1/2 or 2/2, but not both. I'm partial to just
documenting the existing behavior and dropping 2/2, it's useful to
know if you passed in something that didn't look like a SHA-1.

But if others disagree we can drop 1/2 and take 2/2. Up to you.

Ævar Arnfjörð Bjarmason (2):
  cat-file doc: document that -e will return some output
  cat-file: -e should not emit output on stderr

 Documentation/git-cat-file.txt | 7 ---
 builtin/cat-file.c | 8 ++--
 t/t1006-cat-file.sh| 7 +++
 3 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.15.1.424.g9478a66081

[PATCH 2/2] cat-file: -e should not emit output on stderr

[Ævar Arnfjörð Bjarmason]

Change "cat-file -e some-garbage" to work as documented. Before it
would emit:

$ git cat-file -e some-garbage; echo $?
fatal: Not a valid object name some-garbage
128

Now:

$ ./git-cat-file -e some-garbage; echo $?
1

This is a change to longstanding behavior established in
7950571ad7 ("A few more options for git-cat-file", 2005-12-03) when
the option was initially added, but we should go with the promise made
in the documentation.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/cat-file.c  | 8 ++--
 t/t1006-cat-file.sh | 7 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75a..75991788af 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -65,8 +65,12 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
 
if (get_oid_with_context(obj_name, GET_OID_RECORD_PATH,
-, _context))
-   die("Not a valid object name %s", obj_name);
+, _context)) {
+   if (opt == 'e')
+   return 1;
+   else
+   die("Not hello a valid object name %s", obj_name);
+   }
 
if (!path)
path = obj_context.path;
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index b19f332694..c05a899bc4 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -201,6 +201,13 @@ do
 '
 done
 
+test_expect_success 'Providing -e should suppress all error output' '
+   ! git cat-file -e some-garbage >stdout 2>stderr &&
+   >expect &&
+   test_cmp expect stdout &&
+   test_cmp expect stderr
+'
+
 for opt in t s e p
 do
 test_expect_success "Passing -$opt with --follow-symlinks fails" '
-- 
2.15.1.424.g9478a66081

[PATCH 1/2] cat-file doc: document that -e will return some output

[Ævar Arnfjörð Bjarmason]

The -e option added in 7950571ad7 ("A few more options for
git-cat-file", 2005-12-03) has always errored out with message on
stderr saying that the provided object is malformed, currently:

$ git cat-file -e malformed; echo $?
fatal: Not a valid object name malformed
128

A careful reader of this documentation would be mislead into thinking
the could write:

if ! git cat-file -e "$object" [...]

As opposed to:

if ! git cat-file -e "$object" 2>/dev/null [...]

To check whether some arbitrary $object string was both valid, and
pointed to an object that exists.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-cat-file.txt | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index fb09cd69d6..f90f09b03f 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -42,8 +42,9 @@ OPTIONS
.
 
 -e::
-   Suppress all output; instead exit with zero status if 
-   exists and is a valid object.
+   Exit with zero status if  exists and is a valid
+   object. If  is of an invalid format exit with non-zero and
+   emits an error on stderr.
 
 -p::
Pretty-print the contents of  based on its type.
@@ -168,7 +169,7 @@ If `-t` is specified, one of the .
 
 If `-s` is specified, the size of the  in bytes.
 
-If `-e` is specified, no output.
+If `-e` is specified, no output, unless the  is malformed.
 
 If `-p` is specified, the contents of  are pretty-printed.
 
-- 
2.15.1.424.g9478a66081

[PATCH/RFC] add--interactive: ignore all internal submodule changes

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

For 'add -i' and 'add -p' the only action we can take on a dirty
submodule entry (from the superproject perspective) is its SHA-1. The
content changes inside do not matter, at least until interactive add has
--recurse-submodules or something.

Ignore all dirty changes to reduce the questions 'add -i' and 'add -p'
throw at the user when submodules are dirty.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 $DAYJOB started to use submodules and this annoys me so much when I
 use 'git add -p'. I'm neither very familiar with add--interactive nor
 submodules code but this seems to work. Hopefully it's a correct
 change.

 git-add--interactive.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 28b325d754..964c3a7542 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -262,7 +262,7 @@ sub list_modified {
}
}
 
-   for (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), 
@ARGV)) {
+   for (run_cmd_pipe(qw(git diff-files --ignore-submodules=dirty --numstat 
--summary --raw --), @ARGV)) {
if (($add, $del, $file) =
/^([-\d]+)  ([-\d]+)(.*)/) {
$file = unquote_path($file);
-- 
2.15.1.600.g899a5f85c6

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Jeff King]

On Tue, Jan 09, 2018 at 02:05:09PM +0100, Johannes Schindelin wrote:

> > I think that could be easily worked around for rebase by asking git to
> > check ambiguity during the conversion.
> 
> Sure.
> 
> It also points to a flaw in your reasoning, and you should take my example
> further: previously, we guaranteed unique abbreviations, and who is to say
> that there is no script out there relying on that guarantee? You?

I'm not sure what you think my reasoning was, since I made the same
point directly after. ;)

> > I am a bit curious if there's a bounded probability that people would
> > find acceptable for Git to give an ambiguous abbreviation. We already
> > accept 1 in 2^160, of course. But would, e.g., one in a million be OK?
> 
> What is that going to solve?

I'm not sure I understand your question. We can bound the probability of
a collision by increasing the size of the abbreviation. My question was
whether there is a size where that tradeoff makes sense. So it "solves"
the problem of worrying about collisions.

> I think a better alternative would be to introduce a new abbreviation mode
> that is *intended* to stop caring about unique abbreviations.
> 
> In web interfaces, for example, it makes tons of sense to show, say, 8
> digits in link texts and have the full name in the actual link URL.

I'm not sure if that would be useful option or not. I certainly agree
that static abbreviations are a useful thing to want. But for scripted
callers, it's usually easy to cut down the abbreviation yourself. I
think your web interface example is a good one. The caller will ask Git
for the full 40-hex hash, and then cut it down itself when generating
the link (and I just so happen to have worked on a web interface that
does this[1]).

So would it be useful for humans to use? That's what I'm not sure about.
It seems cumbersome to have to add "--abbrev=8!" on the command line to
each invocation. But if it were triggered by config, it seems like we
would risk breaking scripts.

> I am just hesitant to change things that would break existing setups.

Me too. I'm not sure if I'm seriously advocating the "bound the
probability" thing. It's just something I think is worth pondering a
little.

-Peff

[1] Actually, there _is_ something that it's useful for Git to tell the
caller, which is the approximate number of objects in the
repository. Eight digits is not enough for the kernel, for example,
and these days we use 12 digits by default. I'm not sure if such a
web interface would rather ask for "%H %h" and generate the link
text from the second half, or it would rather just get a ballpark on
the number of objects, and do the simple abbreviation math itself
(right now GitHub does not do either; I don't know about other
interfaces).

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

[Duy Nguyen]

On Wed, Jan 10, 2018 at 5:38 PM, Adam Dinwoodie  wrote:
>> One disadvantage of this though, if this kind of framework does not
>> get popular, then any new test feature must be added at both places
>> but it's a waste of time to support both. So...
>
> I don't follow: if we end up implementing every test twice, as we have
> here, then I agree, but I don't think there's much value in doing that
> except as a proof of concept, as in this immediate discussion.  The
> obvious-to-me way to do this would be following the precedent of the
> core code: gradually migrate things away from shell code to C code.

Not the tests themselves. Test features, like --valgrind, --debug,
--verbose and that kind of stuff. These are handled by test-lib.sh. If
we add support for --new-fancy-thing to test-lib.sh then we need some
more code in test-lib.c as well.
-- 
Duy

[PATCH] run-command.c: print env vars when GIT_TRACE is set

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

Occasionally submodule code could execute new commands with GIT_DIR set
to some submodule. GIT_TRACE prints just the command line which makes it
hard to tell that it's not really executed on this repository.

Print env variables in this case. Note that the code deliberately ignore
variables unsetting because there are so many of them (to keep git
environment clean for the next process) and really hard to read.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 A minor thing that I ignored in this patch is quoting the environment
 variables. For me it's meh. Perhaps I should just dumb the env
 without quoting at all.

 run-command.c |  3 ++-
 trace.c   | 38 +++---
 trace.h   | 18 +++---
 3 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/run-command.c b/run-command.c
index 31fc5ea86e..235367087d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)
cmd->err = fderr[0];
}
 
-   trace_argv_printf(cmd->argv, "trace: run_command:");
+   trace_env_argv_printf(cmd->env, cmd->argv, "trace: run_command:");
+
fflush(NULL);
 
 #ifndef GIT_WINDOWS_NATIVE
diff --git a/trace.c b/trace.c
index b7530b51a9..d8967b66e8 100644
--- a/trace.c
+++ b/trace.c
@@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int line, 
struct trace_key *key,
print_trace_line(key, );
 }
 
+static void concatenate_env(struct strbuf *dst, const char *const *env)
+{
+   int i;
+
+   /* Copy into destination buffer. */
+   strbuf_grow(dst, 255);
+   for (i = 0; env[i]; ++i) {
+   /*
+* the main interesting is setting new vars
+* e.g. GIT_DIR, ignore the unsetting to reduce noise.
+*/
+   if (!strchr(env[i], '='))
+   continue;
+   strbuf_addch(dst, ' ');
+   sq_quote_buf(dst, env[i]);
+   }
+}
+
 static void trace_argv_vprintf_fl(const char *file, int line,
+ const char *const *env,
  const char **argv, const char *format,
  va_list ap)
 {
@@ -157,6 +176,9 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 
strbuf_vaddf(, format, ap);
 
+   if (env)
+   concatenate_env(, env);
+
sq_quote_argv(, argv, 0);
print_trace_line(_default_key, );
 }
@@ -214,7 +236,16 @@ void trace_argv_printf(const char **argv, const char 
*format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_argv_vprintf_fl(NULL, 0, argv, format, ap);
+   trace_argv_vprintf_fl(NULL, 0, NULL, argv, format, ap);
+   va_end(ap);
+}
+
+void trace_env_argv_printf(const char *const *env, const char **argv,
+  const char *format, ...)
+{
+   va_list ap;
+   va_start(ap, format);
+   trace_argv_vprintf_fl(NULL, 0, env, argv, format, ap);
va_end(ap);
 }
 
@@ -251,12 +282,13 @@ void trace_printf_key_fl(const char *file, int line, 
struct trace_key *key,
va_end(ap);
 }
 
-void trace_argv_printf_fl(const char *file, int line, const char **argv,
+void trace_argv_printf_fl(const char *file, int line,
+ const char *const *env, const char **argv,
  const char *format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_argv_vprintf_fl(file, line, argv, format, ap);
+   trace_argv_vprintf_fl(file, line, env, argv, format, ap);
va_end(ap);
 }
 
diff --git a/trace.h b/trace.h
index 88055abef7..830d9dcd19 100644
--- a/trace.h
+++ b/trace.h
@@ -34,6 +34,10 @@ extern void trace_printf_key(struct trace_key *key, const 
char *format, ...);
 __attribute__((format (printf, 2, 3)))
 extern void trace_argv_printf(const char **argv, const char *format, ...);
 
+__attribute__((format (printf, 3, 4)))
+extern void trace_env_argv_printf(const char * const*env, const char **argv,
+ const char *format, ...);
+
 extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
 
 /* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */
@@ -93,7 +97,14 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
do {\
if (trace_pass_fl(_default_key))  \
trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,   \
-   argv, __VA_ARGS__); \
+NULL, argv, __VA_ARGS__);  \
+   } while (0)
+
+#define trace_env_argv_printf(env, argv, ...)  \
+   do {\
+   if 

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

[Adam Dinwoodie]

On Wednesday 10 January 2018 at 04:07 pm +0700, Duy Nguyen wrote:
> On Mon, Jan 08, 2018 at 01:25:04PM +0100, Johannes Schindelin wrote:
> > I agree that it would make a ton of sense to use a proper, portable test
> > framework written in pure, portable C.
> > 
> > However, this ship has long sailed, hasn't it?
> 
> If you meant converting the whole test suite, oh yeah that's not gonna
> happen. But it's still possible to have some tests written in C.
> 
> I played a bit with this. The assumption is if it's agreed that we can
> get something bare bone (but functional) in then we could start having
> more and more C-based unit tests in future and also improve the C
> framework to be on par with shell one on the side.
> 
> There are still some minor problems with my patch, and a bunch of
> optional features not supported. But the numbers looks unexpectedly
> promising. 0.7 seconds on the shell version and 0.03 on the C one.

I see even more promising results from a single run on one of my Cygwin
systems: from 21.3 seconds for t3070 to 0.112 seconds for your t3071.  I
expect there's a similar improvement in Dscho's Git for Windows
environment.

> One disadvantage of this though, if this kind of framework does not
> get popular, then any new test feature must be added at both places
> but it's a waste of time to support both. So...

I don't follow: if we end up implementing every test twice, as we have
here, then I agree, but I don't think there's much value in doing that
except as a proof of concept, as in this immediate discussion.  The
obvious-to-me way to do this would be following the precedent of the
core code: gradually migrate things away from shell code to C code.

Adam

Re: [PATCH] t3900: add some more quotes

[Jeff King]

On Wed, Jan 10, 2018 at 10:58:32AM +0100, Beat Bolli wrote:

> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> hander, two files are deleted which must be quoted individually.

Doh. I can't believe I missed that when reading the patch.

Thanks.

-Peff

[PATCH] t3900: add some more quotes

[Beat Bolli]

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
hander, two files are deleted which must be quoted individually.

Signed-off-by: Beat Bolli 
---
 t/t3900-i18n-commit.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d9..dc00db87b 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
 '
 
 test_expect_success 'UTF-8 invalid characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
echo "UTF-8 characters" >F &&
printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
>"$HOME/invalid" &&
@@ -49,7 +49,7 @@ test_expect_success 'UTF-8 invalid characters refused' '
 '
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
rm -f "$HOME/stderr" "$HOME/invalid" &&
echo "UTF-8 overlong" >F &&
printf "\340\202\251ommit message\n\nThis is not a space:\300\240\n" \
@@ -59,7 +59,7 @@ test_expect_success 'UTF-8 overlong sequences rejected' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
echo "UTF-8 non-character 1" >F &&
printf "Commit message\n\nNon-character:\364\217\277\276\n" \
>"$HOME/invalid" &&
@@ -68,7 +68,7 @@ test_expect_success 'UTF-8 non-characters refused' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
echo "UTF-8 non-character 2." >F &&
printf "Commit message\n\nNon-character:\357\267\220\n" \
>"$HOME/invalid" &&
-- 
2.15.0.rc1.299.gda03b47c3

[PATCH v2 09/18] cat-file: simplify expand_atom function

[Olga Telezhnaya]

Not sure, but looks like there is no need in that checking.
There is a checking before whether it is null and we die in such case.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 9055fa3a8b0ae..dd43457c0ad7e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,10 +193,9 @@ static void expand_atom(struct strbuf *sb, const char 
*atom, int len,
strbuf_addf(sb, "%lu", data->size);
else if (is_atom("objectsize:disk", atom, len))
strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
-   else if (is_atom("rest", atom, len)) {
-   if (data->rest)
-   strbuf_addstr(sb, data->rest);
-   } else if (is_atom("deltabase", atom, len))
+   else if (is_atom("rest", atom, len))
+   strbuf_addstr(sb, data->rest);
+   else if (is_atom("deltabase", atom, len))
strbuf_addstr(sb, oid_to_hex(>delta_base_oid));
else
die("unknown format element: %.*s", len, atom);

--
https://github.com/git/git/pull/450

[PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Olga Telezhnaya]

Make valid_atom as a function parameter,
there could be another variable further.
Need that for further reusing of formatting logic in cat-file.c.

We do not need to allow users to pass their own valid_atom variable in
global functions like verify_ref_format because in the end we want to
have same set of valid atoms for all commands. But, as a first step
of migrating, I create further another version of valid_atom
for cat-file.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3f9161707e66b..32b7e918bca75 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -325,7 +325,7 @@ static void head_atom_parser(const struct ref_format 
*format, struct used_atom *
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
 }
 
-static struct {
+static struct valid_atom {
const char *name;
cmp_type cmp_type;
void (*parser)(const struct ref_format *format, struct used_atom *atom, 
const char *arg);
@@ -396,6 +396,7 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
+const struct valid_atom *valid_atom, int 
n_atoms,
 const char *atom, const char *ep)
 {
const char *sp;
@@ -425,13 +426,13 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
atom_len = (arg ? arg : ep) - sp;
 
/* Is the atom a valid one? */
-   for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
+   for (i = 0; i < n_atoms; i++) {
int len = strlen(valid_atom[i].name);
if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
break;
}
 
-   if (ARRAY_SIZE(valid_atom) <= i)
+   if (n_atoms <= i)
die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
 
/* Add it in, including the deref prefix */
@@ -708,7 +709,8 @@ int verify_ref_format(struct ref_format *format)
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
-   at = parse_ref_filter_atom(format, sp + 2, ep);
+   at = parse_ref_filter_atom(format, valid_atom,
+  ARRAY_SIZE(valid_atom), sp + 2, ep);
cp = ep + 1;
 
if (skip_prefix(used_atom[at].name, "color:", ))
@@ -2139,7 +2141,9 @@ void format_ref_array_item(struct ref_array_item *info,
if (cp < sp)
append_literal(cp, sp, );
get_ref_atom_value(info,
-  parse_ref_filter_atom(format, sp + 2, ep),
+  parse_ref_filter_atom(format, valid_atom,
+ARRAY_SIZE(valid_atom),
+sp + 2, ep),
   );
atomv->handler(atomv, );
}
@@ -2187,7 +2191,8 @@ static int parse_sorting_atom(const char *atom)
 */
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
-   return parse_ref_filter_atom(, atom, end);
+   return parse_ref_filter_atom(, valid_atom,
+ARRAY_SIZE(valid_atom), atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */

--
https://github.com/git/git/pull/450

[PATCH v2 06/18] ref-filter: reuse parse_ref_filter_atom

[Olga Telezhnaya]

Continue migrating formatting logic from cat-file to ref-filter.
Reuse parse_ref_filter_atom for unifying all processes in ref-filter
and further reducing of expand_atom_into_fields function.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 38 +++---
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6ca4a671086ee..bb09875e2dbf4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,6 +100,7 @@ static struct used_atom {
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
+struct expand_data *cat_file_info;
 
 static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
 {
@@ -251,6 +252,16 @@ static void objectname_atom_parser(const struct ref_format 
*format, struct used_
die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
+static void objectsize_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
+{
+   if (!arg)
+   ; /* default to normal object size */
+   else if (!strcmp(arg, "disk"))
+   cat_file_info->info.disk_sizep = _file_info->disk_size;
+   else
+   die(_("urecognized %%(objectsize) argument: %s"), arg);
+}
+
 static void refname_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
refname_atom_parser_internal(>u.refname, arg, atom->name);
@@ -371,6 +382,14 @@ static struct valid_atom {
{ "else" },
 };
 
+static struct valid_atom valid_cat_file_atom[] = {
+   { "objectname" },
+   { "objecttype" },
+   { "objectsize", FIELD_ULONG, objectsize_atom_parser },
+   { "rest" },
+   { "deltabase" },
+};
+
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
 struct ref_formatting_stack {
@@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int 
slen)
 static void expand_atom_into_fields(const char *atom, int len,
struct expand_data *data)
 {
-   if (is_atom("objectname", atom, len))
-   ; /* do nothing */
-   else if (is_atom("objecttype", atom, len))
+   if (is_atom("objecttype", atom, len))
data->info.typep = >type;
else if (is_atom("objectsize", atom, len))
data->info.sizep = >size;
-   else if (is_atom("objectsize:disk", atom, len))
-   data->info.disk_sizep = >disk_size;
else if (is_atom("rest", atom, len))
data->split_on_whitespace = 1;
else if (is_atom("deltabase", atom, len))
data->info.delta_base_sha1 = data->delta_base_oid.hash;
-   else
-   die("unknown format element: %.*s", len, atom);
 }
 
 /*
@@ -483,6 +496,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
+   if (cat_file_info)
+   expand_atom_into_fields(atom, atom_len, cat_file_info);
return at;
 }
 
@@ -726,6 +741,7 @@ int verify_ref_format(struct ref_format *format)
 {
const char *cp, *sp;
 
+   cat_file_info = format->cat_file_data;
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
const char *color, *ep = strchr(sp, ')');
@@ -735,10 +751,10 @@ int verify_ref_format(struct ref_format *format)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
 
-   if (format->cat_file_data)
-   expand_atom_into_fields(sp + 2, ep - sp - 2,
-   format->cat_file_data);
-   else {
+   if (format->cat_file_data) {
+   at = parse_ref_filter_atom(format, valid_cat_file_atom,
+  
ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
+   } else {
at = parse_ref_filter_atom(format, valid_atom,
   ARRAY_SIZE(valid_atom), sp + 
2, ep);
if (skip_prefix(used_atom[at].name, "color:", ))

--
https://github.com/git/git/pull/450

[PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Olga Telezhnaya]

Need that for further reusing of formatting logic in cat-file.
Have plans to get rid of using expand_data in cat-file at all,
and use it only in ref-filter for collecting, formatting and printing
needed data.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 36 
 ref-filter.h   | 36 
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7655a9a726773..7fd5b960ad698 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -176,42 +176,6 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
return 0;
 }
 
-struct expand_data {
-   struct object_id oid;
-   enum object_type type;
-   unsigned long size;
-   off_t disk_size;
-   const char *rest;
-   struct object_id delta_base_oid;
-
-   /*
-* If mark_query is true, we do not expand anything, but rather
-* just mark the object_info with items we wish to query.
-*/
-   int mark_query;
-
-   /*
-* Whether to split the input on whitespace before feeding it to
-* get_sha1; this is decided during the mark_query phase based on
-* whether we have a %(rest) token in our format.
-*/
-   int split_on_whitespace;
-
-   /*
-* After a mark_query run, this object_info is set up to be
-* passed to sha1_object_info_extended. It will point to the data
-* elements above, so you can retrieve the response from there.
-*/
-   struct object_info info;
-
-   /*
-* This flag will be true if the requested batch format and options
-* don't require us to call sha1_object_info, which can then be
-* optimized out.
-*/
-   unsigned skip_object_info : 1;
-};
-
 static int is_atom(const char *atom, const char *s, int slen)
 {
int alen = strlen(atom);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..16d00e4b1bded 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -72,6 +72,42 @@ struct ref_filter {
verbose;
 };
 
+struct expand_data {
+   struct object_id oid;
+   enum object_type type;
+   unsigned long size;
+   off_t disk_size;
+   const char *rest;
+   struct object_id delta_base_oid;
+
+   /*
+* If mark_query is true, we do not expand anything, but rather
+* just mark the object_info with items we wish to query.
+*/
+   int mark_query;
+
+   /*
+* Whether to split the input on whitespace before feeding it to
+* get_sha1; this is decided during the mark_query phase based on
+* whether we have a %(rest) token in our format.
+*/
+   int split_on_whitespace;
+
+   /*
+* After a mark_query run, this object_info is set up to be
+* passed to sha1_object_info_extended. It will point to the data
+* elements above, so you can retrieve the response from there.
+*/
+   struct object_info info;
+
+   /*
+* This flag will be true if the requested batch format and options
+* don't require us to call sha1_object_info, which can then be
+* optimized out.
+*/
+   unsigned skip_object_info : 1;
+};
+
 struct ref_format {
/*
 * Set these to define the format; make sure you call

--
https://github.com/git/git/pull/450

[PATCH v2 12/18] ref-filter: make populate_value global

[Olga Telezhnaya]

Make function global for further using in cat-file.
Also added return value for handling errors.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 4 ++--
 ref-filter.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 575c5351d0f79..c15906cb091c7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1454,7 +1454,7 @@ static void need_object(struct ref_array_item *ref) {
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+int populate_value(struct ref_array_item *ref)
 {
int i;
 
@@ -1575,7 +1575,7 @@ static void populate_value(struct ref_array_item *ref)
break;
}
}
-   return;
+   return 0;
 }
 
 /*
diff --git a/ref-filter.h b/ref-filter.h
index 9bd36243481b4..6df45c5bd9dcb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -176,4 +176,7 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const unsigned char *sha1,
  const struct ref_format *format);
 
+/* Fill the values of request and prepare all data for final string creation */
+int populate_value(struct ref_array_item *ref);
+
 #endif /*  REF_FILTER_H  */

--
https://github.com/git/git/pull/450

[PATCH v2 13/18] cat-file: start reusing populate_value

[Olga Telezhnaya]

Move logic related to getting object info from cat-file to ref-filter.
It will help to reuse whole formatting logic from ref-filter further.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 16 +++-
 ref-filter.c   | 15 +++
 ref-filter.h   |  2 ++
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1c92194faaede..e11dbf88e386c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -284,21 +284,11 @@ static void batch_object_write(const char *obj_name, 
struct batch_options *opt,
struct strbuf buf = STRBUF_INIT;
struct ref_array_item item;
 
-   if (!data->skip_object_info &&
-   sha1_object_info_extended(data->oid.hash, >info,
- OBJECT_INFO_LOOKUP_REPLACE) < 0) {
-   printf("%s missing\n",
-  obj_name ? obj_name : oid_to_hex(>oid));
-   fflush(stdout);
-   return;
-   }
-
item.objectname = data->oid;
-   item.type = data->type;
-   item.size = data->size;
-   item.disk_size = data->disk_size;
item.rest = data->rest;
-   item.delta_base_oid = data->delta_base_oid;
+   item.start_of_request = obj_name;
+
+   if (populate_value()) return;
 
strbuf_expand(, opt->format->format, expand_format, );
strbuf_addch(, '\n');
diff --git a/ref-filter.c b/ref-filter.c
index c15906cb091c7..35e16cec6d862 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1467,6 +1467,21 @@ int populate_value(struct ref_array_item *ref)
ref->symref = "";
}
 
+   if (cat_file_info) {
+   if (!cat_file_info->skip_object_info &&
+   sha1_object_info_extended(ref->objectname.hash, 
_file_info->info,
+ OBJECT_INFO_LOOKUP_REPLACE) < 0) {
+   const char *e = ref->start_of_request;
+   printf("%s missing\n", e ? e : 
oid_to_hex(>objectname));
+   fflush(stdout);
+   return -1;
+   }
+   ref->type = cat_file_info->type;
+   ref->size = cat_file_info->size;
+   ref->disk_size = cat_file_info->disk_size;
+   ref->delta_base_oid = cat_file_info->delta_base_oid;
+   }
+
/* Fill in specials first */
for (i = 0; i < used_atom_cnt; i++) {
struct used_atom *atom = _atom[i];
diff --git a/ref-filter.h b/ref-filter.h
index 6df45c5bd9dcb..0c7253913735b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -45,6 +45,8 @@ struct ref_array_item {
off_t disk_size;
const char *rest;
struct object_id delta_base_oid;
+   /* Need it for better explanation in error log. */
+   const char *start_of_request;
char refname[FLEX_ARRAY];
 };
 

--
https://github.com/git/git/pull/450

[PATCH v2 11/18] cat-file: start use ref_array_item struct

[Olga Telezhnaya]

Moving from using expand_data to ref_array_item structure.
That helps us to reuse functions from ref-filter easier.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 30 +++---
 ref-filter.h   |  5 +
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1f331559e55c7..1c92194faaede 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -183,26 +183,26 @@ static int is_atom(const char *atom, const char *s, int 
slen)
 }
 
 static void expand_atom(struct strbuf *sb, const char *atom, int len,
-struct expand_data *data)
+struct ref_array_item *item)
 {
if (is_atom("objectname", atom, len))
-   strbuf_addstr(sb, oid_to_hex(>oid));
+   strbuf_addstr(sb, oid_to_hex(>objectname));
else if (is_atom("objecttype", atom, len))
-   strbuf_addstr(sb, typename(data->type));
+   strbuf_addstr(sb, typename(item->type));
else if (is_atom("objectsize", atom, len))
-   strbuf_addf(sb, "%lu", data->size);
+   strbuf_addf(sb, "%lu", item->size);
else if (is_atom("objectsize:disk", atom, len))
-   strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+   strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)item->disk_size);
else if (is_atom("rest", atom, len))
-   strbuf_addstr(sb, data->rest);
+   strbuf_addstr(sb, item->rest);
else if (is_atom("deltabase", atom, len))
-   strbuf_addstr(sb, oid_to_hex(>delta_base_oid));
+   strbuf_addstr(sb, oid_to_hex(>delta_base_oid));
 }
 
-static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
+static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 {
const char *end;
-   struct expand_data *data = vdata;
+   struct ref_array_item *item = data;
 
if (*start != '(')
return 0;
@@ -210,7 +210,7 @@ static size_t expand_format(struct strbuf *sb, const char 
*start, void *vdata)
if (!end)
die("format element '%s' does not end in ')'", start);
 
-   expand_atom(sb, start + 1, end - start - 1, data);
+   expand_atom(sb, start + 1, end - start - 1, item);
return end - start + 1;
 }
 
@@ -282,6 +282,7 @@ static void batch_object_write(const char *obj_name, struct 
batch_options *opt,
   struct expand_data *data)
 {
struct strbuf buf = STRBUF_INIT;
+   struct ref_array_item item;
 
if (!data->skip_object_info &&
sha1_object_info_extended(data->oid.hash, >info,
@@ -292,7 +293,14 @@ static void batch_object_write(const char *obj_name, 
struct batch_options *opt,
return;
}
 
-   strbuf_expand(, opt->format->format, expand_format, data);
+   item.objectname = data->oid;
+   item.type = data->type;
+   item.size = data->size;
+   item.disk_size = data->disk_size;
+   item.rest = data->rest;
+   item.delta_base_oid = data->delta_base_oid;
+
+   strbuf_expand(, opt->format->format, expand_format, );
strbuf_addch(, '\n');
batch_write(opt, buf.buf, buf.len);
strbuf_release();
diff --git a/ref-filter.h b/ref-filter.h
index 590a60ffe034d..9bd36243481b4 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -40,6 +40,11 @@ struct ref_array_item {
const char *symref;
struct commit *commit;
struct atom_value *value;
+   enum object_type type;
+   unsigned long size;
+   off_t disk_size;
+   const char *rest;
+   struct object_id delta_base_oid;
char refname[FLEX_ARRAY];
 };
 

--
https://github.com/git/git/pull/450

[PATCH v2 10/18] cat-file: get rid of duplicate checking

[Olga Telezhnaya]

We could remove this because we have already checked that
at verify_ref_format function in ref-filter.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index dd43457c0ad7e..1f331559e55c7 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -197,8 +197,6 @@ static void expand_atom(struct strbuf *sb, const char 
*atom, int len,
strbuf_addstr(sb, data->rest);
else if (is_atom("deltabase", atom, len))
strbuf_addstr(sb, oid_to_hex(>delta_base_oid));
-   else
-   die("unknown format element: %.*s", len, atom);
 }
 
 static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)

--
https://github.com/git/git/pull/450

[PATCH v2 07/18] cat-file: remove unused code

[Olga Telezhnaya]

No further need in mark_query parameter.
All logic related to expand_atom_into_fields is not needed here also,
we are doing that in ref-filter.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 25 +
 ref-filter.h   |  6 --
 2 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0a3f4a47bf1ae..9055fa3a8b0ae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -182,25 +182,6 @@ static int is_atom(const char *atom, const char *s, int 
slen)
return alen == slen && !memcmp(atom, s, alen);
 }
 
-static void expand_atom_into_fields(struct strbuf *sb, const char *atom, int 
len,
-   struct expand_data *data)
-{
-   if (is_atom("objectname", atom, len))
-   ; /* do nothing */
-   else if (is_atom("objecttype", atom, len))
-   data->info.typep = >type;
-   else if (is_atom("objectsize", atom, len))
-   data->info.sizep = >size;
-   else if (is_atom("objectsize:disk", atom, len))
-   data->info.disk_sizep = >disk_size;
-   else if (is_atom("rest", atom, len))
-   data->split_on_whitespace = 1;
-   else if (is_atom("deltabase", atom, len))
-   data->info.delta_base_sha1 = data->delta_base_oid.hash;
-   else
-   die("unknown format element: %.*s", len, atom);
-}
-
 static void expand_atom(struct strbuf *sb, const char *atom, int len,
 struct expand_data *data)
 {
@@ -232,11 +213,7 @@ static size_t expand_format(struct strbuf *sb, const char 
*start, void *vdata)
if (!end)
die("format element '%s' does not end in ')'", start);
 
-   if (data->mark_query)
-   expand_atom_into_fields(sb, start + 1, end - start - 1, data);
-   else
-   expand_atom(sb, start + 1, end - start - 1, data);
-
+   expand_atom(sb, start + 1, end - start - 1, data);
return end - start + 1;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 97169548939cd..590a60ffe034d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -80,12 +80,6 @@ struct expand_data {
const char *rest;
struct object_id delta_base_oid;
 
-   /*
-* If mark_query is true, we do not expand anything, but rather
-* just mark the object_info with items we wish to query.
-*/
-   int mark_query;
-
/*
 * Whether to split the input on whitespace before feeding it to
 * get_sha1; this is decided during the mark_query phase based on

--
https://github.com/git/git/pull/450

[PATCH v2 17/18] cat-file: move skip_object_info into ref-filter

[Olga Telezhnaya]

Move logic related to skip_object_info into ref-filter,
so that cat-file does not use that field at all.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 7 +--
 ref-filter.c   | 5 +
 ref-filter.h   | 1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 5aac10b9808ff..19cee0d22fabe 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -395,16 +395,11 @@ static int batch_objects(struct batch_options *opt)
opt->format->cat_file_data = 
opt->format->is_cat = 1;
opt->format->split_on_whitespace = _on_whitespace;
+   opt->format->all_objects = opt->all_objects;
verify_ref_format(opt->format);
if (opt->cmdmode)
data.split_on_whitespace = 1;
 
-   if (opt->all_objects) {
-   struct object_info empty = OBJECT_INFO_INIT;
-   if (!memcmp(, , sizeof(empty)))
-   data.skip_object_info = 1;
-   }
-
/*
 * If we are printing out the object, then always fill in the type,
 * since we will want to decide whether or not to stream.
diff --git a/ref-filter.c b/ref-filter.c
index 0fb33198fb450..83bacdd8bdd78 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -764,6 +764,11 @@ int verify_ref_format(struct ref_format *format)
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
+   if (is_cat && format->all_objects) {
+   struct object_info empty = OBJECT_INFO_INIT;
+   if (!memcmp(_file_info->info, , sizeof(empty)))
+   cat_file_info->skip_object_info = 1;
+   }
return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index aaf1790e43e62..e588d1f50eef6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -128,6 +128,7 @@ struct ref_format {
struct expand_data *cat_file_data;
int is_cat;
int *split_on_whitespace;
+   int all_objects;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/450

[PATCH v2 05/18] cat-file: start migrating to ref-filter

[Olga Telezhnaya]

Start moving all formatting stuff from cat-file to ref-filter.
Start from simple moving, it would be integrated into
all ref-filter processes further.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c |  5 ++---
 ref-filter.c   | 41 -
 ref-filter.h   |  6 ++
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7fd5b960ad698..0a3f4a47bf1ae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -420,9 +420,8 @@ static int batch_objects(struct batch_options *opt)
 * object.
 */
memset(, 0, sizeof(data));
-   data.mark_query = 1;
-   strbuf_expand(, opt->format->format, expand_format, );
-   data.mark_query = 0;
+   opt->format->cat_file_data = 
+   verify_ref_format(opt->format);
if (opt->cmdmode)
data.split_on_whitespace = 1;
 
diff --git a/ref-filter.c b/ref-filter.c
index 32b7e918bca75..6ca4a671086ee 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -392,6 +392,31 @@ struct atom_value {
struct used_atom *atom;
 };
 
+static int is_atom(const char *atom, const char *s, int slen)
+{
+   int alen = strlen(atom);
+   return alen == slen && !memcmp(atom, s, alen);
+}
+
+static void expand_atom_into_fields(const char *atom, int len,
+   struct expand_data *data)
+{
+   if (is_atom("objectname", atom, len))
+   ; /* do nothing */
+   else if (is_atom("objecttype", atom, len))
+   data->info.typep = >type;
+   else if (is_atom("objectsize", atom, len))
+   data->info.sizep = >size;
+   else if (is_atom("objectsize:disk", atom, len))
+   data->info.disk_sizep = >disk_size;
+   else if (is_atom("rest", atom, len))
+   data->split_on_whitespace = 1;
+   else if (is_atom("deltabase", atom, len))
+   data->info.delta_base_sha1 = data->delta_base_oid.hash;
+   else
+   die("unknown format element: %.*s", len, atom);
+}
+
 /*
  * Used to parse format string and sort specifiers
  */
@@ -709,12 +734,18 @@ int verify_ref_format(struct ref_format *format)
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
-   at = parse_ref_filter_atom(format, valid_atom,
-  ARRAY_SIZE(valid_atom), sp + 2, ep);
-   cp = ep + 1;
 
-   if (skip_prefix(used_atom[at].name, "color:", ))
-   format->need_color_reset_at_eol = !!strcmp(color, 
"reset");
+   if (format->cat_file_data)
+   expand_atom_into_fields(sp + 2, ep - sp - 2,
+   format->cat_file_data);
+   else {
+   at = parse_ref_filter_atom(format, valid_atom,
+  ARRAY_SIZE(valid_atom), sp + 
2, ep);
+   if (skip_prefix(used_atom[at].name, "color:", ))
+   format->need_color_reset_at_eol = 
!!strcmp(color, "reset");
+   }
+
+   cp = ep + 1;
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
diff --git a/ref-filter.h b/ref-filter.h
index 16d00e4b1bded..97169548939cd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,6 +119,12 @@ struct ref_format {
 
/* Internal state to ref-filter */
int need_color_reset_at_eol;
+
+   /*
+* Helps to move all formatting logic from cat-file to ref-filter,
+* hopefully would be reduced later.
+*/
+   struct expand_data *cat_file_data;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/450

[PATCH v2 02/18] cat-file: reuse struct ref_format

[Olga Telezhnaya]

Start using ref_format struct instead of simple char*.
Need that for further reusing of formatting logic from ref-filter.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f783b39b9bd5c..7655a9a726773 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -13,15 +13,16 @@
 #include "tree-walk.h"
 #include "sha1-array.h"
 #include "packfile.h"
+#include "ref-filter.h"
 
 struct batch_options {
+   struct ref_format *format;
int enabled;
int follow_symlinks;
int print_contents;
int buffer_output;
int all_objects;
int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
-   const char *format;
 };
 
 static const char *force_path;
@@ -353,7 +354,7 @@ static void batch_object_write(const char *obj_name, struct 
batch_options *opt,
return;
}
 
-   strbuf_expand(, opt->format, expand_format, data);
+   strbuf_expand(, opt->format->format, expand_format, data);
strbuf_addch(, '\n');
batch_write(opt, buf.buf, buf.len);
strbuf_release();
@@ -446,8 +447,8 @@ static int batch_objects(struct batch_options *opt)
int save_warning;
int retval = 0;
 
-   if (!opt->format)
-   opt->format = "%(objectname) %(objecttype) %(objectsize)";
+   if (!opt->format->format)
+   opt->format->format = "%(objectname) %(objecttype) 
%(objectsize)";
 
/*
 * Expand once with our special mark_query flag, which will prime the
@@ -456,7 +457,7 @@ static int batch_objects(struct batch_options *opt)
 */
memset(, 0, sizeof(data));
data.mark_query = 1;
-   strbuf_expand(, opt->format, expand_format, );
+   strbuf_expand(, opt->format->format, expand_format, );
data.mark_query = 0;
if (opt->cmdmode)
data.split_on_whitespace = 1;
@@ -548,7 +549,7 @@ static int batch_option_callback(const struct option *opt,
 
bo->enabled = 1;
bo->print_contents = !strcmp(opt->long_name, "batch");
-   bo->format = arg;
+   bo->format->format = arg;
 
return 0;
 }
@@ -557,7 +558,8 @@ int cmd_cat_file(int argc, const char **argv, const char 
*prefix)
 {
int opt = 0;
const char *exp_type = NULL, *obj_name = NULL;
-   struct batch_options batch = {0};
+   struct ref_format format = REF_FORMAT_INIT;
+   struct batch_options batch = {};
int unknown_type = 0;
 
const struct option options[] = {

--
https://github.com/git/git/pull/450

[PATCH v2 18/18] ref-filter: make cat_file_info independent

[Olga Telezhnaya]

Remove connection between expand_data variable
in cat-file and in ref-filter.
It will help further to get rid of using expand_data in cat-file.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c |  2 +-
 ref-filter.c   | 28 ++--
 ref-filter.h   |  1 -
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 19cee0d22fabe..ffa8e61213e36 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -289,6 +289,7 @@ static void batch_object_write(const char *obj_name, struct 
batch_options *opt,
item.start_of_request = obj_name;
 
if (populate_value()) return;
+   data->type = item.type;
 
strbuf_expand(, opt->format->format, expand_format, );
strbuf_addch(, '\n');
@@ -392,7 +393,6 @@ static int batch_objects(struct batch_options *opt)
 * object.
 */
memset(, 0, sizeof(data));
-   opt->format->cat_file_data = 
opt->format->is_cat = 1;
opt->format->split_on_whitespace = _on_whitespace;
opt->format->all_objects = opt->all_objects;
diff --git a/ref-filter.c b/ref-filter.c
index 83bacdd8bdd78..aee6be32f53ef 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,7 +100,7 @@ static struct used_atom {
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
-struct expand_data *cat_file_info;
+struct expand_data cat_file_info;
 static int is_cat = 0;
 
 static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
@@ -256,9 +256,9 @@ static void objectname_atom_parser(const struct ref_format 
*format, struct used_
 static void objectsize_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   cat_file_info->info.sizep = _file_info->size;
+   cat_file_info.info.sizep = _file_info.size;
else if (!strcmp(arg, "disk"))
-   cat_file_info->info.disk_sizep = _file_info->disk_size;
+   cat_file_info.info.disk_sizep = _file_info.disk_size;
else
die(_("urecognized %%(objectsize) argument: %s"), arg);
 }
@@ -266,7 +266,7 @@ static void objectsize_atom_parser(const struct ref_format 
*format, struct used_
 static void objecttype_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   cat_file_info->info.typep = _file_info->type;
+   cat_file_info.info.typep = _file_info.type;
else
die(_("urecognized %%(objecttype) argument: %s"), arg);
 }
@@ -274,7 +274,7 @@ static void objecttype_atom_parser(const struct ref_format 
*format, struct used_
 static void deltabase_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   cat_file_info->info.delta_base_sha1 = 
cat_file_info->delta_base_oid.hash;
+   cat_file_info.info.delta_base_sha1 = 
cat_file_info.delta_base_oid.hash;
else
die(_("urecognized %%(deltabase) argument: %s"), arg);
 }
@@ -739,7 +739,6 @@ int verify_ref_format(struct ref_format *format)
 {
const char *cp, *sp;
 
-   cat_file_info = format->cat_file_data;
is_cat = format->is_cat;
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
@@ -766,8 +765,8 @@ int verify_ref_format(struct ref_format *format)
format->need_color_reset_at_eol = 0;
if (is_cat && format->all_objects) {
struct object_info empty = OBJECT_INFO_INIT;
-   if (!memcmp(_file_info->info, , sizeof(empty)))
-   cat_file_info->skip_object_info = 1;
+   if (!memcmp(_file_info.info, , sizeof(empty)))
+   cat_file_info.skip_object_info = 1;
}
return 0;
 }
@@ -1472,18 +1471,19 @@ int populate_value(struct ref_array_item *ref)
}
 
if (is_cat) {
-   if (!cat_file_info->skip_object_info &&
-   sha1_object_info_extended(ref->objectname.hash, 
_file_info->info,
+   if (!cat_file_info.info.typep) cat_file_info.info.typep = 
_file_info.type;
+   if (!cat_file_info.skip_object_info &&
+   sha1_object_info_extended(ref->objectname.hash, 
_file_info.info,
  OBJECT_INFO_LOOKUP_REPLACE) < 0) {
const char *e = ref->start_of_request;
printf("%s missing\n", e ? e : 
oid_to_hex(>objectname));
fflush(stdout);
return -1;
}
-   ref->type = cat_file_info->type;
-   ref->size = cat_file_info->size;
-   

[PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Olga Telezhnaya]

Split expand_atom function into 2 different functions,
expand_atom_into_fields prepares variable for further filling,
(new) expand_atom creates resulting string.
Need that for further reusing of formatting logic from ref-filter.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 73 +-
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75af26..f783b39b9bd5c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -217,47 +217,49 @@ static int is_atom(const char *atom, const char *s, int 
slen)
return alen == slen && !memcmp(atom, s, alen);
 }
 
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-   void *vdata)
+static void expand_atom_into_fields(struct strbuf *sb, const char *atom, int 
len,
+   struct expand_data *data)
 {
-   struct expand_data *data = vdata;
+   if (is_atom("objectname", atom, len))
+   ; /* do nothing */
+   else if (is_atom("objecttype", atom, len))
+   data->info.typep = >type;
+   else if (is_atom("objectsize", atom, len))
+   data->info.sizep = >size;
+   else if (is_atom("objectsize:disk", atom, len))
+   data->info.disk_sizep = >disk_size;
+   else if (is_atom("rest", atom, len))
+   data->split_on_whitespace = 1;
+   else if (is_atom("deltabase", atom, len))
+   data->info.delta_base_sha1 = data->delta_base_oid.hash;
+   else
+   die("unknown format element: %.*s", len, atom);
+}
 
-   if (is_atom("objectname", atom, len)) {
-   if (!data->mark_query)
-   strbuf_addstr(sb, oid_to_hex(>oid));
-   } else if (is_atom("objecttype", atom, len)) {
-   if (data->mark_query)
-   data->info.typep = >type;
-   else
-   strbuf_addstr(sb, typename(data->type));
-   } else if (is_atom("objectsize", atom, len)) {
-   if (data->mark_query)
-   data->info.sizep = >size;
-   else
-   strbuf_addf(sb, "%lu", data->size);
-   } else if (is_atom("objectsize:disk", atom, len)) {
-   if (data->mark_query)
-   data->info.disk_sizep = >disk_size;
-   else
-   strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
-   } else if (is_atom("rest", atom, len)) {
-   if (data->mark_query)
-   data->split_on_whitespace = 1;
-   else if (data->rest)
+static void expand_atom(struct strbuf *sb, const char *atom, int len,
+struct expand_data *data)
+{
+   if (is_atom("objectname", atom, len))
+   strbuf_addstr(sb, oid_to_hex(>oid));
+   else if (is_atom("objecttype", atom, len))
+   strbuf_addstr(sb, typename(data->type));
+   else if (is_atom("objectsize", atom, len))
+   strbuf_addf(sb, "%lu", data->size);
+   else if (is_atom("objectsize:disk", atom, len))
+   strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+   else if (is_atom("rest", atom, len)) {
+   if (data->rest)
strbuf_addstr(sb, data->rest);
-   } else if (is_atom("deltabase", atom, len)) {
-   if (data->mark_query)
-   data->info.delta_base_sha1 = data->delta_base_oid.hash;
-   else
-   strbuf_addstr(sb,
- oid_to_hex(>delta_base_oid));
-   } else
+   } else if (is_atom("deltabase", atom, len))
+   strbuf_addstr(sb, oid_to_hex(>delta_base_oid));
+   else
die("unknown format element: %.*s", len, atom);
 }
 
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
+static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
 {
const char *end;
+   struct expand_data *data = vdata;
 
if (*start != '(')
return 0;
@@ -265,7 +267,10 @@ static size_t expand_format(struct strbuf *sb, const char 
*start, void *data)
if (!end)
die("format element '%s' does not end in ')'", start);
 
-   expand_atom(sb, start + 1, end - start - 1, data);
+   if (data->mark_query)
+   expand_atom_into_fields(sb, start + 1, end - start - 1, data);
+   else
+   expand_atom(sb, start + 1, end - start - 1, data);
 
return end - start + 1;
 }

--
https://github.com/git/git/pull/450

[PATCH v2 15/18] ref-filter: add is_cat flag

[Olga Telezhnaya]

Add is_cat flag, further it helps to get rid of cat_file_data field
in ref_format.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 1 +
 ref-filter.c   | 8 +---
 ref-filter.h   | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e11dbf88e386c..289912ab1f858 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -393,6 +393,7 @@ static int batch_objects(struct batch_options *opt)
 */
memset(, 0, sizeof(data));
opt->format->cat_file_data = 
+   opt->format->is_cat = 1;
verify_ref_format(opt->format);
if (opt->cmdmode)
data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 93248ce18152f..2c955e90bb4c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,6 +101,7 @@ static struct used_atom {
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 struct expand_data *cat_file_info;
+static int is_cat = 0;
 
 static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
 {
@@ -493,7 +494,7 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
-   if (cat_file_info && !strcmp(valid_atoms[i].name, "rest"))
+   if (is_cat && !strcmp(valid_atoms[i].name, "rest"))
cat_file_info->split_on_whitespace = 1;
return at;
 }
@@ -739,6 +740,7 @@ int verify_ref_format(struct ref_format *format)
const char *cp, *sp;
 
cat_file_info = format->cat_file_data;
+   is_cat = format->is_cat;
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
const char *color, *ep = strchr(sp, ')');
@@ -748,7 +750,7 @@ int verify_ref_format(struct ref_format *format)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
 
-   if (format->cat_file_data) {
+   if (is_cat) {
at = parse_ref_filter_atom(format, valid_cat_file_atom,
   
ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
} else {
@@ -1464,7 +1466,7 @@ int populate_value(struct ref_array_item *ref)
ref->symref = "";
}
 
-   if (cat_file_info) {
+   if (is_cat) {
if (!cat_file_info->skip_object_info &&
sha1_object_info_extended(ref->objectname.hash, 
_file_info->info,
  OBJECT_INFO_LOOKUP_REPLACE) < 0) {
diff --git a/ref-filter.h b/ref-filter.h
index 0c7253913735b..12a938b0aa6a3 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -126,6 +126,7 @@ struct ref_format {
 * hopefully would be reduced later.
 */
struct expand_data *cat_file_data;
+   int is_cat;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/450

[PATCH v2 14/18] ref-filter: get rid of expand_atom_into_fields

[Olga Telezhnaya]

Remove expand_atom_into_fields function and create same logic
in terms of ref-filter style.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 45 +
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 35e16cec6d862..93248ce18152f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -255,13 +255,29 @@ static void objectname_atom_parser(const struct 
ref_format *format, struct used_
 static void objectsize_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   ; /* default to normal object size */
+   cat_file_info->info.sizep = _file_info->size;
else if (!strcmp(arg, "disk"))
cat_file_info->info.disk_sizep = _file_info->disk_size;
else
die(_("urecognized %%(objectsize) argument: %s"), arg);
 }
 
+static void objecttype_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
+{
+   if (!arg)
+   cat_file_info->info.typep = _file_info->type;
+   else
+   die(_("urecognized %%(objecttype) argument: %s"), arg);
+}
+
+static void deltabase_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
+{
+   if (!arg)
+   cat_file_info->info.delta_base_sha1 = 
cat_file_info->delta_base_oid.hash;
+   else
+   die(_("urecognized %%(deltabase) argument: %s"), arg);
+}
+
 static void refname_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
refname_atom_parser_internal(>u.refname, arg, atom->name);
@@ -384,10 +400,10 @@ static struct valid_atom {
 
 static struct valid_atom valid_cat_file_atom[] = {
{ "objectname" },
-   { "objecttype" },
+   { "objecttype", FIELD_STR, objecttype_atom_parser },
{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
{ "rest" },
-   { "deltabase" },
+   { "deltabase", FIELD_STR, deltabase_atom_parser },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -411,25 +427,6 @@ struct atom_value {
struct used_atom *atom;
 };
 
-static int is_atom(const char *atom, const char *s, int slen)
-{
-   int alen = strlen(atom);
-   return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void expand_atom_into_fields(const char *atom, int len,
-   struct expand_data *data)
-{
-   if (is_atom("objecttype", atom, len))
-   data->info.typep = >type;
-   else if (is_atom("objectsize", atom, len))
-   data->info.sizep = >size;
-   else if (is_atom("rest", atom, len))
-   data->split_on_whitespace = 1;
-   else if (is_atom("deltabase", atom, len))
-   data->info.delta_base_sha1 = data->delta_base_oid.hash;
-}
-
 /*
  * Used to parse format string and sort specifiers
  */
@@ -496,8 +493,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
-   if (cat_file_info)
-   expand_atom_into_fields(atom, atom_len, cat_file_info);
+   if (cat_file_info && !strcmp(valid_atoms[i].name, "rest"))
+   cat_file_info->split_on_whitespace = 1;
return at;
 }
 

--
https://github.com/git/git/pull/450

[PATCH v2 16/18] ref_format: add split_on_whitespace flag

[Olga Telezhnaya]

Add flag to ref_format struct so that we could pass needed info
to cat-file.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 1 +
 ref-filter.c   | 4 ++--
 ref-filter.h   | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 289912ab1f858..5aac10b9808ff 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -394,6 +394,7 @@ static int batch_objects(struct batch_options *opt)
memset(, 0, sizeof(data));
opt->format->cat_file_data = 
opt->format->is_cat = 1;
+   opt->format->split_on_whitespace = _on_whitespace;
verify_ref_format(opt->format);
if (opt->cmdmode)
data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 2c955e90bb4c9..0fb33198fb450 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -494,8 +494,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
-   if (is_cat && !strcmp(valid_atoms[i].name, "rest"))
-   cat_file_info->split_on_whitespace = 1;
+   if (!strcmp(valid_atom[i].name, "rest"))
+   *format->split_on_whitespace = 1;
return at;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 12a938b0aa6a3..aaf1790e43e62 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -127,6 +127,7 @@ struct ref_format {
 */
struct expand_data *cat_file_data;
int is_cat;
+   int *split_on_whitespace;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/450

[PATCH v2 08/18] ref-filter: get rid of goto

[Olga Telezhnaya]

Get rid of goto command in ref-filter for better readability.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 103 ++-
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index bb09875e2dbf4..575c5351d0f79 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1403,16 +1403,60 @@ static const char *get_refname(struct used_atom *atom, 
struct ref_array_item *re
return show_ref(>u.refname, ref->refname);
 }
 
+static void need_object(struct ref_array_item *ref) {
+   struct object *obj;
+   const struct object_id *tagged;
+   unsigned long size;
+   int eaten;
+   void *buf = get_obj(>objectname, , , );
+   if (!buf)
+   die(_("missing object %s for %s"),
+   oid_to_hex(>objectname), ref->refname);
+   if (!obj)
+   die(_("parse_object_buffer failed on %s for %s"),
+   oid_to_hex(>objectname), ref->refname);
+
+   grab_values(ref->value, 0, obj, buf, size);
+   if (!eaten)
+   free(buf);
+
+   /*
+* If there is no atom that wants to know about tagged
+* object, we are done.
+*/
+   if (!need_tagged || (obj->type != OBJ_TAG))
+   return;
+
+   /*
+* If it is a tag object, see if we use a value that derefs
+* the object, and if we do grab the object it refers to.
+*/
+   tagged = &((struct tag *)obj)->tagged->oid;
+
+   /*
+* NEEDSWORK: This derefs tag only once, which
+* is good to deal with chains of trust, but
+* is not consistent with what deref_tag() does
+* which peels the onion to the core.
+*/
+   buf = get_obj(tagged, , , );
+   if (!buf)
+   die(_("missing object %s for %s"),
+   oid_to_hex(tagged), ref->refname);
+   if (!obj)
+   die(_("parse_object_buffer failed on %s for %s"),
+   oid_to_hex(tagged), ref->refname);
+   grab_values(ref->value, 1, obj, buf, size);
+   if (!eaten)
+   free(buf);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct ref_array_item *ref)
 {
-   void *buf;
-   struct object *obj;
-   int eaten, i;
-   unsigned long size;
-   const struct object_id *tagged;
+   int i;
 
ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1526,53 +1570,12 @@ static void populate_value(struct ref_array_item *ref)
 
for (i = 0; i < used_atom_cnt; i++) {
struct atom_value *v = >value[i];
-   if (v->s == NULL)
-   goto need_obj;
+   if (v->s == NULL) {
+   need_object(ref);
+   break;
+   }
}
return;
-
- need_obj:
-   buf = get_obj(>objectname, , , );
-   if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(>objectname), ref->refname);
-   if (!obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(>objectname), ref->refname);
-
-   grab_values(ref->value, 0, obj, buf, size);
-   if (!eaten)
-   free(buf);
-
-   /*
-* If there is no atom that wants to know about tagged
-* object, we are done.
-*/
-   if (!need_tagged || (obj->type != OBJ_TAG))
-   return;
-
-   /*
-* If it is a tag object, see if we use a value that derefs
-* the object, and if we do grab the object it refers to.
-*/
-   tagged = &((struct tag *)obj)->tagged->oid;
-
-   /*
-* NEEDSWORK: This derefs tag only once, which
-* is good to deal with chains of trust, but
-* is not consistent with what deref_tag() does
-* which peels the onion to the core.
-*/
-   buf = get_obj(tagged, , , );
-   if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(tagged), ref->refname);
-   if (!obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(tagged), ref->refname);
-   grab_values(ref->value, 1, obj, buf, size);
-   if (!eaten)
-   free(buf);
 }
 
 /*

--
https://github.com/git/git/pull/450

Re: [PATCH] status: handle worktree renames

[Duy Nguyen]

On Wed, Jan 3, 2018 at 4:14 AM, Jeff Hostetler  wrote:
> Also, does this introduce any new cases for reporting conflicts?
> I haven't really thought about it too much yet, but if there was a
> divergent rename in both branches of a merge, do we now have to handle
> showing possibly 4 pathnames for a file?  (merge-base, branch-a,
> branch-b, worktree)

It's an interesting question but unfortunately I don't have an answer
(I read your mail earlier but waited for so long because I didn't know
the answer then).
-- 
Duy

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

[Duy Nguyen]

On Mon, Jan 08, 2018 at 01:25:04PM +0100, Johannes Schindelin wrote:
> I agree that it would make a ton of sense to use a proper, portable test
> framework written in pure, portable C.
> 
> However, this ship has long sailed, hasn't it?

If you meant converting the whole test suite, oh yeah that's not gonna
happen. But it's still possible to have some tests written in C.

I played a bit with this. The assumption is if it's agreed that we can
get something bare bone (but functional) in then we could start having
more and more C-based unit tests in future and also improve the C
framework to be on par with shell one on the side.

There are still some minor problems with my patch, and a bunch of
optional features not supported. But the numbers looks unexpectedly
promising. 0.7 seconds on the shell version and 0.03 on the C one.

One disadvantage of this though, if this kind of framework does not
get popular, then any new test feature must be added at both places
but it's a waste of time to support both. So...

Anyway here it is. t3071 is the same as t3070 (this is on master)

 Makefile |   2 +
 t/helper/test-3071-wildmatch.c (new) | 273 
++
 t/t3071-wildmatch.sh (new +x)|   3 +
 test-lib.c (new) |  97 ++
 test-lib.h (new) |   5 +

-- 8< --
diff --git a/Makefile b/Makefile
index 2a81ae22e9..567387b558 100644
--- a/Makefile
+++ b/Makefile
@@ -644,6 +644,7 @@ X =
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
+TEST_PROGRAMS_NEED_X += test-3071-wildmatch
 TEST_PROGRAMS_NEED_X += test-chmtime
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-config
@@ -895,6 +896,7 @@ LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
+LIB_OBJS += test-lib.o
 LIB_OBJS += tmp-objdir.o
 LIB_OBJS += trace.o
 LIB_OBJS += trailer.o
diff --git a/t/helper/test-3071-wildmatch.c b/t/helper/test-3071-wildmatch.c
new file mode 100644
index 00..24a657202d
--- /dev/null
+++ b/t/helper/test-3071-wildmatch.c
@@ -0,0 +1,273 @@
+#include "cache.h"
+#include "test-lib.h"
+
+struct match_input {
+   int expect_true;
+   const char *text;
+   const char *pattern;
+};
+
+static struct match_input match_tests[] = {
+   /* Basic wildmatch features */
+   { 1, "foo", "foo" },
+   { 0, "foo", "bar" },
+   { 1, "", "" },
+   { 1, "foo", "???" },
+   { 0, "foo", "??" },
+   { 1, "foo", "*" },
+   { 1, "foo", "f*" },
+   { 0, "foo", "*f" },
+   { 1, "foo", "*foo*" },
+   { 1, "foobar", "*ob*a*r*" },
+   { 1, "aaabababab", "*ab" },
+   { 1, "foo*", "foo\\*" },
+   { 0, "foobar", "foo\\*bar" },
+   { 1, "f\\oo", "foo" },
+   { 1, "ball", "*[al]?" },
+   { 0, "ten", "[ten]" },
+   { 0, "ten", "**[!te]" },
+   { 0, "ten", "**[!ten]" },
+   { 1, "ten", "t[a-g]n" },
+   { 0, "ten", "t[!a-g]n" },
+   { 1, "ton", "t[!a-g]n" },
+   { 1, "ton", "t[^a-g]n" },
+   { 1, "a]b", "a[]]b" },
+   { 1, "a-b", "a[]-]b" },
+   { 1, "a]b", "a[]-]b" },
+   { 0, "aab", "a[]-]b" },
+   { 1, "aab", "a[]a-]b" },
+   { 1, "]", "]" },
+
+   /* Extended slash-matching features */
+   { 0, "foo/baz/bar", "foo*bar" },
+   { 0, "foo/baz/bar", "foo**bar" },
+   { 0, "foobazbar", "foo**bar" },
+   { 1, "foo/baz/bar", "foo/**/bar" },
+   { 1, "foo/baz/bar", "foo/**/**/bar" },
+   { 1, "foo/b/a/z/bar", "foo/**/bar" },
+   { 1, "foo/b/a/z/bar", "foo/**/**/bar" },
+   { 1, "foo/bar", "foo/**/bar" },
+   { 1, "foo/bar", "foo/**/**/bar" },
+   { 0, "foo/bar", "foo?bar" },
+   { 0, "foo/bar", "foo[/]bar" },
+   { 0, "foo/bar", "foo[^a-z]bar" },
+   { 0, "foo/bar", "f[^eiu][^eiu][^eiu][^eiu][^eiu]r" },
+   { 1, "foo-bar", "f[^eiu][^eiu][^eiu][^eiu][^eiu]r" },
+   { 1, "foo", "**/foo" },
+   { 1, "XXX/foo", "**/foo" },
+   { 1, "bar/baz/foo", "**/foo" },
+   { 0, "bar/baz/foo", "*/foo" },
+   { 0, "foo/bar/baz", "**/bar*" },
+   { 1, "deep/foo/bar/baz", "**/bar/*" },
+   { 0, "deep/foo/bar/baz/", "**/bar/*" },
+   { 1, "deep/foo/bar/baz/", "**/bar/**" },
+   { 0, "deep/foo/bar", "**/bar/*" },
+   { 1, "deep/foo/bar/", "**/bar/**" },
+   { 0, "foo/bar/baz", "**/bar**" },
+   { 1, "foo/bar/baz/x", "*/bar/**" },
+   { 0, "deep/foo/bar/baz/x", "*/bar/**" },
+   { 1, "deep/foo/bar/baz/x", "**/bar/*/*" },
+
+   /* Various additional tests */
+   { 0, "acrt", "a[c-c]st" },
+   { 1, "acrt", "a[c-c]rt" },
+   { 0, "]", "[!]-]" },
+   { 1, "a", "[!]-]" },
+   { 0, "", "\\" },
+   { 0, "\\", "\\" },
+   { 0, "XXX/\\", "*/\\" },
+   { 1, "XXX/\\", "*/" },
+   { 1, "foo", "foo" },
+   { 1, "@foo", "@foo" },
+   { 0, "foo", "@foo" },
+   { 1, "[ab]", "\\[ab]" },
+   { 1, "[ab]", "[[]ab]" },
+