Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Jeff King]

On Tue, Jan 02, 2018 at 07:39:46PM -0500, Todd Zullinger wrote:

> Brandon Williams wrote:
> > Seems good to me.  I think I was just being overly cautious when i was
> > implementing those patches.  Thanks!
> 
> Cool, thanks for taking a look.
> 
> In this case, the test isn't dependent on apache > 2.4, but
> before I checked that I wondered if we had any way to run a
> test only if the apache version was greater or lesser than
> some release.  Luckily, I didn't have to work out such a
> method. :)
> 
> I don't know if there's a clean way to do that
> automatically, short of parsing the output of 'httpd -v'
> should we ever need to add such a prereq.

In the general case, we could probably define an endpoint within an 
block, and then try to access the endpoint from the test script.

E.g., something like:

= 2.4>
Alias /have-2.4.txt www/yes.txt


in the apache config, and then:

  test_lazy_prereq APACHE24 '
echo yes >"$HTTPD_DOCUMENT_ROOT_PATH/yes.txt" &&
curl -f "$HTTPD_URL/have-2.4.txt"
  '

in the test script (of course we may not want to depend on having
command-line curl, but we could replace that with "git ls-remote" or
similar).

One nice thing about that approach is that it can be extended to other
"If" blocks, like if we have a particular module available, or if ssl is
configured.

-Peff

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Jeff King]

On Sat, Dec 30, 2017 at 09:32:34PM -0500, Todd Zullinger wrote:

> The apache config used by tests was updated to use the SetEnvIf
> directive to set the Git-Protocol header in 19113a26b6 ("http: tell
> server that the client understands v1", 2017-10-16).
> 
> Setting the Git-Protocol header is restricted to httpd >= 2.4, but
> mod_setenvif and the SetEnvIf directive work with lower versions, at
> least as far back as 2.0, according to the httpd documentation:
> 
> https://httpd.apache.org/docs/2.0/mod/mod_setenvif.html
> 
> Drop the restriction.  Tested with httpd 2.2 and 2.4.

Makes sense. I think the only way this could backfire is if somebody has
a funny build that doesn't include mod_setenvif at all. But I don't
think we can know that for sure without applying this and seeing if
anybody screams.

> I removed the version restriction entirely rather than adjust
> the version.  I believe SetEnvIf works on httpd >= 2.0.  I'm
> not sure if we aim to support anything less than httpd 2.0,
> but I'm betting not.  If that's incorrect, I can add some
> IfVersion conditions.

IIRC, anything less than 2.0 is broken already. And it's not worth
changing that, given the age (and I think there were some pretty severe
hardships in making 1.3.x, but it's been a while).

-Peff

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Bryan Turner wrote:
> On Tue, Jan 2, 2018 at 9:07 PM, Jonathan Nieder  wrote:

>> So my first question is why the basename detection is not working for
>> you.  What value of GIT_SSH, GIT_SSH_COMMAND, or core.sshCommand are
>> you using?
>
> So I'd been digging further into this for the last hour because I
> wasn't seeing quite the behavior I was expecting when I ran Git from
> the command line on Ubuntu 12.04 or 14.04, and this nudged me to the
> right answer: We're setting GIT_SSH to a wrapper script. In our case,
> that wrapper script is just calling OpenSSH's ssh with all the
> provided arguments (plus a couple extra ones), but because we're
> setting GIT_SSH at all, that's why the auto variant code is running.
> That being the case, explicitly setting GIT_SSH_VARIANT=ssh may be the
> correct thing to do, to tell Git that we want to be treated like
> "normal" OpenSSH, as opposed to expecting Git to assume we behave like
> OpenSSH (when the Android repo use case clearly shows that assumption
> also doesn't hold).

Ah, that's a comfort.  Setting GIT_SSH_VARIANT would avoid this
autodetection code and is the recommended thing to do.

That said, we can't go back in time and update everyone's tools to do
that (e.g. there is not even a release of repo with [1] out yet), so
this is still considered a regression and I'm glad you found it.

Jonathan

[1] https://gerrit-review.googlesource.com/c/git-repo/+/134950

Re: [ANNOUNCE] Git v2.16.0-rc0

[Bryan Turner]

On Tue, Jan 2, 2018 at 9:07 PM, Jonathan Nieder  wrote:
> Hi Bryan,
>
> Bryan Turner wrote:
>
>> Our test environment is still on Ubuntu 12.04 LTS (it's a long story,
>> but one I doubt is unique to us), which means it's using OpenSSH 5.9.
>> ssh -G was added in OpenSSH 6.8 [1], circa March 2015, which means the
>> "auto" detection "fails" and chooses "simple" instead of "ssh". But
>> OpenSSH 5.9 _does_ support -4, -6 and -p. As a result, commands which
>> have been working without issue on all previous versions of Git start
>> to fail saying
>>
>> git -c gc.auto=0 -c credential.helper= fetch --force --prune --progress 
>> ssh://localhost:64281/repo.git +refs/*:refs/*' exited with code 128 saying: 
>> fatal: ssh variant 'simple' does not support setting port
>
> Hm, that's not expected.  git-config(1) says:
>
> By default, Git determines the command line arguments to use
> based on the basename of the configured SSH command
> (configured using the environment variable GIT_SSH or
> GIT_SSH_COMMAND or the config setting core.sshCommand). If the
> basename is unrecognized, Git will attempt to detect support
> of OpenSSH options by [...]
>
> So my first question is why the basename detection is not working for
> you.  What value of GIT_SSH, GIT_SSH_COMMAND, or core.sshCommand are
> you using?

So I'd been digging further into this for the last hour because I
wasn't seeing quite the behavior I was expecting when I ran Git from
the command line on Ubuntu 12.04 or 14.04, and this nudged me to the
right answer: We're setting GIT_SSH to a wrapper script. In our case,
that wrapper script is just calling OpenSSH's ssh with all the
provided arguments (plus a couple extra ones), but because we're
setting GIT_SSH at all, that's why the auto variant code is running.
That being the case, explicitly setting GIT_SSH_VARIANT=ssh may be the
correct thing to do, to tell Git that we want to be treated like
"normal" OpenSSH, as opposed to expecting Git to assume we behave like
OpenSSH (when the Android repo use case clearly shows that assumption
also doesn't hold).

Based on that, I'm not sure if you _actually_ need to change anything,
and I apologize for not turning up that we were setting GIT_SSH before
I bothered you.

Thanks immensely for looking into it, Jonathan, and my apologies again
for wasting your time!
Bryan Turner

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi,

A few more notes.

Bryan Turner wrote:

> bturner@ubuntu:~$ ssh -V
> OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014
>
> bturner@ubuntu:~$ ssh -G -p 7999 localhost
> unknown option -- G
> usage: ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]
[...]
> Is it possible to adjust the check, somehow, so it doesn't impact
> older OpenSSH versions like this? As it stands, it seems likely a fair
> number of users who have an SSH command that does support -4, -6 and
> -p are going to end up getting "penalized" because it doesn't also
> support -G, and have to manually set their SSH variant to "ssh" (or
> something other than "auto") to avoid the automatic detection.
>
> I'd love to say I have a brilliant idea for how to work around this,
> oh and here's a patch, but I don't. One option might be trying to
> actually review the output, and another might be to run "ssh -V", but
> both of those have their own flaws (and the extra process forks aren't
> "free").

I have tomorrow off, so I've filed https://crbug.com/git/7 to make
sure I remember to follow up the day after.  Of course I'll be happy
if someone updates that bug saying they've fixed it in the meantime.

One possibility would be to use -V as a fallback when -G fails, or
even as a replacement for this usage of -G.  To avoid misdetecting
PuTTY and other ssh variants that also implement -V as OpenSSH, we
would have to parse the output.  This would also misdetect a script
that does

host=$1; shift
ssh "$host" -- "$@"

as supporting OpenSSH options, when the use of -- ensures it doesn't.

Another possibility is to parse the output when -G fails.  That's
hacky, but I think it would work well!  We would not have to be too
clever, since we can look for the exact output produced by the
versions of OpenSSH that we care about.  This still has issues with
scripts that forward arguments to OpenSSH, but at least those issues
would go away once the user updates their copy of ssh. ;-)

Another possibility is to pass options *before* -V:

ssh -p 7999 -V

Since OpenSSH parses its arguments left-to-right, this gives similar
information to what we did with -G, and scripts like

host=$1; shift
ssh "$host" -- "$@"

would even be correctly detected as not supporting OpenSSH options.
We still would need to parse the output to distinguish OpenSSH from
other ssh implementations like putty (unlike OpenSSH, putty saves up
argument errors in an 'error' variable and forgets about them once it
sees -V).

Trying -G and falling back to -V seems like the simplest detection
mechanism to me at the moment.  I'm hoping I'm missing something
simple (another ssh option?) that allows avoiding this mess.

Regardless, I think we should do something like [1] first to get rid
of the regression.  Thanks again for reporting it.

Sincerely,
Jonathan

[1] 
https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com/

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi Bryan,

Bryan Turner wrote:

> Our test environment is still on Ubuntu 12.04 LTS (it's a long story,
> but one I doubt is unique to us), which means it's using OpenSSH 5.9.
> ssh -G was added in OpenSSH 6.8 [1], circa March 2015, which means the
> "auto" detection "fails" and chooses "simple" instead of "ssh". But
> OpenSSH 5.9 _does_ support -4, -6 and -p. As a result, commands which
> have been working without issue on all previous versions of Git start
> to fail saying
>
> git -c gc.auto=0 -c credential.helper= fetch --force --prune --progress 
> ssh://localhost:64281/repo.git +refs/*:refs/*' exited with code 128 saying: 
> fatal: ssh variant 'simple' does not support setting port

Hm, that's not expected.  git-config(1) says:

By default, Git determines the command line arguments to use
based on the basename of the configured SSH command
(configured using the environment variable GIT_SSH or
GIT_SSH_COMMAND or the config setting core.sshCommand). If the
basename is unrecognized, Git will attempt to detect support
of OpenSSH options by [...]

So my first question is why the basename detection is not working for
you.  What value of GIT_SSH, GIT_SSH_COMMAND, or core.sshCommand are
you using?

> I know Ubuntu 12.04 LTS is end-of-life, but 14.04 LTS, which is
> running OpenSSH 6.6 [2], has the same issue. The following is from a
> fully patched 14.04.5:
>
> bturner@ubuntu:~$ cat /etc/*ease | head -4
> DISTRIB_ID=Ubuntu
> DISTRIB_RELEASE=14.04
> DISTRIB_CODENAME=trusty
> DISTRIB_DESCRIPTION="Ubuntu 14.04.5 LTS"
>
> bturner@ubuntu:~$ ssh -V
> OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014
>
> bturner@ubuntu:~$ ssh -G -p 7999 localhost
> unknown option -- G

It's good you caught this flaw in the detection.  Would something like
the following make sense?  If so, I can resend with a commit message
and tests tomorrow or the day after.

diff --git i/Documentation/config.txt w/Documentation/config.txt
index 64c1dbba94..75eafd8db6 100644
--- i/Documentation/config.txt
+++ w/Documentation/config.txt
@@ -2118,8 +2118,8 @@ ssh.variant::
unrecognized, Git will attempt to detect support of OpenSSH
options by first invoking the configured SSH command with the
`-G` (print configuration) option and will subsequently use
-   OpenSSH options (if that is successful) or no options besides
-   the host and remote command (if it fails).
+   OpenSSH options if that is successful or a conservative set of
+   OpenSSH-style options if it fails.
 +
 The config variable `ssh.variant` can be set to override this detection.
 Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
diff --git i/connect.c w/connect.c
index c3a014c5ba..3784c2be53 100644
--- i/connect.c
+++ w/connect.c
@@ -941,10 +941,9 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
 
if (flags & CONNECT_IPV4) {
switch (variant) {
-   case VARIANT_AUTO:
-   BUG("VARIANT_AUTO passed to push_ssh_options");
case VARIANT_SIMPLE:
die("ssh variant 'simple' does not support -4");
+   case VARIANT_AUTO:
case VARIANT_SSH:
case VARIANT_PLINK:
case VARIANT_PUTTY:
@@ -953,10 +952,9 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
}
} else if (flags & CONNECT_IPV6) {
switch (variant) {
-   case VARIANT_AUTO:
-   BUG("VARIANT_AUTO passed to push_ssh_options");
case VARIANT_SIMPLE:
die("ssh variant 'simple' does not support -6");
+   case VARIANT_AUTO:
case VARIANT_SSH:
case VARIANT_PLINK:
case VARIANT_PUTTY:
@@ -970,10 +968,9 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
 
if (port) {
switch (variant) {
-   case VARIANT_AUTO:
-   BUG("VARIANT_AUTO passed to push_ssh_options");
case VARIANT_SIMPLE:
die("ssh variant 'simple' does not support setting 
port");
+   case VARIANT_AUTO:
case VARIANT_SSH:
argv_array_push(args, "-p");
break;
@@ -1026,7 +1023,7 @@ static void fill_ssh_args(struct child_process *conn, 
const char *ssh_host,
 VARIANT_SSH, port, flags);
argv_array_push(, ssh_host);
 
-   variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;
+   variant = run_command() ? VARIANT_AUTO : VARIANT_SSH;
}
 
argv_array_push(>args, ssh);
diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh
index 0f895478f0..0224edc85b 100755
--- i/t/t5601-clone.sh
+++ w/t/t5601-clone.sh
@@ -365,6 +365,11 @@ 

Re: [PATCH v5 04/34] directory rename detection: basic testcases

[Elijah Newren]

On Tue, Jan 2, 2018 at 5:18 PM, SZEDER Gábor  wrote:
>
>> ---
>>  t/t6043-merge-rename-directories.sh | 430 
>> 
>
> Many of the new tests added in this patch series perform a lot of checks
> running 'test', which has the drawback to fail quietly, leaving us no
> clue about which one of the several conditions failed and why.  We have
> several test helper functions that check the same but provide a useful
> error message if the condition were to fail.
> Here are a couple of suggestions in one of the tests, but they are
> applicable to many other tests in this patch series.

Cool, thanks for taking a look and for the suggestions.  It's good to
know of those other test functions; I'll incorporate them into the
testcase patches in the series.

Re: [ANNOUNCE] Git v2.16.0-rc0

[Bryan Turner]

On Thu, Dec 28, 2017 at 8:30 PM, Junio C Hamano  wrote:
> An early preview release Git v2.16.0-rc0 is now available for
> testing at the usual places.  It is comprised of 435 non-merge
> commits since v2.15.0, contributed by 76 people, 22 of which are
> new faces.

> Brandon Williams (24):
>   ssh: introduce a 'simple' ssh variant

> Jonathan Nieder (10):
>   ssh test: make copy_ssh_wrapper_as clean up after itself
>   connect: move no_fork fallback to git_tcp_connect
>   connect: split git:// setup into a separate function
>   connect: split ssh command line options into separate function
>   connect: split ssh option computation to its own function
>   ssh: 'auto' variant to select between 'ssh' and 'simple'
>   ssh: 'simple' variant does not support -4/-6
>   ssh: 'simple' variant does not support --port
>   connect: correct style of C-style comment
>   generate-cmdlist: avoid non-deterministic output

Sorry for being late to the party on the "simple" variant for SSH, but
we've been doing some testing with 2.16.0-rc0 and noticed an
unexpected issue.

Our test environment is still on Ubuntu 12.04 LTS (it's a long story,
but one I doubt is unique to us), which means it's using OpenSSH 5.9.
ssh -G was added in OpenSSH 6.8 [1], circa March 2015, which means the
"auto" detection "fails" and chooses "simple" instead of "ssh". But
OpenSSH 5.9 _does_ support -4, -6 and -p. As a result, commands which
have been working without issue on all previous versions of Git start
to fail saying:

git -c gc.auto=0 -c credential.helper= fetch --force --prune
--progress ssh://localhost:64281/repo.git +refs/*:refs/*' exited with
code 128 saying: fatal: ssh variant 'simple' does not support setting
port

I know Ubuntu 12.04 LTS is end-of-life, but 14.04 LTS, which is
running OpenSSH 6.6 [2], has the same issue. The following is from a
fully patched 14.04.5:

bturner@ubuntu:~$ cat /etc/*ease | head -4
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=14.04
DISTRIB_CODENAME=trusty
DISTRIB_DESCRIPTION="Ubuntu 14.04.5 LTS"

bturner@ubuntu:~$ ssh -V
OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014

bturner@ubuntu:~$ ssh -G -p 7999 localhost
unknown option -- G
usage: ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]
   [-D [bind_address:]port] [-E log_file] [-e escape_char]
   [-F configfile] [-I pkcs11] [-i identity_file]
   [-L [bind_address:]port:host:hostport] [-l login_name] [-m mac_spec]
   [-O ctl_cmd] [-o option] [-p port]
   [-Q cipher | cipher-auth | mac | kex | key]
   [-R [bind_address:]port:host:hostport] [-S ctl_path] [-W host:port]
   [-w local_tun[:remote_tun]] [user@]hostname [command]

Is it possible to adjust the check, somehow, so it doesn't impact
older OpenSSH versions like this? As it stands, it seems likely a fair
number of users who have an SSH command that does support -4, -6 and
-p are going to end up getting "penalized" because it doesn't also
support -G, and have to manually set their SSH variant to "ssh" (or
something other than "auto") to avoid the automatic detection.

I'd love to say I have a brilliant idea for how to work around this,
oh and here's a patch, but I don't. One option might be trying to
actually review the output, and another might be to run "ssh -V", but
both of those have their own flaws (and the extra process forks aren't
"free").

[1] https://www.openssh.com/txt/release-6.8
[2] https://launchpad.net/ubuntu/+source/openssh

Best regards,
Bryan Turner

Re: [PATCH v5 04/34] directory rename detection: basic testcases

[SZEDER Gábor]

> ---
>  t/t6043-merge-rename-directories.sh | 430 
> 

Many of the new tests added in this patch series perform a lot of checks
running 'test', which has the drawback to fail quietly, leaving us no
clue about which one of the several conditions failed and why.  We have
several test helper functions that check the same but provide a useful
error message if the condition were to fail.
Here are a couple of suggestions in one of the tests, but they are
applicable to many other tests in this patch series.

> +test_expect_failure '1d-check: Directory renames cause a rename/rename(2to1) 
> conflict' '
> + (
> + cd 1d &&
> +
> + git checkout A^0 &&
> +
> + test_must_fail git merge -s recursive B^0 >out &&
> + test_i18ngrep "CONFLICT (rename/rename)" out &&
> +
> + test 8 -eq $(git ls-files -s | wc -l) &&
> + test 2 -eq $(git ls-files -u | wc -l) &&
> + test 3 -eq $(git ls-files -o | wc -l) &&

  git ls-files -s >out &&
  test_line_count = 8 out &&
  git ls-files -u >out &&
  test_line_count = 2 out &&
  git ls-files -o >out &&
  test_line_count = 3 out &&

> +
> + git rev-parse >actual \
> + :0:x/b :0:x/c :0:x/d :0:x/e :0:x/m :0:x/n &&
> + git rev-parse >expect \
> + O:z/b O:z/c O:y/d O:y/e A:y/m B:z/n &&
> + test_cmp expect actual &&
> +
> + test_must_fail git rev-parse :0:x/wham &&
> + git rev-parse >actual \
> + :2:x/wham :3:x/wham &&
> + git rev-parse >expect \
> + A:y/wham B:z/wham &&
> + test_cmp expect actual &&
> +
> + test ! -f x/wham &&

This one checks that the given path is not a file, therefore it would
succeed if that path were a directory or a FIFO or whatnot.  I believe
you want to check that the path doesn't exist:

  test_path_is_missing x/wham &&

> + test -f x/wham~HEAD &&
> + test -f x/wham~B^0 &&

  test_path_is_file x/wham~HEAD &&
  test_path_is_file x/wham~B^0 &&


> +
> + git hash-object >actual \
> + x/wham~HEAD x/wham~B^0 &&
> + git rev-parse >expect \
> + A:y/wham B:z/wham &&
> + test_cmp expect actual
> + )
> +'

[PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation

[Stefan Beller]

I was compiling origin/master today with stricter compiler flags today
and was greeted by

t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
 printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
 ^~~
 nr,
 ~~~
 (double)avg_single/10,
 ~~
 (avg_single < avg_multi ? '<' : '>'),
 ~
 (double)avg_multi/10,
 ~
 nr_threads_used);
 
t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared 
here
  int nr_threads_used;
  ^~~

I do not see how we can arrive at that line without having `nr_threads_used`
initialized, as we'd have `count > 1`  (which asserts that we ran the
loop above at least once, such that it *should* be initialized).

I do not have time to dive into further analysis.

Signed-off-by: Stefan Beller 
---
 t/helper/test-lazy-init-name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-lazy-init-name-hash.c 
b/t/helper/test-lazy-init-name-hash.c
index 6368a89345..297fb01d61 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -112,7 +112,7 @@ static void analyze_run(void)
 {
uint64_t t1s, t1m, t2s, t2m;
int cache_nr_limit;
-   int nr_threads_used;
+   int nr_threads_used = 0;
int i;
int nr;
 
-- 
2.15.1.620.gb9897f4670-goog

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

[Stefan Beller]

When using hard reset or forced checkout with the option to recurse into
submodules, the submodules need to be reset, too.

It turns out that we need to omit the duplicate old argument to read-tree
in all forced cases to omit the 2 way merge and use the more assertive
behavior of reading the specific new tree into the index and updating
the working tree.

Signed-off-by: Stefan Beller 
---
 submodule.c   |  4 +++-
 t/lib-submodule-update.sh | 14 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index fa25888783..db0f7ac51e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,
else
argv_array_push(, "-m");
 
-   argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
+   if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
+   argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
+
argv_array_push(, new ? new : EMPTY_TREE_SHA1_HEX);
 
if (run_command()) {
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index fb0173ea87..1f38a85371 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1015,4 +1015,18 @@ test_submodule_forced_switch_recursing_with_args () {
test_submodule_content sub1 origin/modify_sub1
)
'
+
+   test_expect_success "$command: changed submodule worktree is reset" '
+   prolog &&
+   reset_work_tree_to_interested add_sub1 &&
+   (
+   cd submodule_update &&
+   rm sub1/file1 &&
+   : >sub1/new_file &&
+   git -C sub1 add new_file &&
+   $command HEAD &&
+   test_path_is_file sub1/file1 &&
+   test_path_is_missing sub1/new_file
+   )
+   '
 }
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller 
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..7657d6ecdd 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
ie_match_stat(o->src_index, old, , 
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
update |= CE_UPDATE;
}
+   if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
+   o->update && !verify_uptodate(old, o))
+   update |= CE_UPDATE;
add_entry(o, old, update, 0);
return 0;
}
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 2/5] t/lib-submodule-update.sh: clarify test

[Stefan Beller]

Keep the local branch name as the upstream branch name to avoid confusion.

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 38dadd2c29..d7699046f6 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() {
cd submodule_update &&
git -C sub1 checkout -b keep_branch &&
git -C sub1 rev-parse HEAD >expect &&
-   git branch -t check-keep origin/modify_sub1 &&
-   $command check-keep &&
+   git branch -t modify_sub1 origin/modify_sub1 &&
+   $command modify_sub1 &&
test_superproject_content origin/modify_sub1 &&
test_submodule_content sub1 origin/modify_sub1 &&
git -C sub1 rev-parse keep_branch >actual &&
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules

[Stefan Beller]

It turns out that the test replacing a submodule with a file with
the submodule containing an ignored file is incorrectly titled,
because the test put the file in place, but never ignored that file.
When having an untracked file Instead of an ignored file in the
submodule, git should refuse to remove the submodule, but that is
a bug in the implementation of recursing into submodules, such that
the test just passed, removing the untracked file.

Fix the test first; in a later patch we'll fix gits behavior,
that will make sure untracked files are not deleted.

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index d7699046f6..fb0173ea87 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () {
(
cd submodule_update &&
git branch -t replace_sub1_with_file 
origin/replace_sub1_with_file &&
+   echo ignored >.git/modules/sub1/info/exclude &&
: >sub1/ignored &&
$command replace_sub1_with_file &&
test_superproject_content origin/replace_sub1_with_file 
&&
-- 
2.15.1.620.gb9897f4670-goog

[PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes

[Stefan Beller]

Thanks Junio for review of this series!
The only change in this version of the series is

--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
update |= CE_UPDATE;
}
if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
-   !verify_uptodate(old, o))
+   o->update && !verify_uptodate(old, o))
update |= CE_UPDATE;
add_entry(o, old, update, 0);


v2:
I dropped the patch to `same()` as I realized we only need to fix the
oneway_merge function, the others (two, three way merge) are fine as
they have the checks already in place.

The test added in the last patch got slightly larger as now we also test for
newly staged files to be blown away in the submodule.

v1:

The fix is in the last patch, the first patches are just massaging the code
base to make the fix easy.

The second patch fixes a bug in the test, which was ineffective at testing.
The third patch shows the problem this series addresses,
the fourth patch is a little refactoring, which I want to keep separate
as I would expect it to be a performance regression[1].
The first patch is unrelated, but improves the readability of submodule test
cases, which we'd want to improve further.

Thanks,
Stefan


Stefan Beller (5):
  t/helper/test-lazy-name-hash: fix compilation
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
submodules
  unpack-trees: oneway_merge to update submodules
  submodule: submodule_move_head omits old argument in forced case

 submodule.c |  4 +++-
 t/helper/test-lazy-init-name-hash.c |  2 +-
 t/lib-submodule-update.sh   | 19 +--
 unpack-trees.c  |  3 +++
 4 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH 5/5] diff: properly error out when combining multiple pickaxe options

[Jacob Keller]

On Tue, Jan 2, 2018 at 4:46 PM, Stefan Beller  wrote:
> die(_("--name-only, --name-status, --check and -s are 
> mutually exclusive"));
> +   count = 0;
> +
> +   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
> +   count++;
> +   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
> +   count++;
> +   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
> +   count++;
> +   if (count > 1)
> +   die(_("-G, -S, --find-object are mutually exclusive"));

Nit: I'd put an "and" in the sentence.

[PATCHv2 4/5] diffcore: add a pickaxe option to find a specific blob

[Stefan Beller]

Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])

One might be tempted to extend git-describe to also work with blobs,
such that `git describe ` gives a description as
':'.  This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.

Junio hinted at a different approach of solving this problem, which this
patch implements. Teach the diff machinery another flag for restricting
the information to what is shown. For example:

$ ./git log --oneline --find-object=v2.0.0:Makefile
b2feb64309 Revert the whole "ask curl-config" topic for now
47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

we observe that the Makefile as shipped with 2.0 was appeared in
v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.  The
reason why these commits both occur prior to v2.0.0 are evil
merges that are not found using this new mechanism.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbel...@google.com/

Signed-off-by: Stefan Beller 
---

We lost the tests in the first version, this includes the new file containing
the tests.

Stefan

 Documentation/diff-options.txt | 10 +++
 diff.c | 21 -
 diff.h |  8 -
 diffcore-pickaxe.c | 45 +---
 revision.c |  3 ++
 t/t4064-diff-oidfind.sh| 68 ++
 6 files changed, 135 insertions(+), 20 deletions(-)
 create mode 100755 t/t4064-diff-oidfind.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..f9cf85db05 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -492,6 +492,15 @@ occurrences of that string did not change).
 See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
 information.
 
+--find-object=::
+   Look for differences that change the number of occurrences of
+   the specified object. Similar to `-S`, just the argument is different
+   in that it doesn't search for a specific string but for a specific
+   object id.
++
+The object can be a blob or a submodule commit. It implies the `-t` option in
+`git-log` to also find trees.
+
 --pickaxe-all::
When `-S` or `-G` finds a change, show all the changes in that
changeset, not just the files that contain the change
@@ -500,6 +509,7 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
 endif::git-format-patch[]
 
 -O::
diff --git a/diff.c b/diff.c
index 5508745dc8..a872bdcac1 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+   options->objfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,23 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+{
+   struct object_id oid;
+
+   if (get_oid(arg, ))
+   return error("unable to resolve '%s'", arg);
+
+   if (!opt->objfind)
+   opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+
+   opt->pickaxe_opts |= DIFF_PICKAXE_KIND_OBJFIND;
+   opt->flags.recursive = 1;
+   opt->flags.tree_in_recursive = 1;
+   oidset_insert(opt->objfind, );
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4736,7 +4754,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   }
+   } else if (skip_prefix(arg, "--find-object=", ))
+   return parse_objfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
diff --git a/diff.h b/diff.h
index 9ec4f824fe..8a56cac2ad 100644
--- a/diff.h
+++ b/diff.h
@@ -7,6 +7,7 @@
 #include "tree-walk.h"
 #include "pathspec.h"
 #include "object.h"
+#include "oidset.h"
 
 struct rev_info;
 struct diff_options;
@@ -173,6 +174,8 @@ struct diff_options {
enum 

[PATCH 4/5] diffcore: add a pickaxe option to find a specific blob

[Stefan Beller]

Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])

One might be tempted to extend git-describe to also work with blobs,
such that `git describe ` gives a description as
':'.  This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.

Junio hinted at a different approach of solving this problem, which this
patch implements. Teach the diff machinery another flag for restricting
the information to what is shown. For example:

$ ./git log --oneline --find-object=v2.0.0:Makefile
b2feb64309 Revert the whole "ask curl-config" topic for now
47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

we observe that the Makefile as shipped with 2.0 was appeared in
v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.  The
reason why these commits both occur prior to v2.0.0 are evil
merges that are not found using this new mechanism.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbel...@google.com/

Signed-off-by: Stefan Beller 
---
 Documentation/diff-options.txt | 10 ++
 diff.c | 21 +++-
 diff.h |  8 +++-
 diffcore-pickaxe.c | 45 +-
 revision.c |  3 +++
 5 files changed, 67 insertions(+), 20 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..f9cf85db05 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -492,6 +492,15 @@ occurrences of that string did not change).
 See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
 information.
 
+--find-object=::
+   Look for differences that change the number of occurrences of
+   the specified object. Similar to `-S`, just the argument is different
+   in that it doesn't search for a specific string but for a specific
+   object id.
++
+The object can be a blob or a submodule commit. It implies the `-t` option in
+`git-log` to also find trees.
+
 --pickaxe-all::
When `-S` or `-G` finds a change, show all the changes in that
changeset, not just the files that contain the change
@@ -500,6 +509,7 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
 endif::git-format-patch[]
 
 -O::
diff --git a/diff.c b/diff.c
index 5508745dc8..a872bdcac1 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+   options->objfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,23 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+{
+   struct object_id oid;
+
+   if (get_oid(arg, ))
+   return error("unable to resolve '%s'", arg);
+
+   if (!opt->objfind)
+   opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+
+   opt->pickaxe_opts |= DIFF_PICKAXE_KIND_OBJFIND;
+   opt->flags.recursive = 1;
+   opt->flags.tree_in_recursive = 1;
+   oidset_insert(opt->objfind, );
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4736,7 +4754,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   }
+   } else if (skip_prefix(arg, "--find-object=", ))
+   return parse_objfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
diff --git a/diff.h b/diff.h
index 9ec4f824fe..8a56cac2ad 100644
--- a/diff.h
+++ b/diff.h
@@ -7,6 +7,7 @@
 #include "tree-walk.h"
 #include "pathspec.h"
 #include "object.h"
+#include "oidset.h"
 
 struct rev_info;
 struct diff_options;
@@ -173,6 +174,8 @@ struct diff_options {
enum diff_words_type word_diff;
enum diff_submodule_format submodule_format;
 
+   struct oidset *objfind;
+
/* this is set by diffcore for DIFF_FORMAT_PATCH */
int 

[PATCH 2/5] diff: migrate diff_flags.pickaxe_ignore_case to a pickaxe_opts bit

[Stefan Beller]

Currently flags for pickaxing are found in different places. Unify the
flags into the `pickaxe_opts` field, which will contain any pickaxe related
flags.

Signed-off-by: Stefan Beller 
---
 diff.h | 3 ++-
 diffcore-pickaxe.c | 6 +++---
 revision.c | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/diff.h b/diff.h
index ea310f76fd..8af1213684 100644
--- a/diff.h
+++ b/diff.h
@@ -91,7 +91,6 @@ struct diff_flags {
unsigned override_submodule_config:1;
unsigned dirstat_by_line:1;
unsigned funccontext:1;
-   unsigned pickaxe_ignore_case:1;
unsigned default_follow_renames:1;
 };
 
@@ -327,6 +326,8 @@ extern void diff_setup_done(struct diff_options *);
 #define DIFF_PICKAXE_KIND_S4 /* traditional plumbing counter */
 #define DIFF_PICKAXE_KIND_G8 /* grep in the patch */
 
+#define DIFF_PICKAXE_IGNORE_CASE   32
+
 extern void diffcore_std(struct diff_options *);
 extern void diffcore_fix_diff_index(struct diff_options *);
 
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 9476bd2108..4b5d88ea46 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -222,11 +222,11 @@ void diffcore_pickaxe(struct diff_options *o)
 
if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
int cflags = REG_EXTENDED | REG_NEWLINE;
-   if (o->flags.pickaxe_ignore_case)
+   if (o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE)
cflags |= REG_ICASE;
regcomp_or_die(, needle, cflags);
regexp = 
-   } else if (o->flags.pickaxe_ignore_case &&
+   } else if (o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE &&
   has_non_ascii(needle)) {
struct strbuf sb = STRBUF_INIT;
int cflags = REG_NEWLINE | REG_ICASE;
@@ -236,7 +236,7 @@ void diffcore_pickaxe(struct diff_options *o)
strbuf_release();
regexp = 
} else {
-   kws = kwsalloc(o->flags.pickaxe_ignore_case
+   kws = kwsalloc(o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE
   ? tolower_trans_tbl : NULL);
kwsincr(kws, needle, strlen(needle));
kwsprep(kws);
diff --git a/revision.c b/revision.c
index e2e691dd5a..ccf1d212ce 100644
--- a/revision.c
+++ b/revision.c
@@ -2076,7 +2076,7 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;
} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
revs->grep_filter.ignore_case = 1;
-   revs->diffopt.flags.pickaxe_ignore_case = 1;
+   revs->diffopt.pickaxe_opts |= DIFF_PICKAXE_IGNORE_CASE;
} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;
} else if (!strcmp(arg, "--perl-regexp") || !strcmp(arg, "-P")) {
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 3/5] diff: introduce DIFF_PICKAXE_KINDS_MASK

[Stefan Beller]

Currently the check whether to perform pickaxing is done via checking
`diffopt->pickaxe`, which contains the command line argument that we
want to pickaxe for. Soon we'll introduce a new type of pickaxing, that
will not store anything in the `.pickaxe` field, so let's migrate the
check to be dependent on pickaxe_opts.

It is not enough to just replace the check for pickaxe by pickaxe_opts,
because flags might be set, but pickaxing was not requested ('-i').
To cope with that, introduce a mask to check only for the bits indicating
the modes of operation.

Signed-off-by: Stefan Beller 
---
 builtin/log.c  | 4 ++--
 combine-diff.c | 2 +-
 diff.c | 4 ++--
 diff.h | 2 ++
 revision.c | 2 +-
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896ad..bd6f2d1efb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -180,8 +180,8 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
if (rev->show_notes)
init_display_notes(>notes_opt);
 
-   if (rev->diffopt.pickaxe || rev->diffopt.filter ||
-   rev->diffopt.flags.follow_renames)
+   if ((rev->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) ||
+   rev->diffopt.filter || rev->diffopt.flags.follow_renames)
rev->always_show_header = 0;
 
if (source)
diff --git a/combine-diff.c b/combine-diff.c
index 2505de119a..bc08c4c5b1 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1438,7 +1438,7 @@ void diff_tree_combined(const struct object_id *oid,
opt->flags.follow_renames   ||
opt->break_opt != -1||
opt->detect_rename  ||
-   opt->pickaxe||
+   (opt->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)   ||
opt->filter;
 
 
diff --git a/diff.c b/diff.c
index 0763e89263..5508745dc8 100644
--- a/diff.c
+++ b/diff.c
@@ -4173,7 +4173,7 @@ void diff_setup_done(struct diff_options *options)
/*
 * Also pickaxe would not work very well if you do not say recursive
 */
-   if (options->pickaxe)
+   if (options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)
options->flags.recursive = 1;
/*
 * When patches are generated, submodules diffed against the work tree
@@ -5777,7 +5777,7 @@ void diffcore_std(struct diff_options *options)
if (options->break_opt != -1)
diffcore_merge_broken();
}
-   if (options->pickaxe)
+   if (options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)
diffcore_pickaxe(options);
if (options->orderfile)
diffcore_order(options->orderfile);
diff --git a/diff.h b/diff.h
index 8af1213684..9ec4f824fe 100644
--- a/diff.h
+++ b/diff.h
@@ -326,6 +326,8 @@ extern void diff_setup_done(struct diff_options *);
 #define DIFF_PICKAXE_KIND_S4 /* traditional plumbing counter */
 #define DIFF_PICKAXE_KIND_G8 /* grep in the patch */
 
+#define DIFF_PICKAXE_KINDS_MASK (DIFF_PICKAXE_KIND_S | DIFF_PICKAXE_KIND_G)
+
 #define DIFF_PICKAXE_IGNORE_CASE   32
 
 extern void diffcore_std(struct diff_options *);
diff --git a/revision.c b/revision.c
index ccf1d212ce..5d11ecaf27 100644
--- a/revision.c
+++ b/revision.c
@@ -2407,7 +2407,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
revs->diff = 1;
 
/* Pickaxe, diff-filter and rename following need diffs */
-   if (revs->diffopt.pickaxe ||
+   if ((revs->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) ||
revs->diffopt.filter ||
revs->diffopt.flags.follow_renames)
revs->diff = 1;
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 5/5] diff: properly error out when combining multiple pickaxe options

[Stefan Beller]

In f506b8e8b5 (git log/diff: add -G that greps in the patch text,
2010-08-23) we were hesitant to check if the user requests both -S and
-G at the same time. Now that the pickaxe family also offers --find-object,
which looks slightly more different than the former two, let's add a check
that those are not used at the same time.

Signed-off-by: Stefan Beller 
---
 diff.c | 10 ++
 diffcore-pickaxe.c |  1 -
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index a872bdcac1..29973e78d6 100644
--- a/diff.c
+++ b/diff.c
@@ -4122,6 +4122,16 @@ void diff_setup_done(struct diff_options *options)
count++;
if (count > 1)
die(_("--name-only, --name-status, --check and -s are mutually 
exclusive"));
+   count = 0;
+
+   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
+   count++;
+   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
+   count++;
+   if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
+   count++;
+   if (count > 1)
+   die(_("-G, -S, --find-object are mutually exclusive"));
 
/*
 * Most of the time we can say "there are changes"
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 72bb5a9514..239ce5122b 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -251,7 +251,6 @@ void diffcore_pickaxe(struct diff_options *o)
}
}
 
-   /* Might want to warn when both S and G are on; I don't care... */
pickaxe(_queued_diff, o, regexp, kws,
(opts & DIFF_PICKAXE_KIND_G) ? diff_grep : has_changes);
 
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 1/5] diff.h: Make pickaxe_opts an unsigned bit field

[Stefan Beller]

This variable is used as a bit field[1], and as we are about to add more
fields, indicate its usage as a bit field by making it unsigned.

[1] containing the bits

#define DIFF_PICKAXE_ALL1
#define DIFF_PICKAXE_REGEX  2
#define DIFF_PICKAXE_KIND_S 4
#define DIFF_PICKAXE_KIND_G 8

Signed-off-by: Stefan Beller 
---
 diff.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.h b/diff.h
index 0fb18dd735..ea310f76fd 100644
--- a/diff.h
+++ b/diff.h
@@ -146,7 +146,7 @@ struct diff_options {
int skip_stat_unmatch;
int line_termination;
int output_format;
-   int pickaxe_opts;
+   unsigned pickaxe_opts;
int rename_score;
int rename_limit;
int needed_rename_limit;
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 0/5] pickaxe refactorings and a new mode to find blobs (WAS: diffcore: add a pickaxe option to find a specific blob)

[Stefan Beller]

After some discussion [1], we're convinced that the original approach for
adding in just another pickaxe mode to find blobs was too hacky.

So I went the less hacky way and did some refactoring first (patches 1-3),
Then we'll have the new pickaxe mode to find blobs in patch 4. It grew
slightly larger as it had issues with the setup (we neither have a regex
nor a KWS to init) in this new world, so there are a few more lines in there.

The last patch is just the cherry on the cake, helping to keep users sane by
warning when they try to use different pickaxe modes at the same time.

Thanks,
Stefan


[1] 
https://public-inbox.org/git/cagz79kab0g9zetf6qtc45+zglm3gosywv7e+gkce2ykohb0...@mail.gmail.com/


Stefan Beller (5):
  diff.h: Make pickaxe_opts an unsigned bit field
  diff: migrate diff_flags.pickaxe_ignore_case to a pickaxe_opts bit
  diff: introduce DIFF_PICKAXE_KINDS_MASK
  diffcore: add a pickaxe option to find a specific blob
  diff: properly error out when combining multiple pickaxe options

 Documentation/diff-options.txt | 10 +
 builtin/log.c  |  4 ++--
 combine-diff.c |  2 +-
 diff.c | 35 +++---
 diff.h | 13 ++--
 diffcore-pickaxe.c | 48 --
 revision.c |  7 --
 7 files changed, 89 insertions(+), 30 deletions(-)

-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Todd Zullinger]

Brandon Williams wrote:
> Seems good to me.  I think I was just being overly cautious when i was
> implementing those patches.  Thanks!

Cool, thanks for taking a look.

In this case, the test isn't dependent on apache > 2.4, but
before I checked that I wondered if we had any way to run a
test only if the apache version was greater or lesser than
some release.  Luckily, I didn't have to work out such a
method. :)

I don't know if there's a clean way to do that
automatically, short of parsing the output of 'httpd -v'
should we ever need to add such a prereq.

-- 
Todd
~~
Sometimes you get the blues because your baby leaves you. Sometimes
you get'em 'cause she comes back.
-- B.B. King

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Brandon Williams]

On 12/30, Todd Zullinger wrote:
> The apache config used by tests was updated to use the SetEnvIf
> directive to set the Git-Protocol header in 19113a26b6 ("http: tell
> server that the client understands v1", 2017-10-16).
> 
> Setting the Git-Protocol header is restricted to httpd >= 2.4, but
> mod_setenvif and the SetEnvIf directive work with lower versions, at
> least as far back as 2.0, according to the httpd documentation:
> 
> https://httpd.apache.org/docs/2.0/mod/mod_setenvif.html
> 
> Drop the restriction.  Tested with httpd 2.2 and 2.4.
> 
> Signed-off-by: Todd Zullinger 
> ---
> 
> These tests fail with 2.16.0-rc0 on CentOS-6, which uses
> httpd-2.2.
> 
> I removed the version restriction entirely rather than adjust
> the version.  I believe SetEnvIf works on httpd >= 2.0.  I'm
> not sure if we aim to support anything less than httpd 2.0,
> but I'm betting not.  If that's incorrect, I can add some
> IfVersion conditions.
> 
> As noted in the commit message, I only tested with httpd 2.2
> and 2.4 (on CentOS 6/7 and Fedora 26-28).  If anyone has older
> httpd systems which need support, it would be great if they
> could test this before 2.16.0 is finalized, so we can avoid
> shipping with any failing tests.

Seems good to me.  I think I was just being overly cautious when i was
implementing those patches.  Thanks!

> 
>  t/lib-httpd/apache.conf | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index df19436314..724d9ae462 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -25,6 +25,9 @@ ErrorLog error.log
>  
>   LoadModule headers_module modules/mod_headers.so
>  
> +
> + LoadModule setenvif_module modules/mod_setenvif.so
> +
>  
>  
>  LockFile accept.lock
> @@ -67,9 +70,6 @@ LockFile accept.lock
>  
>   LoadModule unixd_module modules/mod_unixd.so
>  
> -
> - LoadModule setenvif_module modules/mod_setenvif.so
> -
>  
>  
>  PassEnv GIT_VALGRIND
> @@ -79,9 +79,7 @@ PassEnv ASAN_OPTIONS
>  PassEnv GIT_TRACE
>  PassEnv GIT_CONFIG_NOSYSTEM
>  
> -= 2.4>
> - SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
> -
> +SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
>  
>  Alias /dumb/ www/
>  Alias /auth/dumb/ www/auth/dumb/
> -- 
> 2.16.0.rc0
> 

-- 
Brandon Williams

[PATCH 10/26] protocol: introduce enum protocol_version value protocol_v2

[Brandon Williams]

Introduce protocol_v2, a new value for 'enum protocol_version'.
Subsequent patches will fill in the implementation of protocol_v2.

Signed-off-by: Brandon Williams 
---
 builtin/fetch-pack.c   | 3 +++
 builtin/receive-pack.c | 6 ++
 builtin/send-pack.c| 3 +++
 connect.c  | 3 +++
 protocol.c | 2 ++
 protocol.h | 1 +
 remote-curl.c  | 3 +++
 transport.c| 9 +
 upload-pack.c  | 6 ++
 9 files changed, 36 insertions(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 85d4faf76..f492e8abd 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -201,6 +201,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
   PACKET_READ_GENTLE_ON_EOF);
 
switch (discover_version()) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, , 0, NULL, );
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b7ce7c7f5..3656e94fd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1963,6 +1963,12 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
unpack_limit = receive_unpack_limit;
 
switch (determine_protocol_version_server()) {
+   case protocol_v2:
+   /*
+* push support for protocol v2 has not been implemented yet,
+* so ignore the request to use v2 and fallback to using v0.
+*/
+   break;
case protocol_v1:
/*
 * v1 is just the original protocol with a version string,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 83cb125a6..b5427f75e 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -263,6 +263,9 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
   PACKET_READ_GENTLE_ON_EOF);
 
switch (discover_version()) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, _refs, REF_NORMAL,
diff --git a/connect.c b/connect.c
index 1787b0212..caa539b75 100644
--- a/connect.c
+++ b/connect.c
@@ -84,6 +84,9 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
 
/* Maybe process capabilities here, at least for v2 */
switch (version) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
/* Read the peeked version line */
packet_reader_read(reader);
diff --git a/protocol.c b/protocol.c
index 43012b7eb..5e636785d 100644
--- a/protocol.c
+++ b/protocol.c
@@ -8,6 +8,8 @@ static enum protocol_version parse_protocol_version(const char 
*value)
return protocol_v0;
else if (!strcmp(value, "1"))
return protocol_v1;
+   else if (!strcmp(value, "2"))
+   return protocol_v2;
else
return protocol_unknown_version;
 }
diff --git a/protocol.h b/protocol.h
index 1b2bc94a8..2ad35e433 100644
--- a/protocol.h
+++ b/protocol.h
@@ -5,6 +5,7 @@ enum protocol_version {
protocol_unknown_version = -1,
protocol_v0 = 0,
protocol_v1 = 1,
+   protocol_v2 = 2,
 };
 
 /*
diff --git a/remote-curl.c b/remote-curl.c
index 9f6d07683..dae8a4a48 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -185,6 +185,9 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_GENTLE_ON_EOF);
 
switch (discover_version()) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, , for_push ? REF_NORMAL : 0,
diff --git a/transport.c b/transport.c
index 2378dcb38..83d9dd1df 100644
--- a/transport.c
+++ b/transport.c
@@ -203,6 +203,9 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
 
data->version = discover_version();
switch (data->version) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, ,
@@ -250,6 +253,9 @@ static int fetch_refs_via_pack(struct transport *transport,
refs_tmp = get_refs_via_connect(transport, 0);
 
switch (data->version) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
refs = fetch_pack(, data->fd, 

[PATCH 06/26] transport: use get_refs_via_connect to get refs

[Brandon Williams]

Remove code duplication and use the existing 'get_refs_via_connect()'
function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
'git_transport_push()'.

Signed-off-by: Brandon Williams 
---
 transport.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/transport.c b/transport.c
index fc802260f..8e8779096 100644
--- a/transport.c
+++ b/transport.c
@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
args.cloning = transport->cloning;
args.update_shallow = data->options.update_shallow;
 
-   if (!data->got_remote_heads) {
-   connect_setup(transport, 0);
-   get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
-NULL, >shallow);
-   data->got_remote_heads = 1;
-   }
+   if (!data->got_remote_heads)
+   refs_tmp = get_refs_via_connect(transport, 0);
 
refs = fetch_pack(, data->fd, data->conn,
  refs_tmp ? refs_tmp : transport->remote_refs,
@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, 
struct ref *remote_re
struct send_pack_args args;
int ret;
 
-   if (!data->got_remote_heads) {
-   struct ref *tmp_refs;
-   connect_setup(transport, 1);
-
-   get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
-NULL, >shallow);
-   data->got_remote_heads = 1;
-   }
+   if (!data->got_remote_heads)
+   get_refs_via_connect(transport, 1);
 
memset(, 0, sizeof(args));
args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 09/26] transport: store protocol version

[Brandon Williams]

Once protocol_v2 is introduced requesting a fetch or a push will need to
be handled differently depending on the protocol version.  Store the
protocol version the server is speaking in 'struct git_transport_data'
and use it to determine what to do in the case of a fetch or a push.

Signed-off-by: Brandon Williams 
---
 transport.c | 35 ++-
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/transport.c b/transport.c
index 63c3dbab9..2378dcb38 100644
--- a/transport.c
+++ b/transport.c
@@ -118,6 +118,7 @@ struct git_transport_data {
struct child_process *conn;
int fd[2];
unsigned got_remote_heads : 1;
+   enum protocol_version version;
struct oid_array extra_have;
struct oid_array shallow;
 };
@@ -200,7 +201,8 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
   PACKET_READ_CHOMP_NEWLINE |
   PACKET_READ_GENTLE_ON_EOF);
 
-   switch (discover_version()) {
+   data->version = discover_version();
+   switch (data->version) {
case protocol_v1:
case protocol_v0:
get_remote_heads(, ,
@@ -221,7 +223,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 {
int ret = 0;
struct git_transport_data *data = transport->data;
-   struct ref *refs;
+   struct ref *refs = NULL;
char *dest = xstrdup(transport->url);
struct fetch_pack_args args;
struct ref *refs_tmp = NULL;
@@ -247,10 +249,18 @@ static int fetch_refs_via_pack(struct transport 
*transport,
if (!data->got_remote_heads)
refs_tmp = get_refs_via_connect(transport, 0);
 
-   refs = fetch_pack(, data->fd, data->conn,
- refs_tmp ? refs_tmp : transport->remote_refs,
- dest, to_fetch, nr_heads, >shallow,
- >pack_lockfile);
+   switch (data->version) {
+   case protocol_v1:
+   case protocol_v0:
+   refs = fetch_pack(, data->fd, data->conn,
+ refs_tmp ? refs_tmp : transport->remote_refs,
+ dest, to_fetch, nr_heads, >shallow,
+ >pack_lockfile);
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
+
close(data->fd[0]);
close(data->fd[1]);
if (finish_connect(data->conn))
@@ -549,7 +559,7 @@ static int git_transport_push(struct transport *transport, 
struct ref *remote_re
 {
struct git_transport_data *data = transport->data;
struct send_pack_args args;
-   int ret;
+   int ret = 0;
 
if (!data->got_remote_heads)
get_refs_via_connect(transport, 1);
@@ -574,8 +584,15 @@ static int git_transport_push(struct transport *transport, 
struct ref *remote_re
else
args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
 
-   ret = send_pack(, data->fd, data->conn, remote_refs,
-   >extra_have);
+   switch (data->version) {
+   case protocol_v1:
+   case protocol_v0:
+   ret = send_pack(, data->fd, data->conn, remote_refs,
+   >extra_have);
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
 
close(data->fd[1]);
close(data->fd[0]);
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 11/26] serve: introduce git-serve

[Brandon Williams]

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current
wire protocol.  The intention is that it will be a simpler, less
wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial
ref advertisement.  In its place a server will export a list of
capabilities and commands which it supports in a capability
advertisement.  A client can then request that a particular command be
executed by providing a number of capabilities and command specific
parameters.  At the completion of a command, a client can request that
another command be executed or can terminate the connection by sending a
flush packet.

Signed-off-by: Brandon Williams 
---
 .gitignore  |   1 +
 Documentation/technical/protocol-v2.txt |  91 
 Makefile|   2 +
 builtin.h   |   1 +
 builtin/serve.c |  30 
 git.c   |   1 +
 serve.c | 239 
 serve.h |  15 ++
 8 files changed, 380 insertions(+)
 create mode 100644 Documentation/technical/protocol-v2.txt
 create mode 100644 builtin/serve.c
 create mode 100644 serve.c
 create mode 100644 serve.h

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..2d0450c26 100644
--- a/.gitignore
+++ b/.gitignore
@@ -140,6 +140,7 @@
 /git-rm
 /git-send-email
 /git-send-pack
+/git-serve
 /git-sh-i18n
 /git-sh-i18n--envsubst
 /git-sh-setup
diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
new file mode 100644
index 0..b87ba3816
--- /dev/null
+++ b/Documentation/technical/protocol-v2.txt
@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+supported by a single service.
+  * Easily extendable as capabilities are moved into their own section
+of the protocol, no longer being hidden behind a NUL byte and
+limited by the size of a pkt-line (as there will be a single
+capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+string as a capability and symrefs can be requested using 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=
+
+A client can request to speak protocol v2 by sending `version=2` in the
+side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first contacting a
+server a list of capabilities will advertised.  Some of these capabilities
+will be commands which a client can request be executed.  Once a command
+has completed, a client can reuse the connection and request that other
+commands be executed.
+
+ Special Packets
+-
+
+In protocol v2 these special packets will have the following semantics:
+
+  * '' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message
+
+ Capability Advertisement
+--
+
+A server which decides to communicate (based on a request from a client)
+using protocol version 2, notifies the client by sending a version string
+in its initial response followed by an advertisement of its capabilities.
+Each capability is a key with an optional value.  Clients must ignore all
+unknown keys.  Semantics of unknown values are left to the definition of
+each key.  Some capabilities will describe commands which can be requested
+to be executed by the client.
+
+capability-advertisement = protocol-version
+  capability-list
+  flush-pkt
+
+protocol-version = PKT-LINE("version 2" LF)
+capability-list = *capability
+capability = PKT-LINE(key[=value] LF)
+
+key = 1*CHAR
+value = 1*CHAR
+CHAR = 1*(ALPHA / DIGIT / "-" / "_")
+
+A client then responds to select the command it wants with any particular
+capabilities or arguments.  There is then an optional section where the
+client can provide any command specific parameters or queries.
+
+command-request = command
+ capability-list
+ (command-args)
+ flush-pkt
+command = PKT-LINE("command=" key LF)
+command-args = delim-pkt
+  *arg
+arg = 1*CHAR
+
+The server will then check to ensure that the client's request is
+comprised of a valid command as well as valid capabilities which were
+advertised.  If the request is valid the server 

[PATCH 17/26] fetch: pass ref patterns when fetching

[Brandon Williams]

Construct a list of ref patterns to be passed to
'transport_get_remote_refs()' from the refspec to be used during the
fetch.  This list of ref patterns will be used to allow the server to
filter the ref advertisement when communicating using protocol v2.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 850382f55..8128450bf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -332,11 +332,21 @@ static struct ref *get_ref_map(struct transport 
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
+   struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = 
 
-   const struct ref *remote_refs = transport_get_remote_refs(transport, 
NULL);
+   const struct ref *remote_refs;
+
+   for (i = 0; i < refspec_count; i++) {
+   if (!refspecs[i].exact_sha1)
+   argv_array_push(_patterns, refspecs[i].src);
+   }
+
+   remote_refs = transport_get_remote_refs(transport, _patterns);
+
+   argv_array_clear(_patterns);
 
if (refspec_count) {
struct refspec *fetch_refspec;
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 15/26] transport: convert transport_get_remote_refs to take a list of ref patterns

[Brandon Williams]

Convert 'transport_get_remote_refs()' to optionally take a list of ref
patterns.

Signed-off-by: Brandon Williams 
---
 builtin/clone.c | 2 +-
 builtin/fetch.c | 4 ++--
 builtin/ls-remote.c | 2 +-
 builtin/remote.c| 2 +-
 transport.c | 7 +--
 transport.h | 3 ++-
 6 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 2da71db10..4db3079ac 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1104,7 +1104,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (transport->smart_options && !deepen)
transport->smart_options->check_self_contained_and_connected = 
1;
 
-   refs = transport_get_remote_refs(transport);
+   refs = transport_get_remote_refs(transport, NULL);
 
if (refs) {
mapped_refs = wanted_peer_refs(refs, refspec);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7bbcd26fa..850382f55 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -250,7 +250,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
 
for_each_ref(add_existing, _refs);
-   for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+   for (ref = transport_get_remote_refs(transport, NULL); ref; ref = 
ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
 
@@ -336,7 +336,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = 
 
-   const struct ref *remote_refs = transport_get_remote_refs(transport);
+   const struct ref *remote_refs = transport_get_remote_refs(transport, 
NULL);
 
if (refspec_count) {
struct refspec *fetch_refspec;
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c4be98ab9..c6e9847c5 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -96,7 +96,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
if (uploadpack != NULL)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
-   ref = transport_get_remote_refs(transport);
+   ref = transport_get_remote_refs(transport, NULL);
if (transport_disconnect(transport))
return 1;
 
diff --git a/builtin/remote.c b/builtin/remote.c
index d95bf904c..d0b6ff6e2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -862,7 +862,7 @@ static int get_remote_ref_states(const char *name,
if (query) {
transport = transport_get(states->remote, 
states->remote->url_nr > 0 ?
states->remote->url[0] : NULL);
-   remote_refs = transport_get_remote_refs(transport);
+   remote_refs = transport_get_remote_refs(transport, NULL);
transport_disconnect(transport);
 
states->queried = 1;
diff --git a/transport.c b/transport.c
index c54a44630..dfc603b36 100644
--- a/transport.c
+++ b/transport.c
@@ -1136,10 +1136,13 @@ int transport_push(struct transport *transport,
return 1;
 }
 
-const struct ref *transport_get_remote_refs(struct transport *transport)
+const struct ref *transport_get_remote_refs(struct transport *transport,
+   const struct argv_array 
*ref_patterns)
 {
if (!transport->got_remote_refs) {
-   transport->remote_refs = 
transport->vtable->get_refs_list(transport, 0, NULL);
+   transport->remote_refs =
+   transport->vtable->get_refs_list(transport, 0,
+ref_patterns);
transport->got_remote_refs = 1;
}
 
diff --git a/transport.h b/transport.h
index 731c78b67..4b656f315 100644
--- a/transport.h
+++ b/transport.h
@@ -178,7 +178,8 @@ int transport_push(struct transport *connection,
   int refspec_nr, const char **refspec, int flags,
   unsigned int * reject_reasons);
 
-const struct ref *transport_get_remote_refs(struct transport *transport);
+const struct ref *transport_get_remote_refs(struct transport *transport,
+   const struct argv_array 
*ref_patterns);
 
 int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 22/26] transport-helper: refactor process_connect_service

[Brandon Williams]

A future patch will need to take advantage of the logic which runs and
processes the response of the connect command on a remote helper so
factor out this logic from 'process_connect_service()' and place it into
a helper function 'run_connect()'.

Signed-off-by: Brandon Williams 
---
 transport-helper.c | 67 +++---
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index d72155768..c032a2a87 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -545,14 +545,13 @@ static int fetch_with_import(struct transport *transport,
return 0;
 }
 
-static int process_connect_service(struct transport *transport,
-  const char *name, const char *exec)
+static int run_connect(struct transport *transport, struct strbuf *cmdbuf)
 {
struct helper_data *data = transport->data;
-   struct strbuf cmdbuf = STRBUF_INIT;
-   struct child_process *helper;
-   int r, duped, ret = 0;
+   int ret = 0;
+   int duped;
FILE *input;
+   struct child_process *helper;
 
helper = get_helper(transport);
 
@@ -568,44 +567,54 @@ static int process_connect_service(struct transport 
*transport,
input = xfdopen(duped, "r");
setvbuf(input, NULL, _IONBF, 0);
 
+   sendline(data, cmdbuf);
+   if (recvline_fh(input, cmdbuf))
+   exit(128);
+
+   if (!strcmp(cmdbuf->buf, "")) {
+   data->no_disconnect_req = 1;
+   if (debug)
+   fprintf(stderr, "Debug: Smart transport connection "
+   "ready.\n");
+   ret = 1;
+   } else if (!strcmp(cmdbuf->buf, "fallback")) {
+   if (debug)
+   fprintf(stderr, "Debug: Falling back to dumb "
+   "transport.\n");
+   } else {
+   die("Unknown response to connect: %s",
+   cmdbuf->buf);
+   }
+
+   fclose(input);
+   return ret;
+}
+
+static int process_connect_service(struct transport *transport,
+  const char *name, const char *exec)
+{
+   struct helper_data *data = transport->data;
+   struct strbuf cmdbuf = STRBUF_INIT;
+   int ret = 0;
+
/*
 * Handle --upload-pack and friends. This is fire and forget...
 * just warn if it fails.
 */
if (strcmp(name, exec)) {
-   r = set_helper_option(transport, "servpath", exec);
+   int r = set_helper_option(transport, "servpath", exec);
if (r > 0)
warning("Setting remote service path not supported by 
protocol.");
else if (r < 0)
warning("Invalid remote service path.");
}
 
-   if (data->connect)
+   if (data->connect) {
strbuf_addf(, "connect %s\n", name);
-   else
-   goto exit;
-
-   sendline(data, );
-   if (recvline_fh(input, ))
-   exit(128);
-
-   if (!strcmp(cmdbuf.buf, "")) {
-   data->no_disconnect_req = 1;
-   if (debug)
-   fprintf(stderr, "Debug: Smart transport connection "
-   "ready.\n");
-   ret = 1;
-   } else if (!strcmp(cmdbuf.buf, "fallback")) {
-   if (debug)
-   fprintf(stderr, "Debug: Falling back to dumb "
-   "transport.\n");
-   } else
-   die("Unknown response to connect: %s",
-   cmdbuf.buf);
+   ret = run_connect(transport, );
+   }
 
-exit:
strbuf_release();
-   fclose(input);
return ret;
 }
 
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 25/26] remote-curl: create copy of the service name

[Brandon Williams]

Make a copy of the service name being requested instead of relying on
the buffer pointed to by the passed in 'const char *' to remain
unchanged.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index dae8a4a48..4086aa733 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -165,7 +165,7 @@ static int set_option(const char *name, const char *value)
 }
 
 struct discovery {
-   const char *service;
+   char *service;
char *buf_alloc;
char *buf;
size_t len;
@@ -257,6 +257,7 @@ static void free_discovery(struct discovery *d)
free(d->shallow.oid);
free(d->buf_alloc);
free_refs(d->refs);
+   free(d->service);
free(d);
}
 }
@@ -343,7 +344,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
warning(_("redirecting to %s"), url.buf);
 
last= xcalloc(1, sizeof(*last_discovery));
-   last->service = service;
+   last->service = xstrdup(service);
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 19/26] upload-pack: introduce fetch server command

[Brandon Williams]

Introduce the 'fetch' server command.

Signed-off-by: Brandon Williams 
---
 Documentation/technical/protocol-v2.txt |  14 ++
 serve.c |   2 +
 upload-pack.c   | 290 
 upload-pack.h   |   9 +
 4 files changed, 315 insertions(+)
 create mode 100644 upload-pack.h

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index 5f4d0e719..2a8e2f226 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -115,3 +115,17 @@ The output of ls-refs is as follows:
 
 symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
 shallow = PKT-LINE("shallow" SP obj-id LF)
+
+ Fetch
+---
+
+Fetch will need to be a modified version of the v1 fetch protocol.  Some
+potential areas for improvement are: Ref-in-want, CDN offloading,
+Fetch-options.
+
+Since we'll have an 'ls-ref' service we can eliminate the need of fetch
+to perform a ref-advertisement, instead a client can run the 'ls-refs'
+service first, in order to find out what refs the server has, and then
+request those refs directly using the fetch service.
+
+//TODO Flesh out the design
diff --git a/serve.c b/serve.c
index 88d548410..ca3bb7190 100644
--- a/serve.c
+++ b/serve.c
@@ -6,6 +6,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "serve.h"
+#include "upload-pack.h"
 
 static int always_advertise(struct repository *r,
struct strbuf *value)
@@ -46,6 +47,7 @@ static struct protocol_capability capabilities[] = {
{ "agent", agent_advertise, NULL },
{ "stateless-rpc", always_advertise, NULL },
{ "ls-refs", always_advertise, ls_refs },
+   { "fetch", always_advertise, upload_pack_v2 },
 };
 
 static void advertise_capabilities(void)
diff --git a/upload-pack.c b/upload-pack.c
index 2ca60d27c..c41f6f528 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -20,6 +20,7 @@
 #include "prio-queue.h"
 #include "protocol.h"
 #include "serve.h"
+#include "upload-pack.h"
 
 static const char * const upload_pack_usage[] = {
N_("git upload-pack [] "),
@@ -1040,6 +1041,295 @@ static void upload_pack(void)
}
 }
 
+struct upload_pack_data {
+   struct object_array wants;
+   struct oid_array haves;
+
+   unsigned stateless_rpc : 1;
+
+   unsigned use_thin_pack : 1;
+   unsigned use_ofs_delta : 1;
+   unsigned no_progress : 1;
+   unsigned use_include_tag : 1;
+   unsigned done : 1;
+};
+
+#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, 0, 0, 0, 0, 
0, 0 }
+
+static void upload_pack_data_clear(struct upload_pack_data *data)
+{
+   object_array_clear(>wants);
+   oid_array_clear(>haves);
+}
+
+static int parse_want(const char *line)
+{
+   const char *arg;
+   if (skip_prefix(line, "want ", )) {
+   struct object_id oid;
+   struct object *o;
+
+   if (get_oid_hex(arg, ))
+   die("git upload-pack: protocol error, "
+   "expected to get oid, not '%s'", line);
+
+   o = parse_object();
+   if (!o) {
+   packet_write_fmt(1,
+"ERR upload-pack: not our ref %s",
+oid_to_hex());
+   die("git upload-pack: not our ref %s",
+   oid_to_hex());
+   }
+
+   if (!(o->flags & WANTED)) {
+   o->flags |= WANTED;
+   add_object_array(o, NULL, _obj);
+   }
+
+   return 1;
+   }
+
+   return 0;
+}
+
+static int parse_have(const char *line, struct oid_array *haves)
+{
+   const char *arg;
+   if (skip_prefix(line, "have ", )) {
+   struct object_id oid;
+
+   if (get_oid_hex(arg, ))
+   die("git upload-pack: expected SHA1 object, got '%s'", 
arg);
+   oid_array_append(haves, );
+   return 1;
+   }
+
+   return 0;
+}
+
+static void process_args(struct argv_array *args, struct upload_pack_data 
*data)
+{
+   int i;
+
+   for (i = 0; i < args->argc; i++) {
+   const char *arg = args->argv[i];
+
+   /* process want */
+   if (parse_want(arg))
+   continue;
+   /* process have line */
+   if (parse_have(arg, >haves))
+   continue;
+
+   /* process args like thin-pack */
+   if (!strcmp(arg, "thin-pack")) {
+   use_thin_pack = 1;
+   continue;
+   }
+   if (!strcmp(arg, "ofs-delta")) {
+   use_ofs_delta = 1;
+   continue;
+   }
+   if (!strcmp(arg, 

[PATCH 12/26] ls-refs: introduce ls-refs server command

[Brandon Williams]

Introduce the ls-refs server command.  In protocol v2, the ls-refs
command is used to request the ref advertisement from the server.  Since
it is a command which can be requested (as opposed to mandatory in v1),
a client can sent a number of parameters in its request to limit the ref
advertisement based on provided ref-patterns.

Signed-off-by: Brandon Williams 
---
 Documentation/technical/protocol-v2.txt | 26 +
 Makefile|  1 +
 ls-refs.c   | 97 +
 ls-refs.h   |  9 +++
 serve.c |  2 +
 5 files changed, 135 insertions(+)
 create mode 100644 ls-refs.c
 create mode 100644 ls-refs.h

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index b87ba3816..5f4d0e719 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -89,3 +89,29 @@ terminate the connection.
 Commands are the core actions that a client wants to perform (fetch, push,
 etc).  Each command will be provided with a list capabilities and
 arguments as requested by a client.
+
+ Ls-refs
+-
+
+Ls-refs is the command used to request a reference advertisement in v2.
+Unlike the current reference advertisement, ls-refs takes in parameters
+which can be used to limit the refs sent from the server.
+
+Ls-ref takes in the following parameters wraped in packet-lines:
+
+  symrefs: In addition to the object pointed by it, show the underlying
+  ref pointed by it when showing a symbolic ref.
+  peel: Show peeled tags.
+  ref-pattern : When specified, only references matching the
+given patterns are displayed.
+
+The output of ls-refs is as follows:
+
+output = *ref
+flush-pkt
+ref = PKT-LINE((tip | peeled) LF)
+tip = obj-id SP refname (SP symref-target)
+peeled = obj-id SP refname "^{}"
+
+symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
+shallow = PKT-LINE("shallow" SP obj-id LF)
diff --git a/Makefile b/Makefile
index 5f3b5fe8b..152a73bec 100644
--- a/Makefile
+++ b/Makefile
@@ -820,6 +820,7 @@ LIB_OBJS += list-objects-filter-options.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o
 LIB_OBJS += log-tree.o
+LIB_OBJS += ls-refs.o
 LIB_OBJS += mailinfo.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
diff --git a/ls-refs.c b/ls-refs.c
new file mode 100644
index 0..ac4904a40
--- /dev/null
+++ b/ls-refs.c
@@ -0,0 +1,97 @@
+#include "cache.h"
+#include "repository.h"
+#include "refs.h"
+#include "remote.h"
+#include "argv-array.h"
+#include "ls-refs.h"
+#include "pkt-line.h"
+
+struct ls_refs_data {
+   unsigned peel;
+   unsigned symrefs;
+   struct argv_array patterns;
+};
+
+/*
+ * Check if one of the patterns matches the tail part of the ref.
+ * If no patterns were provided, all refs match.
+ */
+static int ref_match(const struct argv_array *patterns, const char *refname)
+{
+   char *pathbuf;
+   int i;
+
+   if (!patterns->argc)
+   return 1; /* no restriction */
+
+   pathbuf = xstrfmt("/%s", refname);
+   for (i = 0; i < patterns->argc; i++) {
+   if (!wildmatch(patterns->argv[i], pathbuf, 0)) {
+   free(pathbuf);
+   return 1;
+   }
+   }
+   free(pathbuf);
+   return 0;
+}
+
+static int send_ref(const char *refname, const struct object_id *oid,
+   int flag, void *cb_data)
+{
+   struct ls_refs_data *data = cb_data;
+   const char *refname_nons = strip_namespace(refname);
+   struct strbuf refline = STRBUF_INIT;
+
+   if (!ref_match(>patterns, refname))
+   return 0;
+
+   strbuf_addf(, "%s %s", oid_to_hex(oid), refname_nons);
+   if (data->symrefs && flag & REF_ISSYMREF) {
+   struct object_id unused;
+   const char *symref_target = resolve_ref_unsafe(refname, 0,
+  ,
+  );
+
+   if (!symref_target)
+   die("'%s' is a symref but it is not?", refname);
+
+   strbuf_addf(, " %s", symref_target);
+   }
+
+   strbuf_addch(, '\n');
+
+   packet_write(1, refline.buf, refline.len);
+   if (data->peel) {
+   struct object_id peeled;
+   if (!peel_ref(refname, ))
+   packet_write_fmt(1, "%s %s^{}\n", oid_to_hex(),
+refname_nons);
+   }
+
+   strbuf_release();
+   return 0;
+}
+
+int ls_refs(struct repository *r, struct argv_array *keys, struct argv_array 
*args)
+{
+   int i;
+   struct ls_refs_data data = { 0, 0, ARGV_ARRAY_INIT };
+
+   for (i = 0; i < args->argc; i++) {
+   const char *arg = args->argv[i];
+ 

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

[Brandon Williams]

Teach remote-curl the 'connect-half-duplex' command which is used to
establish a half-duplex connection with servers which support protocol
version 2.  This allows remote-curl to act as a proxy, allowing the git
client to communicate natively with a remote end, simply using
remote-curl as a pass through to convert requests to http.

Signed-off-by: Brandon Williams 
---
 remote-curl.c  | 185 -
 t/t5701-protocol-v2.sh |  41 +++
 2 files changed, 224 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4086aa733..b63b06398 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -171,6 +171,7 @@ struct discovery {
size_t len;
struct ref *refs;
struct oid_array shallow;
+   enum protocol_version version;
unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -184,9 +185,13 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_CHOMP_NEWLINE |
   PACKET_READ_GENTLE_ON_EOF);
 
-   switch (discover_version()) {
+   heads->version = discover_version();
+   switch (heads->version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   /*
+* Do nothing.  Client should run 'connect-half-duplex' and
+* request the refs themselves.
+*/
break;
case protocol_v1:
case protocol_v0:
@@ -1047,6 +1052,178 @@ static void parse_push(struct strbuf *buf)
free(specs);
 }
 
+struct proxy_state {
+   char *service_name;
+   char *service_url;
+   char *hdr_content_type;
+   char *hdr_accept;
+   struct strbuf request_buffer;
+   int in;
+   int out;
+   struct packet_reader reader;
+   size_t pos;
+   int seen_flush;
+};
+
+static void proxy_state_init(struct proxy_state *p, const char *service_name)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   memset(p, 0, sizeof(*p));
+   p->service_name = xstrdup(service_name);
+
+   p->in = 0;
+   p->out = 1;
+   strbuf_init(>request_buffer, 0);
+
+   strbuf_addf(, "%s%s", url.buf, p->service_name);
+   p->service_url = strbuf_detach(, NULL);
+
+   strbuf_addf(, "Content-Type: application/x-%s-request", 
p->service_name);
+   p->hdr_content_type = strbuf_detach(, NULL);
+
+   strbuf_addf(, "Accept: application/x-%s-result", p->service_name);
+   p->hdr_accept = strbuf_detach(, NULL);
+
+   packet_reader_init(>reader, p->in, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF);
+}
+
+static void proxy_state_clear(struct proxy_state *p)
+{
+   free(p->service_name);
+   free(p->service_url);
+   free(p->hdr_content_type);
+   free(p->hdr_accept);
+   strbuf_release(>request_buffer);
+}
+
+static size_t proxy_in(void *ptr, size_t eltsize,
+  size_t nmemb, void *buffer_)
+{
+   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");
+   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;
+}
+static size_t proxy_out(char *ptr, size_t eltsize,
+   size_t nmemb, void *buffer_)
+{
+   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");
+

[PATCH 24/26] pkt-line: add packet_buf_write_len function

[Brandon Williams]

Add the 'packet_buf_write_len()' function which allows for writing an
arbitrary length buffer into a 'struct strbuf' and formatting it in
packet-line format.

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

diff --git a/pkt-line.c b/pkt-line.c
index 3159cbe10..e9968b7df 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -215,6 +215,22 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, 
...)
va_end(args);
 }
 
+void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
+{
+   size_t orig_len, n;
+
+   orig_len = buf->len;
+   strbuf_addstr(buf, "");
+   strbuf_add(buf, data, len);
+   n = buf->len - orig_len;
+
+   if (n > LARGE_PACKET_MAX)
+   die("protocol error: impossibly long line");
+
+   set_packet_header(>buf[orig_len], n);
+   packet_trace(buf->buf + orig_len + 4, n - 4, 1);
+}
+
 int write_packetized_from_fd(int fd_in, int fd_out)
 {
static char buf[LARGE_PACKET_DATA_MAX];
diff --git a/pkt-line.h b/pkt-line.h
index 97b6dd1c7..d411fcb30 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -26,6 +26,7 @@ void packet_buf_flush(struct strbuf *buf);
 void packet_buf_delim(struct strbuf *buf);
 void packet_write(int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
+void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
 int write_packetized_from_fd(int fd_in, int fd_out);
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 23/26] transport-helper: introduce connect-half-duplex

[Brandon Williams]

Introduce the transport-helper capability 'connect-half-duplex'.  This
capability indicates that the transport-helper can be requested to run
the 'connect-half-duplex' command which should attempt to make a
half-duplex connection with a remote end.  Once established, the
half-duplex connection can be used by the git client to communicate with
the remote end natively in a stateles-rpc manner as supported by
protocol v2.

If a half-duplex connection cannot be established then the remote-helper
will respond in the same manner as the 'connect' command indicating that
the client should fallback to using the dumb remote-helper commands.

Signed-off-by: Brandon Williams 
---
 transport-helper.c | 8 
 transport.c| 1 +
 transport.h| 6 ++
 3 files changed, 15 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index c032a2a87..d037609bc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -26,6 +26,7 @@ struct helper_data {
option : 1,
push : 1,
connect : 1,
+   connect_half_duplex : 1,
signed_tags : 1,
check_connectivity : 1,
no_disconnect_req : 1,
@@ -188,6 +189,8 @@ static struct child_process *get_helper(struct transport 
*transport)
refspecs[refspec_nr++] = xstrdup(arg);
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
+   } else if (!strcmp(capname, "connect-half-duplex")) {
+   data->connect_half_duplex = 1;
} else if (!strcmp(capname, "signed-tags")) {
data->signed_tags = 1;
} else if (skip_prefix(capname, "export-marks ", )) {
@@ -612,6 +615,11 @@ static int process_connect_service(struct transport 
*transport,
if (data->connect) {
strbuf_addf(, "connect %s\n", name);
ret = run_connect(transport, );
+   } else if (data->connect_half_duplex) {
+   strbuf_addf(, "connect-half-duplex %s\n", name);
+   ret = run_connect(transport, );
+   if (ret)
+   transport->stateless_rpc = 1;
}
 
strbuf_release();
diff --git a/transport.c b/transport.c
index 4fdbd9adc..aafb8fbb4 100644
--- a/transport.c
+++ b/transport.c
@@ -250,6 +250,7 @@ static int fetch_refs_via_pack(struct transport *transport,
data->options.check_self_contained_and_connected;
args.cloning = transport->cloning;
args.update_shallow = data->options.update_shallow;
+   args.stateless_rpc = transport->stateless_rpc;
 
if (!data->got_remote_heads)
refs_tmp = get_refs_via_connect(transport, 0, NULL);
diff --git a/transport.h b/transport.h
index 4b656f315..9eac809ee 100644
--- a/transport.h
+++ b/transport.h
@@ -55,6 +55,12 @@ struct transport {
 */
unsigned cloning : 1;
 
+   /*
+* Indicates that the transport is connected via a half-duplex
+* connection and should operate in stateless-rpc mode.
+*/
+   unsigned stateless_rpc : 1;
+
/*
 * These strings will be passed to the {pre, post}-receive hook,
 * on the remote side, if both sides support the push options 
capability.
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 20/26] fetch-pack: perform a fetch using v2

[Brandon Williams]

When communicating with a v2 server, perform a fetch by requesting the
'fetch' command.

Signed-off-by: Brandon Williams 
---
 builtin/fetch-pack.c   |   2 +-
 fetch-pack.c   | 267 -
 fetch-pack.h   |   4 +-
 t/t5701-protocol-v2.sh |  40 
 transport.c|   8 +-
 5 files changed, 314 insertions(+), 7 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f492e8abd..867dd3cc7 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -213,7 +213,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
}
 
ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
-, pack_lockfile_ptr);
+, pack_lockfile_ptr, protocol_v0);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
diff --git a/fetch-pack.c b/fetch-pack.c
index 9f6b07ad9..c26fdc539 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1008,6 +1008,262 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
return ref;
 }
 
+static void add_wants(const struct ref *wants, struct strbuf *req_buf)
+{
+   for ( ; wants ; wants = wants->next) {
+   const struct object_id *remote = >old_oid;
+   const char *remote_hex;
+   struct object *o;
+
+   /*
+* If that object is complete (i.e. it is an ancestor of a
+* local ref), we tell them we have it but do not have to
+* tell them about its ancestors, which they already know
+* about.
+*
+* We use lookup_object here because we are only
+* interested in the case we *know* the object is
+* reachable and we have already scanned it.
+*/
+   if (((o = lookup_object(remote->hash)) != NULL) &&
+   (o->flags & COMPLETE)) {
+   continue;
+   }
+
+   remote_hex = oid_to_hex(remote);
+   packet_buf_write(req_buf, "want %s\n", remote_hex);
+   }
+}
+
+static int add_haves(struct strbuf *req_buf, int *in_vain)
+{
+   int ret = 0;
+   int haves_added = 0;
+   const struct object_id *oid;
+
+   while ((oid = get_rev())) {
+   packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
+   if (++haves_added >= INITIAL_FLUSH)
+   break;
+   };
+
+   *in_vain += haves_added;
+   if (!haves_added || *in_vain >= MAX_IN_VAIN) {
+   /* Send Done */
+   packet_buf_write(req_buf, "done\n");
+   ret = 1;
+   }
+
+   return ret;
+}
+
+static int send_haves(int fd_out, int *in_vain)
+{
+   int ret = 0;
+   struct strbuf req_buf = STRBUF_INIT;
+
+   ret = add_haves(_buf, in_vain);
+
+   /* Send request */
+   packet_buf_flush(_buf);
+   write_or_die(fd_out, req_buf.buf, req_buf.len);
+
+   strbuf_release(_buf);
+   return ret;
+}
+
+static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,
+ const struct ref *wants, struct oidset *common,
+ int *in_vain)
+{
+   int ret = 0;
+   struct strbuf req_buf = STRBUF_INIT;
+
+   packet_buf_write(_buf, "command=fetch");
+   packet_buf_write(_buf, "agent=%s", git_user_agent_sanitized());
+   if (args->stateless_rpc)
+   packet_buf_write(_buf, "stateless-rpc=true");
+
+   packet_buf_delim(_buf);
+   if (args->use_thin_pack)
+   packet_buf_write(_buf, "thin-pack");
+   if (args->no_progress)
+   packet_buf_write(_buf, "no-progress");
+   if (args->include_tag)
+   packet_buf_write(_buf, "include-tag");
+   if (prefer_ofs_delta)
+   packet_buf_write(_buf, "ofs-delta");
+
+   /* add wants */
+   add_wants(wants, _buf);
+
+   /*
+* If we are running stateless-rpc we need to add all the common
+* commits we've found in previous rounds
+*/
+   if (args->stateless_rpc) {
+   struct oidset_iter iter;
+   const struct object_id *oid;
+   oidset_iter_init(common, );
+
+   while ((oid = oidset_iter_next())) {
+   packet_buf_write(_buf, "have %s\n", 
oid_to_hex(oid));
+   }
+   }
+
+   /* Add initial haves */
+   ret = add_haves(_buf, in_vain);
+
+   /* Send request */
+   packet_buf_flush(_buf);
+   write_or_die(fd_out, req_buf.buf, req_buf.len);
+
+   strbuf_release(_buf);
+   return ret;
+}
+
+static enum ack_type process_ack(const char *line, struct object_id *oid)
+{
+   const char *arg;
+
+   if (!strcmp(line, "NAK"))
+   return NAK;
+   if (skip_prefix(line, 

[PATCH 21/26] transport-helper: remove name parameter

[Brandon Williams]

Commit 266f1fdfa (transport-helper: be quiet on read errors from
helpers, 2013-06-21) removed a call to 'die()' which printed the name of
the remote helper passed in to the 'recvline_fh()' function using the
'name' parameter.  Once the call to 'die()' was removed the parameter
was no longer necessary but wasn't removed.  Clean up 'recvline_fh()'
parameter list by removing the 'name' parameter.

Signed-off-by: Brandon Williams 
---
 transport-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 4c334b5ee..d72155768 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -49,7 +49,7 @@ static void sendline(struct helper_data *helper, struct 
strbuf *buffer)
die_errno("Full write to remote helper failed");
 }
 
-static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)
+static int recvline_fh(FILE *helper, struct strbuf *buffer)
 {
strbuf_reset(buffer);
if (debug)
@@ -67,7 +67,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, 
const char *name)
 
 static int recvline(struct helper_data *helper, struct strbuf *buffer)
 {
-   return recvline_fh(helper->out, buffer, helper->name);
+   return recvline_fh(helper->out, buffer);
 }
 
 static void write_constant(int fd, const char *str)
@@ -586,7 +586,7 @@ static int process_connect_service(struct transport 
*transport,
goto exit;
 
sendline(data, );
-   if (recvline_fh(input, , name))
+   if (recvline_fh(input, ))
exit(128);
 
if (!strcmp(cmdbuf.buf, "")) {
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 05/26] upload-pack: factor out processing lines

[Brandon Williams]

Factor out the logic for processing shallow, deepen, deepen_since, and
deepen_not lines into their own functions to simplify the
'receive_needs()' function in addition to making it easier to reuse some
of this logic when implementing protocol_v2.

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

diff --git a/upload-pack.c b/upload-pack.c
index 20acaa49d..9a507ae53 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -731,6 +731,75 @@ static void deepen_by_rev_list(int ac, const char **av,
packet_flush(1);
 }
 
+static int process_shallow(const char *line, struct object_array *shallows)
+{
+   const char *arg;
+   if (skip_prefix(line, "shallow ", )) {
+   struct object_id oid;
+   struct object *object;
+   if (get_oid_hex(arg, ))
+   die("invalid shallow line: %s", line);
+   object = parse_object();
+   if (!object)
+   return 1;
+   if (object->type != OBJ_COMMIT)
+   die("invalid shallow object %s", oid_to_hex());
+   if (!(object->flags & CLIENT_SHALLOW)) {
+   object->flags |= CLIENT_SHALLOW;
+   add_object_array(object, NULL, shallows);
+   }
+   return 1;
+   }
+
+   return 0;
+}
+
+static int process_deepen(const char *line, int *depth)
+{
+   const char *arg;
+   if (skip_prefix(line, "deepen ", )) {
+   char *end = NULL;
+   *depth = strtol(arg, , 0);
+   if (!end || *end || depth <= 0)
+   die("Invalid deepen: %s", line);
+   return 1;
+   }
+
+   return 0;
+}
+
+static int process_deepen_since(const char *line, timestamp_t *deepen_since, 
int *deepen_rev_list)
+{
+   const char *arg;
+   if (skip_prefix(line, "deepen-since ", )) {
+   char *end = NULL;
+   *deepen_since = parse_timestamp(arg, , 0);
+   if (!end || *end || !deepen_since ||
+   /* revisions.c's max_age -1 is special */
+   *deepen_since == -1)
+   die("Invalid deepen-since: %s", line);
+   *deepen_rev_list = 1;
+   return 1;
+   }
+   return 0;
+}
+
+static int process_deepen_not(const char *line, struct string_list 
*deepen_not, int *deepen_rev_list)
+{
+   const char *arg;
+   if (skip_prefix(line, "deepen-not ", )) {
+   char *ref = NULL;
+   struct object_id oid;
+   if (expand_ref(arg, strlen(arg), , ) != 1)
+   die("git upload-pack: ambiguous deepen-not: %s", line);
+   string_list_append(deepen_not, ref);
+   free(ref);
+   *deepen_rev_list = 1;
+   return 1;
+   }
+   return 0;
+}
+
 static void receive_needs(void)
 {
struct object_array shallows = OBJECT_ARRAY_INIT;
@@ -752,49 +821,15 @@ static void receive_needs(void)
if (!line)
break;
 
-   if (skip_prefix(line, "shallow ", )) {
-   struct object_id oid;
-   struct object *object;
-   if (get_oid_hex(arg, ))
-   die("invalid shallow line: %s", line);
-   object = parse_object();
-   if (!object)
-   continue;
-   if (object->type != OBJ_COMMIT)
-   die("invalid shallow object %s", 
oid_to_hex());
-   if (!(object->flags & CLIENT_SHALLOW)) {
-   object->flags |= CLIENT_SHALLOW;
-   add_object_array(object, NULL, );
-   }
+   if (process_shallow(line, ))
continue;
-   }
-   if (skip_prefix(line, "deepen ", )) {
-   char *end = NULL;
-   depth = strtol(arg, , 0);
-   if (!end || *end || depth <= 0)
-   die("Invalid deepen: %s", line);
+   if (process_deepen(line, ))
continue;
-   }
-   if (skip_prefix(line, "deepen-since ", )) {
-   char *end = NULL;
-   deepen_since = parse_timestamp(arg, , 0);
-   if (!end || *end || !deepen_since ||
-   /* revisions.c's max_age -1 is special */
-   deepen_since == -1)
-   die("Invalid deepen-since: %s", line);
-   deepen_rev_list = 1;
+   if (process_deepen_since(line, _since, _rev_list))
continue;
-   

[PATCH 03/26] pkt-line: add delim packet support

[Brandon Williams]

One of the design goals of protocol-v2 is to improve the semantics of
flush packets.  Currently in protocol-v1, flush packets are used both to
indicate a break in a list of packet lines as well as an indication that
one side has finished speaking.  This makes it particularly difficult
to implement proxies as a proxy would need to completely understand git
protocol instead of simply looking for a flush packet.

To do this, introduce the special deliminator packet '0001'.  A delim
packet can then be used as a deliminator between lists of packet lines
while flush packets can be reserved to indicate the end of a response.

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

diff --git a/pkt-line.c b/pkt-line.c
index 98c2d7d68..3159cbe10 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -91,6 +91,12 @@ void packet_flush(int fd)
write_or_die(fd, "", 4);
 }
 
+void packet_delim(int fd)
+{
+   packet_trace("0001", 4, 1);
+   write_or_die(fd, "0001", 4);
+}
+
 int packet_flush_gently(int fd)
 {
packet_trace("", 4, 1);
@@ -105,6 +111,12 @@ void packet_buf_flush(struct strbuf *buf)
strbuf_add(buf, "", 4);
 }
 
+void packet_buf_delim(struct strbuf *buf)
+{
+   packet_trace("0001", 4, 1);
+   strbuf_add(buf, "0001", 4);
+}
+
 static void set_packet_header(char *buf, const int size)
 {
static char hexchar[] = "0123456789abcdef";
@@ -297,7 +309,10 @@ enum packet_read_status packet_read_with_status(int fd, 
char **src_buffer, size_
if (len == 0) {
packet_trace("", 4, 0);
return PACKET_READ_FLUSH;
-   } else if (len >= 1 && len <= 3) {
+   } else if (len == 1) {
+   packet_trace("0001", 4, 0);
+   return PACKET_READ_DELIM;
+   } else if (len >= 2 && len <= 3) {
die("protocol error: bad line length character: %.4s", linelen);
}
 
@@ -333,6 +348,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len,
break;
case PACKET_READ_NORMAL:
break;
+   case PACKET_READ_DELIM:
case PACKET_READ_FLUSH:
pktlen = 0;
break;
@@ -445,6 +461,7 @@ enum packet_read_status packet_reader_read(struct 
packet_reader *reader)
case PACKET_READ_NORMAL:
reader->line = reader->buffer;
break;
+   case PACKET_READ_DELIM:
case PACKET_READ_FLUSH:
reader->pktlen = 0;
reader->line = NULL;
diff --git a/pkt-line.h b/pkt-line.h
index c446e886a..97b6dd1c7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -20,8 +20,10 @@
  * side can't, we stay with pure read/write interfaces.
  */
 void packet_flush(int fd);
+void packet_delim(int fd);
 void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format 
(printf, 2, 3)));
 void packet_buf_flush(struct strbuf *buf);
+void packet_buf_delim(struct strbuf *buf);
 void packet_write(int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
 int packet_flush_gently(int fd);
@@ -75,6 +77,7 @@ enum packet_read_status {
PACKET_READ_EOF = -1,
PACKET_READ_NORMAL,
PACKET_READ_FLUSH,
+   PACKET_READ_DELIM,
 };
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
size_t *src_len,
char *buffer, unsigned size, 
int *pktlen,
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 18/26] push: pass ref patterns when pushing

[Brandon Williams]

Construct a list of ref patterns to be passed to 'get_refs_list()' from
the refspec to be used during the push.  This list of ref patterns will
be used to allow the server to filter the ref advertisement when
communicating using protocol v2.

Signed-off-by: Brandon Williams 
---
 transport.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index dfc603b36..6ea3905e3 100644
--- a/transport.c
+++ b/transport.c
@@ -1026,11 +1026,26 @@ int transport_push(struct transport *transport,
int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
+   struct refspec *tmp_rs;
+   struct argv_array ref_patterns = ARGV_ARRAY_INIT;
+   int i;
 
if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
return -1;
 
-   remote_refs = transport->vtable->get_refs_list(transport, 1, 
NULL);
+   tmp_rs = parse_push_refspec(refspec_nr, refspec);
+   for (i = 0; i < refspec_nr; i++) {
+   if (tmp_rs[i].dst)
+   argv_array_push(_patterns, tmp_rs[i].dst);
+   else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
+   argv_array_push(_patterns, tmp_rs[i].src);
+   }
+
+   remote_refs = transport->vtable->get_refs_list(transport, 1,
+  _patterns);
+
+   argv_array_clear(_patterns);
+   free_refspec(refspec_nr, tmp_rs);
 
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 00/26] protocol version 2

[Brandon Williams]

The following patches extend what I sent out as an WIP
(https://public-inbox.org/git/20171204235823.63299-1-bmw...@google.com/) and
implement protocol version 2.

Some changes from that series are as follows:
 * Lots of various cleanup on the ls-refs and fetch command code, both server
   and client.
 * Fetch command now supports a stateless-rpc mode which enables communicating
   with a half-duplex connection.
 * 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.
 * The transport code is refactored for ls-remote, fetch, and push to provide a
   list of ref-patterns (based on the refspec being used) when requesting refs
   from the remote end.  This allows the ls-refs code to send this list of
   patterns so the remote end and filter the refs it sends back.

This series effectively implements protocol version 2 for listing a remotes
refs (ls-remote) as well as for fetch for the builtin transports (ssh, git,
file) and for the http/https transports.  Push is not implemented yet and
doesn't need to be implemented at the same time as fetch since the
receive-pack code can default to using protocol v0 when v2 is requested by the
client.

Any feedback is appreciated! Thanks!
Brandon

Brandon Williams (26):
  pkt-line: introduce packet_read_with_status
  pkt-line: introduce struct packet_reader
  pkt-line: add delim packet support
  upload-pack: convert to a builtin
  upload-pack: factor out processing lines
  transport: use get_refs_via_connect to get refs
  connect: convert get_remote_heads to use struct packet_reader
  connect: discover protocol version outside of get_remote_heads
  transport: store protocol version
  protocol: introduce enum protocol_version value protocol_v2
  serve: introduce git-serve
  ls-refs: introduce ls-refs server command
  connect: request remote refs using v2
  transport: convert get_refs_list to take a list of ref patterns
  transport: convert transport_get_remote_refs to take a list of ref
patterns
  ls-remote: pass ref patterns when requesting a remote's refs
  fetch: pass ref patterns when fetching
  push: pass ref patterns when pushing
  upload-pack: introduce fetch server command
  fetch-pack: perform a fetch using v2
  transport-helper: remove name parameter
  transport-helper: refactor process_connect_service
  transport-helper: introduce connect-half-duplex
  pkt-line: add packet_buf_write_len function
  remote-curl: create copy of the service name
  remote-curl: implement connect-half-duplex command

 .gitignore  |   1 +
 Documentation/technical/protocol-v2.txt | 131 ++
 Makefile|   6 +-
 builtin.h   |   2 +
 builtin/clone.c |   2 +-
 builtin/fetch-pack.c|  21 +-
 builtin/fetch.c |  14 +-
 builtin/ls-remote.c |   7 +-
 builtin/receive-pack.c  |   6 +
 builtin/remote.c|   2 +-
 builtin/send-pack.c |  20 +-
 builtin/serve.c |  30 +++
 connect.c   | 226 +-
 connect.h   |   3 +
 fetch-pack.c| 267 -
 fetch-pack.h|   4 +-
 git.c   |   2 +
 ls-refs.c   |  97 
 ls-refs.h   |   9 +
 pkt-line.c  | 147 +++-
 pkt-line.h  |  76 ++
 protocol.c  |   2 +
 protocol.h  |   1 +
 remote-curl.c   | 209 +++-
 remote.h|   9 +-
 serve.c | 243 +++
 serve.h |  15 ++
 t/t5701-protocol-v2.sh  | 117 +
 transport-helper.c  |  84 ---
 transport-internal.h|   4 +-
 transport.c | 119 ++---
 transport.h |   9 +-
 upload-pack.c   | 412 
 upload-pack.h   |   9 +
 34 files changed, 2108 insertions(+), 198 deletions(-)
 create mode 100644 Documentation/technical/protocol-v2.txt
 create mode 100644 builtin/serve.c
 create mode 100644 ls-refs.c
 create mode 100644 ls-refs.h
 create mode 100644 serve.c
 create mode 100644 serve.h
 create mode 100755 t/t5701-protocol-v2.sh
 create mode 100644 upload-pack.h

-- 

[PATCH 16/26] ls-remote: pass ref patterns when requesting a remote's refs

[Brandon Williams]

Construct an argv_array of the ref patterns supplied via the command
line and pass them to 'transport_get_remote_refs()' to be used when
communicating protocol v2 so that the server can limit the ref
advertisement based on the supplied patterns.

Signed-off-by: Brandon Williams 
---
 builtin/ls-remote.c| 7 +--
 t/t5701-protocol-v2.sh | 8 
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c6e9847c5..caf1051f3 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -43,6 +43,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
int show_symref_target = 0;
const char *uploadpack = NULL;
const char **pattern = NULL;
+   struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
struct remote *remote;
struct transport *transport;
@@ -74,8 +75,10 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
if (argc > 1) {
int i;
pattern = xcalloc(argc, sizeof(const char *));
-   for (i = 1; i < argc; i++)
+   for (i = 1; i < argc; i++) {
pattern[i - 1] = xstrfmt("*/%s", argv[i]);
+   argv_array_push(_patterns, argv[i]);
+   }
}
 
remote = remote_get(dest);
@@ -96,7 +99,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
if (uploadpack != NULL)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
-   ref = transport_get_remote_refs(transport, NULL);
+   ref = transport_get_remote_refs(transport, _patterns);
if (transport_disconnect(transport))
return 1;
 
diff --git a/t/t5701-protocol-v2.sh b/t/t5701-protocol-v2.sh
index 4bf4d61ac..7d8aeb766 100755
--- a/t/t5701-protocol-v2.sh
+++ b/t/t5701-protocol-v2.sh
@@ -25,4 +25,12 @@ test_expect_success 'list refs with file:// using protocol 
v2' '
test_cmp actual expect
 '
 
+test_expect_success 'ref advertisment is filtered with ls-remote using 
protocol v2' '
+   GIT_TRACE_PACKET=1 git -c protocol.version=2 \
+   ls-remote "file://$(pwd)/file_parent" master 2>log &&
+
+   grep "ref-pattern master" log &&
+   ! grep "refs/tags/" log
+'
+
 test_done
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 14/26] transport: convert get_refs_list to take a list of ref patterns

[Brandon Williams]

Convert the 'struct transport' virtual function 'get_refs_list()' to
optionally take an argv_array of ref patterns.  When communicating with
a server using protocol v2 these ref patterns can be sent when
requesting a listing of their refs allowing the server to filter the
refs it sends based on the sent patterns.

Signed-off-by: Brandon Williams 
---
 transport-helper.c   |  5 +++--
 transport-internal.h |  4 +++-
 transport.c  | 16 +---
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 508015023..4c334b5ee 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1026,7 +1026,8 @@ static int has_attribute(const char *attrs, const char 
*attr) {
}
 }
 
-static struct ref *get_refs_list(struct transport *transport, int for_push)
+static struct ref *get_refs_list(struct transport *transport, int for_push,
+const struct argv_array *ref_patterns)
 {
struct helper_data *data = transport->data;
struct child_process *helper;
@@ -1039,7 +1040,7 @@ static struct ref *get_refs_list(struct transport 
*transport, int for_push)
 
if (process_connect(transport, for_push)) {
do_take_over(transport);
-   return transport->vtable->get_refs_list(transport, for_push);
+   return transport->vtable->get_refs_list(transport, for_push, 
ref_patterns);
}
 
if (data->push && for_push)
diff --git a/transport-internal.h b/transport-internal.h
index 3c1a29d72..a67657ce3 100644
--- a/transport-internal.h
+++ b/transport-internal.h
@@ -3,6 +3,7 @@
 
 struct ref;
 struct transport;
+struct argv_array;
 
 struct transport_vtable {
/**
@@ -21,7 +22,8 @@ struct transport_vtable {
 * the ref without a huge amount of effort, it should store it
 * in the ref's old_sha1 field; otherwise it should be all 0.
 **/
-   struct ref *(*get_refs_list)(struct transport *transport, int for_push);
+   struct ref *(*get_refs_list)(struct transport *transport, int for_push,
+const struct argv_array *ref_patterns);
 
/**
 * Fetch the objects for the given refs. Note that this gets
diff --git a/transport.c b/transport.c
index ffc6b2614..c54a44630 100644
--- a/transport.c
+++ b/transport.c
@@ -72,7 +72,7 @@ struct bundle_transport_data {
struct bundle_header header;
 };
 
-static struct ref *get_refs_from_bundle(struct transport *transport, int 
for_push)
+static struct ref *get_refs_from_bundle(struct transport *transport, int 
for_push, const struct argv_array *ref_patterns)
 {
struct bundle_transport_data *data = transport->data;
struct ref *result = NULL;
@@ -189,7 +189,8 @@ static int connect_setup(struct transport *transport, int 
for_push)
return 0;
 }
 
-static struct ref *get_refs_via_connect(struct transport *transport, int 
for_push)
+static struct ref *get_refs_via_connect(struct transport *transport, int 
for_push,
+   const struct argv_array *ref_patterns)
 {
struct git_transport_data *data = transport->data;
struct ref *refs = NULL;
@@ -204,7 +205,8 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
data->version = discover_version();
switch (data->version) {
case protocol_v2:
-   get_remote_refs(data->fd[1], , , for_push, NULL);
+   get_remote_refs(data->fd[1], , , for_push,
+   ref_patterns);
break;
case protocol_v1:
case protocol_v0:
@@ -250,7 +252,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.update_shallow = data->options.update_shallow;
 
if (!data->got_remote_heads)
-   refs_tmp = get_refs_via_connect(transport, 0);
+   refs_tmp = get_refs_via_connect(transport, 0, NULL);
 
switch (data->version) {
case protocol_v2:
@@ -568,7 +570,7 @@ static int git_transport_push(struct transport *transport, 
struct ref *remote_re
int ret = 0;
 
if (!data->got_remote_heads)
-   get_refs_via_connect(transport, 1);
+   get_refs_via_connect(transport, 1, NULL);
 
memset(, 0, sizeof(args));
args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
@@ -1028,7 +1030,7 @@ int transport_push(struct transport *transport,
if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
return -1;
 
-   remote_refs = transport->vtable->get_refs_list(transport, 1);
+   remote_refs = transport->vtable->get_refs_list(transport, 1, 
NULL);
 
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
@@ -1137,7 +1139,7 @@ int transport_push(struct transport *transport,
 const struct ref 

[PATCH 04/26] upload-pack: convert to a builtin

[Brandon Williams]

In order to allow for code sharing with the server-side of fetch in
protocol-v2 convert upload-pack to be a builtin.

Signed-off-by: Brandon Williams 
---
 Makefile  | 3 ++-
 builtin.h | 1 +
 git.c | 1 +
 upload-pack.c | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 2a81ae22e..e0740b452 100644
--- a/Makefile
+++ b/Makefile
@@ -636,7 +636,6 @@ PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
 PROGRAM_OBJS += show-index.o
-PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += remote-testsvn.o
 
 # Binary suffix, set to .exe for Windows builds
@@ -701,6 +700,7 @@ BUILT_INS += git-merge-subtree$X
 BUILT_INS += git-show$X
 BUILT_INS += git-stage$X
 BUILT_INS += git-status$X
+BUILT_INS += git-upload-pack$X
 BUILT_INS += git-whatchanged$X
 
 # what 'all' will build and 'install' will install in gitexecdir,
@@ -904,6 +904,7 @@ LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
 LIB_OBJS += unpack-trees.o
+LIB_OBJS += upload-pack.o
 LIB_OBJS += url.o
 LIB_OBJS += urlmatch.o
 LIB_OBJS += usage.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa..f332a1257 100644
--- a/builtin.h
+++ b/builtin.h
@@ -231,6 +231,7 @@ extern int cmd_update_ref(int argc, const char **argv, 
const char *prefix);
 extern int cmd_update_server_info(int argc, const char **argv, const char 
*prefix);
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive_writer(int argc, const char **argv, const char 
*prefix);
+extern int cmd_upload_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index c870b9719..f71073dc8 100644
--- a/git.c
+++ b/git.c
@@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {
{ "update-server-info", cmd_update_server_info, RUN_SETUP },
{ "upload-archive", cmd_upload_archive },
{ "upload-archive--writer", cmd_upload_archive_writer },
+   { "upload-pack", cmd_upload_pack },
{ "var", cmd_var, RUN_SETUP_GENTLY },
{ "verify-commit", cmd_verify_commit, RUN_SETUP },
{ "verify-pack", cmd_verify_pack },
diff --git a/upload-pack.c b/upload-pack.c
index d5de18127..20acaa49d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1032,7 +1032,7 @@ static int upload_pack_config(const char *var, const char 
*value, void *unused)
return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 {
const char *dir;
int strict = 0;
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 07/26] connect: convert get_remote_heads to use struct packet_reader

[Brandon Williams]

In order to allow for better control flow when protocol_v2 is introduced
convert 'get_remote_heads()' to use 'struct packet_reader' to read
packet lines.  This enables a client to be able to peek the first line
of a server's response (without consuming it) in order to determine the
protocol version its speaking and then passing control to the
appropriate handler.

This is needed because the initial response from a server speaking
protocol_v0 includes the first ref, while subsequent protocol versions
respond with a version line.  We want to be able to read this first line
without consuming the first ref sent in the protocol_v0 case so that the
protocol version the server is speaking can be determined outside of
'get_remote_heads()' in a future patch.

Signed-off-by: Brandon Williams 
---
 connect.c | 127 +++---
 1 file changed, 72 insertions(+), 55 deletions(-)

diff --git a/connect.c b/connect.c
index c3a014c5b..03bbb74e4 100644
--- a/connect.c
+++ b/connect.c
@@ -48,6 +48,12 @@ int check_ref_type(const struct ref *ref, int flags)
 
 static void die_initial_contact(int unexpected)
 {
+   /*
+* A hang-up after seeing some response from the other end
+* means that it is unexpected, as we know the other end is
+* willing to talk to us.  A hang-up before seeing any
+* response does not necessarily mean an ACL problem, though.
+*/
if (unexpected)
die(_("The remote end hung up upon initial contact"));
else
@@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
 }
 
+static enum protocol_version discover_version(struct packet_reader *reader)
+{
+   enum protocol_version version = protocol_unknown_version;
+
+   /*
+* Peek the first line of the server's response to
+* determine the protocol version the server is speaking.
+*/
+   switch (packet_reader_peek(reader)) {
+   case PACKET_READ_EOF:
+   die_initial_contact(0);
+   case PACKET_READ_FLUSH:
+   case PACKET_READ_DELIM:
+   version = protocol_v0;
+   break;
+   case PACKET_READ_NORMAL:
+   version = determine_protocol_version_client(reader->line);
+   break;
+   }
+
+   /* Maybe process capabilities here, at least for v2 */
+   switch (version) {
+   case protocol_v1:
+   /* Read the peeked version line */
+   packet_reader_read(reader);
+   break;
+   case protocol_v0:
+   break;
+   case protocol_unknown_version:
+   die("unknown protocol version: '%s'\n", reader->line);
+   }
+
+   return version;
+}
+
 static void parse_one_symref_info(struct string_list *symref, const char *val, 
int len)
 {
char *sym, *target;
@@ -109,44 +150,10 @@ static void annotate_refs_with_symref_info(struct ref 
*ref)
string_list_clear(, 0);
 }
 
-/*
- * Read one line of a server's ref advertisement into packet_buffer.
- */
-static int read_remote_ref(int in, char **src_buf, size_t *src_len,
-  int *responded)
-{
-   int len = packet_read(in, src_buf, src_len,
- packet_buffer, sizeof(packet_buffer),
- PACKET_READ_GENTLE_ON_EOF |
- PACKET_READ_CHOMP_NEWLINE);
-   const char *arg;
-   if (len < 0)
-   die_initial_contact(*responded);
-   if (len > 4 && skip_prefix(packet_buffer, "ERR ", ))
-   die("remote error: %s", arg);
-
-   *responded = 1;
-
-   return len;
-}
-
-#define EXPECTING_PROTOCOL_VERSION 0
-#define EXPECTING_FIRST_REF 1
-#define EXPECTING_REF 2
-#define EXPECTING_SHALLOW 3
-
-/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
-static int process_protocol_version(void)
-{
-   switch (determine_protocol_version_client(packet_buffer)) {
-   case protocol_v1:
-   return 1;
-   case protocol_v0:
-   return 0;
-   default:
-   die("server is speaking an unknown protocol");
-   }
-}
+#define EXPECTING_FIRST_REF 0
+#define EXPECTING_REF 1
+#define EXPECTING_SHALLOW 2
+#define EXPECTING_DONE 3
 
 static void process_capabilities(int *len)
 {
@@ -230,28 +237,36 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
  struct oid_array *shallow_points)
 {
struct ref **orig_list = list;
+   int len = 0;
+   int state = EXPECTING_FIRST_REF;
+   struct packet_reader reader;
+   const char *arg;
 
-   /*
-* A hang-up after seeing some response from the other end
-* means that it is unexpected, as we know the other end is
-* willing to talk to us.  A hang-up before seeing any
-* response does not necessarily mean an 

[PATCH 01/26] pkt-line: introduce packet_read_with_status

[Brandon Williams]

The current pkt-line API encodes the status of a pkt-line read in the
length of the read content.  An error is indicated with '-1', a flush
with '0' (which can be confusing since a return value of '0' can also
indicate an empty pkt-line), and a positive integer for the length of
the read content otherwise.  This doesn't leave much room for allowing
the addition of additional special packets in the future.

To solve this introduce 'packet_read_with_status()' which reads a packet
and returns the status of the read encoded as an 'enum packet_status'
type.  This allows for easily identifying between special and normal
packets as well as errors.  It also enables easily adding a new special
packet in the future.

Signed-off-by: Brandon Williams 
---
 pkt-line.c | 55 ++-
 pkt-line.h | 15 +++
 2 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 2827ca772..8d7cd389f 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
 }
 
-int packet_read(int fd, char **src_buf, size_t *src_len,
-   char *buffer, unsigned size, int options)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
size_t *src_len,
+   char *buffer, unsigned size, 
int *pktlen,
+   int options)
 {
-   int len, ret;
+   int len;
char linelen[4];
 
-   ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
-   if (ret < 0)
-   return ret;
+   if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
+   return PACKET_READ_EOF;
+
len = packet_length(linelen);
if (len < 0)
die("protocol error: bad line length character: %.4s", linelen);
-   if (!len) {
+
+   if (len == 0) {
packet_trace("", 4, 0);
-   return 0;
+   return PACKET_READ_FLUSH;
+   } else if (len >= 1 && len <= 3) {
+   die("protocol error: bad line length character: %.4s", linelen);
}
+
len -= 4;
-   if (len >= size)
+   if ((len < 0) || ((unsigned)len >= size))
die("protocol error: bad line length %d", len);
-   ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
-   if (ret < 0)
-   return ret;
+
+   if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
+   return PACKET_READ_EOF;
 
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
len && buffer[len-1] == '\n')
@@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
 
buffer[len] = 0;
packet_trace(buffer, len, 0);
-   return len;
+   *pktlen = len;
+   return PACKET_READ_NORMAL;
+}
+
+int packet_read(int fd, char **src_buffer, size_t *src_len,
+   char *buffer, unsigned size, int options)
+{
+   enum packet_read_status status;
+   int pktlen;
+
+   status = packet_read_with_status(fd, src_buffer, src_len,
+buffer, size, ,
+options);
+   switch (status) {
+   case PACKET_READ_EOF:
+   pktlen = -1;
+   break;
+   case PACKET_READ_NORMAL:
+   break;
+   case PACKET_READ_FLUSH:
+   pktlen = 0;
+   break;
+   }
+
+   return pktlen;
 }
 
 static char *packet_read_line_generic(int fd,
diff --git a/pkt-line.h b/pkt-line.h
index 3dad583e2..06c468927 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t 
len, int fd_out);
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
*buffer, unsigned size, int options);
 
+/*
+ * Read a packetized line into a buffer like the 'packet_read()' function but
+ * returns an 'enum packet_read_status' which indicates the status of the read.
+ * The number of bytes read will be assigined to *pktlen if the status of the
+ * read was 'PACKET_READ_NORMAL'.
+ */
+enum packet_read_status {
+   PACKET_READ_EOF = -1,
+   PACKET_READ_NORMAL,
+   PACKET_READ_FLUSH,
+};
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
size_t *src_len,
+   char *buffer, unsigned size, 
int *pktlen,
+   int options);
+
 /*
  * Convenience wrapper for packet_read that is not gentle, and sets the
  * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 13/26] connect: request remote refs using v2

[Brandon Williams]

Teach the client to be able to request a remote's refs using protocol
v2.  This is done by having a client issue a 'ls-refs' request to a v2
server.

Signed-off-by: Brandon Williams 
---
 connect.c  | 101 -
 remote.h   |   4 ++
 t/t5701-protocol-v2.sh |  28 ++
 transport.c|   2 +-
 upload-pack.c  |   9 +++--
 5 files changed, 138 insertions(+), 6 deletions(-)
 create mode 100755 t/t5701-protocol-v2.sh

diff --git a/connect.c b/connect.c
index caa539b75..9badd403f 100644
--- a/connect.c
+++ b/connect.c
@@ -12,9 +12,11 @@
 #include "sha1-array.h"
 #include "transport.h"
 #include "strbuf.h"
+#include "version.h"
 #include "protocol.h"
 
 static char *server_capabilities;
+static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
 static const char *parse_feature_value(const char *, const char *, int *);
 
 static int check_ref(const char *name, unsigned int flags)
@@ -62,6 +64,33 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
 }
 
+static int server_supports_v2(const char *c, int die_on_error)
+{
+   int i;
+
+   for (i = 0; i < server_capabilities_v2.argc; i++) {
+   const char *out;
+   if (skip_prefix(server_capabilities_v2.argv[i], c, ) &&
+   (!*out || *out == '='))
+   return 1;
+   }
+
+   if (die_on_error)
+   die("server doesn't support '%s'", c);
+
+   return 0;
+}
+
+static void process_capabilities_v2(struct packet_reader *reader)
+{
+   while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+   argv_array_push(_capabilities_v2, reader->line);
+   }
+
+   if (reader->status != PACKET_READ_FLUSH)
+   die("protocol error");
+}
+
 enum protocol_version discover_version(struct packet_reader *reader)
 {
enum protocol_version version = protocol_unknown_version;
@@ -85,7 +114,7 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
/* Maybe process capabilities here, at least for v2 */
switch (version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   process_capabilities_v2(reader);
break;
case protocol_v1:
/* Read the peeked version line */
@@ -292,6 +321,76 @@ struct ref **get_remote_heads(struct packet_reader *reader,
return list;
 }
 
+static int process_ref_v2(const char *line, struct ref ***list)
+{
+   int ret = 1;
+   int i = 0;
+   struct object_id old_oid;
+   struct ref *ref;
+   struct string_list line_sections = STRING_LIST_INIT_DUP;
+
+   if (string_list_split(_sections, line, ' ', -1) < 2) {
+   ret = 0;
+   goto out;
+   }
+
+   if (get_oid_hex(line_sections.items[i++].string, _oid)) {
+   ret = 0;
+   goto out;
+   }
+
+   ref = alloc_ref(line_sections.items[i++].string);
+
+   if (i < line_sections.nr)
+   ref->symref = xstrdup(line_sections.items[i++].string);
+
+   oidcpy(>old_oid, _oid);
+   **list = ref;
+   *list = >next;
+
+out:
+   string_list_clear(_sections, 0);
+   return ret;
+}
+
+struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
+struct ref **list, int for_push,
+const struct argv_array *ref_patterns)
+{
+   int i;
+   *list = NULL;
+
+   /* Check that the server supports the ls-refs command */
+   /* Issue request for ls-refs */
+   if (server_supports_v2("ls-refs", 1))
+   packet_write_fmt(fd_out, "command=ls-refs\n");
+
+   if (server_supports_v2("agent", 0))
+   packet_write_fmt(fd_out, "agent=%s", git_user_agent_sanitized());
+
+   packet_delim(fd_out);
+   /* When pushing we don't want to request the peeled tags */
+   if (!for_push)
+   packet_write_fmt(fd_out, "peel\n");
+   packet_write_fmt(fd_out, "symrefs\n");
+   for (i = 0; ref_patterns && i < ref_patterns->argc; i++) {
+   packet_write_fmt(fd_out, "ref-pattern %s\n",
+ref_patterns->argv[i]);
+   }
+   packet_flush(fd_out);
+
+   /* Process response from server */
+   while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+   if (!process_ref_v2(reader->line, ))
+   die("invalid ls-refs response: %s", reader->line);
+   }
+
+   if (reader->status != PACKET_READ_FLUSH)
+   die("protocol error");
+
+   return list;
+}
+
 static const char *parse_feature_value(const char *feature_list, const char 
*feature, int *lenp)
 {
int len;
diff --git a/remote.h b/remote.h
index 2016461df..21d0c776c 100644
--- a/remote.h
+++ b/remote.h
@@ -151,10 +151,14 @@ 

[PATCH 08/26] connect: discover protocol version outside of get_remote_heads

[Brandon Williams]

In order to prepare for the addition of protocol_v2 push the protocol
version discovery outside of 'get_remote_heads()'.  This will allow for
keeping the logic for processing the reference advertisement for
protocol_v1 and protocol_v0 separate from the logic for protocol_v2.

Signed-off-by: Brandon Williams 
---
 builtin/fetch-pack.c | 16 +++-
 builtin/send-pack.c  | 17 +++--
 connect.c| 15 ---
 connect.h|  3 +++
 remote-curl.c| 20 ++--
 remote.h |  5 +++--
 transport.c  | 24 +++-
 7 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 366b9d13f..85d4faf76 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -4,6 +4,7 @@
 #include "remote.h"
 #include "connect.h"
 #include "sha1-array.h"
+#include "protocol.h"
 
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct fetch_pack_args args;
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
+   struct packet_reader reader;
 
packet_trace_identity("fetch-pack");
 
@@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
if (!conn)
return args.diag_url ? 0 : 1;
}
-   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
+
+   packet_reader_init(, fd[0], NULL, 0,
+  PACKET_READ_CHOMP_NEWLINE |
+  PACKET_READ_GENTLE_ON_EOF);
+
+   switch (discover_version()) {
+   case protocol_v1:
+   case protocol_v0:
+   get_remote_heads(, , 0, NULL, );
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
 
ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
 , pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fc4f0bb5f..83cb125a6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -14,6 +14,7 @@
 #include "sha1-array.h"
 #include "gpg-interface.h"
 #include "gettext.h"
+#include "protocol.h"
 
 static const char * const send_pack_usage[] = {
N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
@@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
int progress = -1;
int from_stdin = 0;
struct push_cas_option cas = {0};
+   struct packet_reader reader;
 
struct option options[] = {
OPT__VERBOSITY(),
@@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.verbose ? CONNECT_VERBOSE : 0);
}
 
-   get_remote_heads(fd[0], NULL, 0, _refs, REF_NORMAL,
-_have, );
+   packet_reader_init(, fd[0], NULL, 0,
+  PACKET_READ_CHOMP_NEWLINE |
+  PACKET_READ_GENTLE_ON_EOF);
+
+   switch (discover_version()) {
+   case protocol_v1:
+   case protocol_v0:
+   get_remote_heads(, _refs, REF_NORMAL,
+_have, );
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
 
transport_verify_remote_names(nr_refspecs, refspecs);
 
diff --git a/connect.c b/connect.c
index 03bbb74e4..1787b0212 100644
--- a/connect.c
+++ b/connect.c
@@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
 }
 
-static enum protocol_version discover_version(struct packet_reader *reader)
+enum protocol_version discover_version(struct packet_reader *reader)
 {
enum protocol_version version = protocol_unknown_version;
 
@@ -231,7 +231,7 @@ static int process_shallow(int len, struct oid_array 
*shallow_points)
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct ref **get_remote_heads(struct packet_reader *reader,
  struct ref **list, unsigned int flags,
  struct oid_array *extra_have,
  struct oid_array *shallow_points)
@@ -239,23 +239,16 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
struct ref **orig_list = list;
int len = 0;
int state = EXPECTING_FIRST_REF;
-   struct packet_reader reader;
const char *arg;
 
-   packet_reader_init(, in, src_buf, src_len,
-  PACKET_READ_CHOMP_NEWLINE |
-  PACKET_READ_GENTLE_ON_EOF);
-
-   discover_version();
-
*list = NULL;
 

[PATCH 02/26] pkt-line: introduce struct packet_reader

[Brandon Williams]

Sometimes it is advantageous to be able to peek the next packet line
without consuming it (e.g. to be able to determine the protocol version
a server is speaking).  In order to do that introduce 'struct
packet_reader' which is an abstraction around the normal packet reading
logic.  This enables a caller to be able to peek a single line at a time
using 'packet_reader_peek()' and having a caller consume a line by
calling 'packet_reader_read()'.

Signed-off-by: Brandon Williams 
---
 pkt-line.c | 59 +++
 pkt-line.h | 57 +
 2 files changed, 116 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 8d7cd389f..98c2d7d68 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -406,3 +406,62 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf 
*sb_out)
}
return sb_out->len - orig_len;
 }
+
+/* Packet Reader Functions */
+void packet_reader_init(struct packet_reader *reader, int fd,
+   char *src_buffer, size_t src_len,
+   int options)
+{
+   memset(reader, 0, sizeof(*reader));
+
+   reader->fd = fd;
+   reader->src_buffer = src_buffer;
+   reader->src_len = src_len;
+   reader->buffer = packet_buffer;
+   reader->buffer_size = sizeof(packet_buffer);
+   reader->options = options;
+}
+
+enum packet_read_status packet_reader_read(struct packet_reader *reader)
+{
+   if (reader->line_peeked) {
+   reader->line_peeked = 0;
+   return reader->status;
+   }
+
+   reader->status = packet_read_with_status(reader->fd,
+>src_buffer,
+>src_len,
+reader->buffer,
+reader->buffer_size,
+>pktlen,
+reader->options);
+
+   switch (reader->status) {
+   case PACKET_READ_EOF:
+   reader->pktlen = -1;
+   reader->line = NULL;
+   break;
+   case PACKET_READ_NORMAL:
+   reader->line = reader->buffer;
+   break;
+   case PACKET_READ_FLUSH:
+   reader->pktlen = 0;
+   reader->line = NULL;
+   break;
+   }
+
+   return reader->status;
+}
+
+enum packet_read_status packet_reader_peek(struct packet_reader *reader)
+{
+   /* Only allow peeking a single line */
+   if (reader->line_peeked)
+   return reader->status;
+
+   /* Peek a line by reading it and setting peeked flag */
+   packet_reader_read(reader);
+   reader->line_peeked = 1;
+   return reader->status;
+}
diff --git a/pkt-line.h b/pkt-line.h
index 06c468927..c446e886a 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -111,6 +111,63 @@ char *packet_read_line_buf(char **src_buf, size_t 
*src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+struct packet_reader {
+   /* source file descriptor */
+   int fd;
+
+   /* source buffer and its size */
+   char *src_buffer;
+   size_t src_len;
+
+   /* buffer that pkt-lines are read into and its size */
+   char *buffer;
+   unsigned buffer_size;
+
+   /* options to be used during reads */
+   int options;
+
+   /* status of the last read */
+   enum packet_read_status status;
+
+   /* length of data read during the last read */
+   int pktlen;
+
+   /* the last line read */
+   const char *line;
+
+   /* indicates if a line has been peeked */
+   int line_peeked;
+};
+
+/*
+ * Initialize a 'struct packet_reader' object which is an
+ * abstraction around the 'packet_read_with_status()' function.
+ */
+extern void packet_reader_init(struct packet_reader *reader, int fd,
+  char *src_buffer, size_t src_len,
+  int options);
+
+/*
+ * Perform a packet read and return the status of the read.
+ * The values of 'pktlen' and 'line' are updated based on the status of the
+ * read as follows:
+ *
+ * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
+ * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
+ *'line' is set to point at the read line
+ * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
+ */
+extern enum packet_read_status packet_reader_read(struct packet_reader 
*reader);
+
+/*
+ * Peek the next packet line without consuming it and return the status.
+ * The next call to 'packet_reader_read()' will perform a read of the same line
+ * that was peeked, consuming the line.
+ *
+ * Only a single line can be peeked at a time.
+ */
+extern enum packet_read_status packet_reader_peek(struct packet_reader 
*reader);
+
 #define 

Re: [PATCH v5 00/34] Add directory rename detection to git

[Elijah Newren]

On Wed, Dec 27, 2017 at 8:13 PM, Elijah Newren  wrote:
> This patchset introduces directory rename detection to merge-recursive.  See
>   https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
> for the first series (including design considerations, etc.), and follow-up
> series can be found at
>   https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
>   https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/
>   https://public-inbox.org/git/20171129014237.32570-1-new...@gmail.com/
>
> Changes since v4:
>   * Squashed Junio's GETTEXT_POISON fixes into the appropriate commits

As per Jonathan's request[1], shamelessly re-sending Stefan's request
for further review.  :-)

Quoting Stefan:

"I have reviewed the first three patches (which could form an
independent series)
that it would warrant a Reviewed-By: Stefan Beller 

While I reviewed the earlier versions of the later patches, I would
prefer if there is another reviewer for these as it seems like a bigger
contribution at a core functionality.

I cc'd some people who were active in some form of rename detection
work earlier; could you review this series, please?"

My note: Stefan also looked through the testcases pretty closely and
even suggested additional tests, which would account for another 11
patches or so, but extra eyes on any part of the series always
welcome.

Thanks!
Elijah

[1] 
https://public-inbox.org/git/20180102221916.ge131...@aiede.mtv.corp.google.com/

Re: [PATCH 4/4] clone: do not clean up directories we didn't create

[Jeff King]

On Tue, Jan 02, 2018 at 05:49:45PM -0500, Eric Sunshine wrote:

> > diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
> > @@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the 
> > directory' '
> > +test_expect_success 'failed clone into empty leaves directory (separate, 
> > git)' '
> > +   mkdir -p empty-git &&
> > +   corrupt_repo &&
> > +   test_must_fail git clone --separate-git-dir empty-git foo no-wt &&
> > +   test_dir_is_empty empty-git &&
> > +   test_path_is_missing no-wt
> > +'
> > +
> > +test_expect_success 'failed clone into empty leaves directory (separate, 
> > git)' '
> > +   mkdir -p empty-wt &&
> > +   corrupt_repo &&
> > +   test_must_fail git clone --separate-git-dir no-git foo empty-wt &&
> > +   test_path_is_missing no-git &&
> > +   test_dir_is_empty empty-wt
> > +'
> 
> The final two tests seem to have the same title...

Oops. The second one should be "wt" (since the idea was to flip the
logic from the previous). Like so:

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 5cd94d5558..4a1a912e03 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -89,7 +89,7 @@ test_expect_success 'failed clone into empty leaves directory 
(separate, git)' '
test_path_is_missing no-wt
 '
 
-test_expect_success 'failed clone into empty leaves directory (separate, git)' 
'
+test_expect_success 'failed clone into empty leaves directory (separate, wt)' '
mkdir -p empty-wt &&
corrupt_repo &&
test_must_fail git clone --separate-git-dir no-git foo empty-wt &&

Re: [PATCH 2/2] travis-ci: check that all build artifacts are .gitignore-d

[SZEDER Gábor]

On Tue, Jan 2, 2018 at 8:40 PM, Lars Schneider  wrote:
>
>> On 31 Dec 2017, at 17:02, SZEDER Gábor  wrote:
>>
>> Every once in a while our explicit .gitignore files get out of sync
>> when our build process learns to create new artifacts, like test
>> helper executables, but the .gitignore files are not updated
>> accordingly.
>>
>> Use Travis CI to help catch such issues earlier: check that there are
>> no untracked files at the end of any build jobs building Git (i.e. the
>> 64 bit Clang and GCC Linux and OSX build jobs, plus the GETTEXT_POISON
>> and 32 bit Linux build jobs) or its documentation, and fail the build
>> job if there are any present.
>>
>> Signed-off-by: SZEDER Gábor 
>> ---
>> ci/lib-travisci.sh   | 10 ++
>> ci/run-linux32-docker.sh |  2 ++
>> ci/run-tests.sh  |  2 ++
>> ci/test-documentation.sh |  6 ++
>> 4 files changed, 20 insertions(+)
>>
>> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
>> index 1543b7959..07f27c727 100755
>> --- a/ci/lib-travisci.sh
>> +++ b/ci/lib-travisci.sh
>> @@ -67,6 +67,16 @@ skip_good_tree () {
>>   exit 0
>> }
>>
>> +check_unignored_build_artifacts ()
>> +{
>> + ! git ls-files --other --exclude-standard --error-unmatch \
>> + -- ':/*' 2>/dev/null ||
>
> What does "-- ':/*'" do?

It makes the whole thing work :)
':/*' is a pattern matching all paths, and '--others --exclude-standard'
limit the command to list files that are both untracked and unignored.
'--error-unmatch' causes the command to error out if any of the files
given on the command line doesn't match anything in the working tree,
in this case if there are no untracked-unignored files.  Without a path
given on the cmdline '--error-unmatch' has basically no effect[1].

In a clean worktree:

  $ git ls-files --other --exclude-standard --error-unmatch ; echo $?
  0
  $ git ls-files --other --exclude-standard --error-unmatch ':/*' ; echo $?
  error: pathspec ':/*' did not match any file(s) known to git.
  Did you forget to 'git add'?
  1

The disambiguating double-dash is not really necessary, because the :/*
pattern can't be confused with any --options, but doesn't hurt, either.

> Plus, why do you redirect stderr?

To prevent the above error message from appearing in the trace log of a
successful build.


[1] - Which makes me think whether we should consider '--error-unmatch'
  without any paths given on the command line as an error.
  On a related note: that error message doesn't seem to make much
  sense with '--other'...

RE: [Bug] NO_INTPTR_T not being honoured in compat/regex/regcomp.c

[Randall S. Becker]

On January 1, 2018 4:51 PM Eric Sunshine wrote:
> On Mon, Jan 1, 2018 at 4:21 PM, Randall S. Becker
>  wrote:
> > * I have defined NO_INTPTR_T = UnfortunatelyYes in config.mak.uname
> > for my platform. The c99 compiler I have does not define it.
> > * The code compiles except for compat/regex/regcomp.c - not sure why
> > this is being used since I have also *not* defined NO_REGEX because
> > regex is sane here.
> 
> Presumably you're compiling for NonStop? config.mak.uname defines
> NO_REGEX for that platform (it also defines NO_INTPTR_T). git-blame points
> at 6c109904bc (Port to HP NonStop, 2012-09-19).

I found one teeny thing. NO_REGEX=NeedsStartEnd is what should be in 
config.mak.uname to get it right rather than ignoring NO_REGEX completely. I 
will commit that after my porting adventure. This still requires regcomp.c to 
be used and misses intptr_t, so still looking for a solution. I'm also 
currently at commit 7234152 and dealing with a bunch of new dependencies that 
weren't there at 2.8.5.

Cheers,
Randall

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

Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index bf8b602901..ac59524a7f 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>>   ie_match_stat(o->src_index, old, , 
>> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>>   update |= CE_UPDATE;
>>   }
>> + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
>> + !verify_uptodate(old, o))
>> + update |= CE_UPDATE;
>>   add_entry(o, old, update, 0);
>>   return 0;
>>   }
>
> This is more sensible than the previous one that broke same() by
> unconditionally checking the working tree state, even when the
> machinery is told not to look at the working tree.
>
> The code in the pre-context of this hunk, (i.e. the original one you
> are half-way mimicking), we realize that the original cache entry
> and the cache entry we are checking out are exactly the same and we
> overwrite when we know that the path in the working tree is dirty,
> and we are not told to "skip" that path in the working tree
> (i.e. sparse checkout).  That happens only when we are told to
> o->update and o->reset.
>
> Shouldn't we be setting the update bit only when o->update is in
> effect,

Yes, we should.

> just like we can see in the pre-context of this hunk?  I do
> not know if we need to require o->reset and/or need to pay attention
> to the skip worktree bit in this new case.

verify_uptodate takes care of the worktree bits
("o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))")

o->reset is taken care of inside the nested verify_uptodate_1 function via

...
else if (o->reset || ce_uptodate(ce))
return 0;

So I'd think we only need to toss in the o->update check.

And that additional check would only be a performance optimization
as it would omit the check in case we do not want to update.
(verify_uptodate is expensive for submodules)

Re: [PATCH 4/4] clone: do not clean up directories we didn't create

[Eric Sunshine]

On Tue, Jan 2, 2018 at 4:11 PM, Jeff King  wrote:
> [...]
> Because we know that the only directory we'll write into is
> an empty one, we can handle this case by just passing the
> KEEP_TOPLEVEL flag to our recursive delete (if we could
> write into populated directories, we'd have to keep track of
> what we wrote and what we did not, which would be much
> harder).
>
> Note that we need to handle the work-tree and git-dir
> separately, though, as only one might exist (and the new
> tests in t5600 cover all cases).
>
> Reported-by: Stephan Janssen 
> Signed-off-by: Jeff King 
> ---
> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
> @@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the 
> directory' '
> +test_expect_success 'failed clone into empty leaves directory (separate, 
> git)' '
> +   mkdir -p empty-git &&
> +   corrupt_repo &&
> +   test_must_fail git clone --separate-git-dir empty-git foo no-wt &&
> +   test_dir_is_empty empty-git &&
> +   test_path_is_missing no-wt
> +'
> +
> +test_expect_success 'failed clone into empty leaves directory (separate, 
> git)' '
> +   mkdir -p empty-wt &&
> +   corrupt_repo &&
> +   test_must_fail git clone --separate-git-dir no-git foo empty-wt &&
> +   test_path_is_missing no-git &&
> +   test_dir_is_empty empty-wt
> +'

The final two tests seem to have the same title...

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

[Jonathan Nieder]

Stefan Beller wrote:
> On Thu, Dec 28, 2017 at 11:02 AM, Junio C Hamano  wrote:
>> Elijah Newren  writes:

>>> surprised by the branch name, though.  Was 'ew/' a typo,
>>
>> Blush X-<.  Yes it is a typo.
>
> Note on that series:
> I have reviewed the first three patches (which could form an independent 
> series)
> that it would warrant a Reviewed-By: Stefan Beller 
>
> While I reviewed the earlier versions of the later patches, I would
> prefer if there is another reviewer for these as it seems like a bigger
> contribution at a core functionality.
>
> I cc'd some people who were active in some form of rename detection
> work earlier; could you review this series, please?

I'm missing context about which series you mean (though I think I can
guess).  Do you mind resending this request-for-review in a reply to
the patch thread?

That way, the request would be in context where my mail reader can
bring up the thread and it's easy for others to see that the request
happened, all in one place.

Thanks,
Jonathan

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

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:
> On 12/21/2017 3:43 PM, Jonathan Nieder wrote:

>> I also wonder if there's a way to achieve the same benefit without
>> having it be configurable.  E.g. if a branch is way behind, couldn't
>> we terminate the walk early to get the same bounded cost per branch
>> without requiring configuration?
>
> I created a config setting because we don't want to force users to
> type "git status --no-ahead-behind" on every interactive command to
> get the benefit of it.  I guess we could ask them to alias it, if we
> don't want a config setting.
>
> Also, I didn't want to change the time-tested behavior that users see,
> so I didn't want to change the algorithm in any way -- just not call it.

I'm not too worried about people relying on the time-tested behavior
in this case.  It's a convenience feature for human users --- any
scripts looking for this information would be likely to use a more
convenient command like rev-list.

The one exception is "git status --porcelain=v2".  For that command,
scripts are likely to expect to be able to parse the "+
-" line.  Alas, guarding it with a config doesn't help such
scripts --- they still need to be able to cope with the new behavior.

git-status(1) says:

Parsers should ignore headers that they don't recognize.

so introducing a new line like "# branch.matchesupstream (true |
false)" like you did seems reasonable enough.  I *suspect* it should
be okay to omit the "# branch.ab" line as long as the script didn't
explicitly pass --ahead-behind but that depends on whether any scripts
in the wild were relying on the "# branch.ab" line being present.

> Would it make more sense to name this something like "status.aheadBehindLimit"
> where 0 would mean no limit and match existing behavior and a positive
> number be the number of commits we are allowed to search before giving up.

Sounds good to me.

Thanks,
Jonathan

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

[Jeff Hostetler]

On 12/21/2017 3:43 PM, Jonathan Nieder wrote:

Hi,

Jeff Hostetler wrote:


Created core.aheadbehind config setting and core_ahead_behind
global variable.  This value defaults to true.

This value will be used in the next few commits as the default value
for the --ahead-behind parameter.

Signed-off-by: Jeff Hostetler 
---
  Documentation/config.txt | 8 
  cache.h  | 1 +
  config.c | 5 +
  environment.c| 1 +
  4 files changed, 15 insertions(+)


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

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

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


I'll look at restacking the commits and make this later in the
series.  I have tests in a later commit that uses the config setting,
but at this point nothing uses it, so I didn't add any tests for it.
So maybe with a restacking, I can split up the tests to go with this
change.



[...]

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfa..c78d6be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -895,6 +895,14 @@ core.abbrev::
abbreviated object names to stay unique for some time.
The minimum length is 4.
  
+core.aheadbehind::

+   If true, tells commands like status and branch to print ahead and
+   behind counts for the branch relative to its upstream branch.
+   This computation may be very expensive when there is a great
+   distance between the two branches.  If false, these commands
+   only print that the two branches refer to different commits.
+   Defaults to true.


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


I wasn't sure where to put it after the earlier conversation in V1.


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


I created a config setting because we don't want to force users to
type "git status --no-ahead-behind" on every interactive command to
get the benefit of it.  I guess we could ask them to alias it, if we
don't want a config setting.

Also, I didn't want to change the time-tested behavior that users see,
so I didn't want to change the algorithm in any way -- just not call it.


Would it make more sense to name this something like "status.aheadBehindLimit"
where 0 would mean no limit and match existing behavior and a positive
number be the number of commits we are allowed to search before giving up.
A value of 1 would match the "different" case in the current patch series.
For most users, a value of say 1000 would be sufficient most of the time
(and report at most a 999 a/b value), but keep us from going off into the
weeds for those ridiculous cases with very old branches.


Thanks,
Jeff

Re: [PATCH] git-archive: accept --owner and --group like GNU tar

[René Scharfe]

Am 02.01.2018 um 07:58 schrieb suzuki toshiya:
> Dear René ,
> 
> René Scharfe wrote:
>> Am 29.12.2017 um 15:05 schrieb suzuki toshiya:
>>> The ownership of files created by git-archive is always
>>> root:root. Add --owner and --group options which work
>>> like the GNU tar equivalent to allow overriding these
>>> defaults.
>>
>> In which situations do you use the new options?
>>
>> (The sender would need to know the names and/or IDs on the receiving
>> end.  And the receiver would need to be root to set both IDs, or be a
>> group member to set the group ID; I guess the latter is more common.)
> 
> Thank you for asking the background.
> 
> In the case that additional contents are appended to the tar file
> generated by git-archive, the part by git-archive and the part
> appended by common tar would have different UID/GID, because common
> tar preserves the UID/GID of the original files.
> 
> Of cource, both of GNU tar and bsdtar have the options to set
> UID/GID manually, but their syntax are different.
> 
> In the recent source package of poppler (poppler.freedesktop.org),
> there are 2 sets of UID/GIDs are found:
> https://poppler.freedesktop.org/poppler-0.62.0.tar.xz
> 
> I've discussed with the maintainers of poppler, and there was a
> suggestion to propose a feature to git.
> https://lists.freedesktop.org/archives/poppler/2017-December/012739.html

So this is in the context of generating release tarballs that contain
untracked files as well.  That's done in Git's own Makefile, too:

  dist: git-archive$(X) configure
  ./git-archive --format=tar \
  --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
  @mkdir -p $(GIT_TARNAME)
  @cp configure $(GIT_TARNAME)
  @echo $(GIT_VERSION) > $(GIT_TARNAME)/version
  @$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version
  $(TAR) rf $(GIT_TARNAME).tar \
  $(GIT_TARNAME)/configure \
  $(GIT_TARNAME)/version \
  $(GIT_TARNAME)/git-gui/version
  @$(RM) -r $(GIT_TARNAME)
  gzip -f -9 $(GIT_TARNAME).tar

Having files with different owners and groups is a non-issue when
extracting with --no-same-owner, which is the default for regular users.
I assume this covers most use cases in the wild.

The generated archive leaks the IDs of the user preparing the archive in
the appended entries for untracked files.  I think that's more of a
concern.  Publishing a valid non-root username on your build system may
invite attackers.

Changing the build procedure to set owner and group to root as well as
UID and GID to zero seems like a better idea.  This is complicated by
the inconsistent command line options for GNU tar and bsdtar, as you
mentioned.

So how about making it possible to append untracked files using git
archive?  This could simplify the dist target for Git as well.  It's
orthogonal to adding the ability to explicitly specify owner and group,
but might suffice in most (all?) cases.

Not sure what kind of file name transformation abilities would be
needed and how to package them nicely.  The --transform option of GNU
tar with its sed replace expressions seems quite heavy for me.  With
poppler it's only used to add the --prefix string; I'd expect that to
be done for all appended files anyway.

Perhaps something like --append-file= with no transformation
feature is already enough for most cases?

>> Would it make sense to support the new options for ZIP files as well?
> 
> I was not aware of the availability of UID/GID in pkzip file format...
> Oh, checking APPNOTE.TXT ( 
> https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT ),
> there is a storage! (see 4.5.7-Unix Extra Field). But it seems
> that current git-archive emits pkzip without the field.

Indeed.  Git doesn't track owners and groups, so it doesn't make
sense to emit that kind of information so far.  If git archive grows
options to specify such meta-information then it should be supported
by all archive formats (or documented to be tar-specific).

However, the UNIX Extra Field in ZIP archives only allows to store
UID and GID (no names), which is useless unless the sender knows the
ID range of the receiver -- which is unlikely when distributing
software on the Internet.  And even then it won't work with Windows,
which has long Security Identifiers (SIDs) instead.

So these are more advantages for letting git archive append untracked
files: It's format-agnostic and more portable.


[snipped interesting history of security-related tar options]

Btw. I like how bsdtar has --insecure as a synonym for -p (preserve
file permissions when extracting).  It's a bit sad that this is still
the default for root, though.  OpenBSD cut that behavior out of their
tar almost 20 years ago.  (An evil tar archive could be used to fill
the quota of unsuspecting users, or add setuid executables.)

>>> +#if ULONG_MAX > 0xUL
>>> +    /*
>>> + * --owner, --group rejects uid/gid 

Re: [PATCH v3 0/6] Renames in git-status "changed not staged" section

[Jeff Hostetler]

On 12/27/2017 5:18 AM, Nguyễn Thái Ngọc Duy wrote:

v3 more or less goes back to v1 after my discussion with Igor about
porcelain formats. So 7/7 is not needed anymore. 4/7 becomes 5/6. The
meat is still in 6/6, now with some more updates in git-status.txt and
to address the comment from Torsten.

Nguyễn Thái Ngọc Duy (6):
   t2203: test status output with porcelain v2 format
   Use DIFF_DETECT_RENAME for detect_rename assignments
   wt-status.c: coding style fix
   wt-status.c: catch unhandled diff status codes
   wt-status.c: rename rename-related fields in wt_status_change_data
   wt-status.c: handle worktree renames

  Documentation/git-status.txt | 23 ++--
  builtin/commit.c |  2 +-
  diff.c   |  2 +-
  t/t2203-add-intent.sh| 72 ++
  wt-status.c  | 83 
  wt-status.h  |  5 +--
  6 files changed, 143 insertions(+), 44 deletions(-)




Signed-off-by: Jeff Hostetler 

Re: [PATCH] status: handle worktree renames

[Jeff Hostetler]

On 12/27/2017 1:12 PM, Junio C Hamano wrote:

Duy Nguyen  writes:


Or we disable rename-from-worktree when porcelain v2 is requested (and
optionally introduce v3 to support it). Jeff, any preference?


Sorry for the delay, I was on vacation last week.

I like the "R." and ".R" lines in your 3rd patch series as that keeps
porcelain V2 output consistent with the changes that you added to plain
and porcelain V1 output.  All 3 formats now report 2 types of renames.
Having a "RR" line would be more consistent with a "MM" line, but I
don't think that happens often enough to define a porcelain V3 format
with a 3 path row variant.

I like that we can now show "unstaged renames" (in all 3 formats)
as I think that is less confusing to the novice user than a
new-file/delete pair.


Having said that, I am a little concerned about us changing V1 and
V2 output at all -- we are breaking the porcelain contract we have
with scripts.  I like the change, so I'm not bothered about it, but
others may think differently.


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)

Jeff

[PATCH 4/4] clone: do not clean up directories we didn't create

[Jeff King]

Once upon a time, git-clone would refuse to write into a
directory that it did not itself create. The cleanup
routines for a failed clone could therefore just remove the
git and worktree dirs completely.

In 55892d2398 (Allow cloning to an existing empty directory,
2009-01-11), we learned to write into an existing directory.
Which means that doing:

  mkdir foo
  git clone will-fail foo

ends up deleting foo. This isn't a huge catastrophe, since
by definition foo must be empty. But it's somewhat
confusing; we should leave the filesystem as we found it.

Because we know that the only directory we'll write into is
an empty one, we can handle this case by just passing the
KEEP_TOPLEVEL flag to our recursive delete (if we could
write into populated directories, we'd have to keep track of
what we wrote and what we did not, which would be much
harder).

Note that we need to handle the work-tree and git-dir
separately, though, as only one might exist (and the new
tests in t5600 cover all cases).

Reported-by: Stephan Janssen 
Signed-off-by: Jeff King 
---
 builtin/clone.c   | 20 
 t/t5600-clone-fail-cleanup.sh | 56 +++
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 04b0d7283f..284651797e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -473,7 +473,9 @@ static void clone_local(const char *src_repo, const char 
*dest_repo)
 }
 
 static const char *junk_work_tree;
+static int junk_work_tree_flags;
 static const char *junk_git_dir;
+static int junk_git_dir_flags;
 static enum {
JUNK_LEAVE_NONE,
JUNK_LEAVE_REPO,
@@ -502,12 +504,12 @@ static void remove_junk(void)
 
if (junk_git_dir) {
strbuf_addstr(, junk_git_dir);
-   remove_dir_recursively(, 0);
+   remove_dir_recursively(, junk_git_dir_flags);
strbuf_reset();
}
if (junk_work_tree) {
strbuf_addstr(, junk_work_tree);
-   remove_dir_recursively(, 0);
+   remove_dir_recursively(, junk_work_tree_flags);
}
strbuf_release();
 }
@@ -972,14 +974,24 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (safe_create_leading_directories_const(work_tree) < 0)
die_errno(_("could not create leading directories of 
'%s'"),
  work_tree);
-   if (!dest_exists && mkdir(work_tree, 0777))
+   if (dest_exists)
+   junk_work_tree_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
+   else if (mkdir(work_tree, 0777))
die_errno(_("could not create work tree dir '%s'"),
  work_tree);
junk_work_tree = work_tree;
set_git_work_tree(work_tree);
}
 
-   junk_git_dir = real_git_dir ? real_git_dir : git_dir;
+   if (real_git_dir) {
+   if (dir_exists(real_git_dir))
+   junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
+   junk_git_dir = real_git_dir;
+   } else {
+   if (dest_exists)
+   junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
+   junk_git_dir = git_dir;
+   }
if (safe_create_leading_directories_const(git_dir) < 0)
die(_("could not create leading directories of '%s'"), git_dir);
 
diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 7b2a8052f8..5cd94d5558 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -7,10 +7,21 @@ test_description='test git clone to cleanup after failure
 
 This test covers the fact that if git clone fails, it should remove
 the directory it created, to avoid the user having to manually
-remove the directory before attempting a clone again.'
+remove the directory before attempting a clone again.
+
+Unless the directory already exists, in which case we clean up only what we
+wrote.
+'
 
 . ./test-lib.sh
 
+corrupt_repo () {
+   test_when_finished "rmdir foo/.git/objects.bak" &&
+   mkdir foo/.git/objects.bak/ &&
+   test_when_finished "mv foo/.git/objects.bak/* foo/.git/objects/" &&
+   mv foo/.git/objects/* foo/.git/objects.bak/
+}
+
 test_expect_success 'clone of non-existent source should fail' '
test_must_fail git clone foo bar
 '
@@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the 
directory' '
 '
 
 test_expect_success 'failed clone --separate-git-dir should not leave any 
directories' '
-   test_when_finished "rmdir foo/.git/objects.bak" &&
-   mkdir foo/.git/objects.bak/ &&
-   test_when_finished "mv foo/.git/objects.bak/* foo/.git/objects/" &&
-   mv foo/.git/objects/* foo/.git/objects.bak/ &&
+   corrupt_repo &&
test_must_fail git clone --separate-git-dir gitdir foo 

[PATCH 3/4] clone: factor out dir_exists() helper

[Jeff King]

Two parts of git-clone's setup logic check whether a
directory exists, and they both call stat directly with the
same scratch "struct stat" buffer. Let's pull that into a
helper, which has a few advantages:

  - it makes the purpose of the stat calls more obvious

  - it makes it clear that we don't care about the
information in "buf" remaining valid

  - if we later decide to make the check more robust (e.g.,
complaining about non-directories), we can do it in one
place

Note that we could just use file_exists() for this, which
has identical code. But we specifically care about
directories, so this future-proofs us against that function
later getting more picky about seeing actual files.

Signed-off-by: Jeff King 
---
 builtin/clone.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 2da71db107..04b0d7283f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -863,10 +863,15 @@ static void dissociate_from_references(void)
free(alternates);
 }
 
+static int dir_exists(const char *path)
+{
+   struct stat sb;
+   return !stat(path, );
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
int is_bundle = 0, is_local;
-   struct stat buf;
const char *repo_name, *repo, *work_tree, *git_dir;
char *path, *dir;
int dest_exists;
@@ -938,7 +943,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
dir = guess_dir_name(repo_name, is_bundle, option_bare);
strip_trailing_slashes(dir);
 
-   dest_exists = !stat(dir, );
+   dest_exists = dir_exists(dir);
if (dest_exists && !is_empty_dir(dir))
die(_("destination path '%s' already exists and is not "
"an empty directory."), dir);
@@ -949,7 +954,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
work_tree = NULL;
else {
work_tree = getenv("GIT_WORK_TREE");
-   if (work_tree && !stat(work_tree, ))
+   if (work_tree && dir_exists(work_tree))
die(_("working tree '%s' already exists."), work_tree);
}
 
-- 
2.16.0.rc0.384.gc477e89267

[PATCH 2/4] t5600: modernize style

[Jeff King]

This is an old script which could use some updating before
we add to it:

  - use the standard line-breaking:

  test_expect_success 'title' '
  body
  '

  - run all code inside test_expect blocks to catch
unexpected failures in setup steps

  - use "test_commit -C" instead of manually entering
sub-repo

  - use test_when_finished for cleanup steps

  - test_path_is_* as appropriate

Signed-off-by: Jeff King 
---
 t/t5600-clone-fail-cleanup.sh | 48 ++-
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index f23f92e5a7..7b2a8052f8 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -11,42 +11,44 @@ remove the directory before attempting a clone again.'
 
 . ./test-lib.sh
 
-test_expect_success \
-'clone of non-existent source should fail' \
-'test_must_fail git clone foo bar'
+test_expect_success 'clone of non-existent source should fail' '
+   test_must_fail git clone foo bar
+'
 
-test_expect_success \
-'failed clone should not leave a directory' \
-'! test -d bar'
+test_expect_success 'failed clone should not leave a directory' '
+   test_path_is_missing bar
+'
 
-# Need a repo to clone
-test_create_repo foo
+test_expect_success 'create a repo to clone' '
+   test_create_repo foo
+'
 
-# create some objects so that we can corrupt the repo later
-(cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 
2>&1)
+test_expect_success 'create objects in repo for later corruption' '
+   test_commit -C foo file
+'
 
 # source repository given to git clone should be relative to the
 # current path not to the target dir
-test_expect_success \
-'clone of non-existent (relative to $PWD) source should fail' \
-'test_must_fail git clone ../foo baz'
+test_expect_success 'clone of non-existent (relative to $PWD) source should 
fail' '
+   test_must_fail git clone ../foo baz
+'
 
-test_expect_success \
-'clone should work now that source exists' \
-'git clone foo bar'
+test_expect_success 'clone should work now that source exists' '
+   git clone foo bar
+'
 
-test_expect_success \
-'successful clone must leave the directory' \
-'test -d bar'
+test_expect_success 'successful clone must leave the directory' '
+   test_path_is_dir bar
+'
 
 test_expect_success 'failed clone --separate-git-dir should not leave any 
directories' '
+   test_when_finished "rmdir foo/.git/objects.bak" &&
mkdir foo/.git/objects.bak/ &&
+   test_when_finished "mv foo/.git/objects.bak/* foo/.git/objects/" &&
mv foo/.git/objects/* foo/.git/objects.bak/ &&
test_must_fail git clone --separate-git-dir gitdir foo worktree &&
-   test_must_fail test -e gitdir &&
-   test_must_fail test -e worktree &&
-   mv foo/.git/objects.bak/* foo/.git/objects/ &&
-   rmdir foo/.git/objects.bak
+   test_path_is_missing gitdir &&
+   test_path_is_missing worktree
 '
 
 test_done
-- 
2.16.0.rc0.384.gc477e89267

[PATCH 1/4] t5600: fix outdated comment about unborn HEAD

[Jeff King]

Back when this test was written, git-clone could not handle
a repository without any commits. These days it works fine,
and this comment is out of date.

At first glance it seems like we could just drop this code
entirely now, but it's necessary for the final test, which
was added later. That test corrupts the repository by
temporarily removing its objects, which means we need to
have some objects to move.

Signed-off-by: Jeff King 
---
 t/t5600-clone-fail-cleanup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 4435693bb2..f23f92e5a7 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -22,7 +22,7 @@ test_expect_success \
 # Need a repo to clone
 test_create_repo foo
 
-# clone doesn't like it if there is no HEAD. Is that a bug?
+# create some objects so that we can corrupt the repo later
 (cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 
2>&1)
 
 # source repository given to git clone should be relative to the
-- 
2.16.0.rc0.384.gc477e89267

[PATCH 0/4] Git removes existing folder when cancelling clone

[Jeff King]

On Tue, Jan 02, 2018 at 03:04:43PM -0500, Jeff King wrote:

> So I don't think there's an urgent data-loss bug here; we will only ever
> destroy an empty directory. However, the original intent was to leave
> the filesystem as we found it on a failed or aborted clone, and we
> obviously don't do that in this case. So it might be nice if we could
> restore it to an empty directory.

Here's a patch series to do that. The first three are just preparatory
cleanups; the last one is the interesting bit.

  [1/4]: t5600: fix outdated comment about unborn HEAD
  [2/4]: t5600: modernize style
  [3/4]: clone: factor out dir_exists() helper
  [4/4]: clone: do not clean up directories we didn't create

 builtin/clone.c   |  31 ++---
 t/t5600-clone-fail-cleanup.sh | 100 +++---
 2 files changed, 98 insertions(+), 33 deletions(-)

-Peff

Re: [PATCH ab/simplify-perl-makefile] perl: treat PERLLIB_EXTRA as an extra path again (Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules)

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 02 2018, Jonathan Nieder jotted:

> Subject: perl: treat PERLLIB_EXTRA as an extra path again
>
> PERLLIB_EXTRA was introduced in v1.9-rc0~88^2 (2013-11-15) as a way
> for packagers to add additional directories such as the location of
> Subversion's perl bindings to Git's perl path.  Since 20d2a30f
> (Makefile: replace perl/Makefile.PL with simple make rules,
> 2012-12-10) setting that variable breaks perl-based commands instead:
>
>  $ PATH=$HOME/opt/git/bin:$PATH
>  $ make install prefix=$HOME/opt/git PERLLIB_EXTRA=anextralibdir
> [...]
>  $ head -2 $HOME/opt/git/libexec/git-core/git-add--interactive
>  #!/usr/bin/perl
>  use lib (split(/:/, $ENV{GITPERLLIB} || ":helloiamanextrainstlibdir" || 
> "/usr/local/google/home/jrn/opt/git/share/perl5"));
>  $ git add -p
>  Empty compile time value given to use lib at 
> /home/jrn/opt/git/libexec/git-core/git-add--interactive line 2.
>
> Removing the spurious ":" at the beginning of ":$PERLLIB_EXTRA" avoids
> the "Empty compile time value" error but with that tweak the problem
> still remains: PERLLIB_EXTRA ends up replacing instead of
> supplementing the perllibdir that would be passed to 'use lib' if
> PERLLIB_EXTRA were not set.
>
> The intent was to simplify, as the commit message to 20d2a30f
> explains:
>
> | The scripts themselves will 'use lib' the target directory, but if
> | INSTLIBDIR is set it overrides it. It doesn't have to be this way,
> | it could be set in addition to INSTLIBDIR, but my reading of
> | [v1.9-rc0~88^2] is that this is the desired behavior.
>
> Restore the previous code structure to make PERLLIB_EXTRA work again.
>
> Reproducing this problem requires an invocation of "make install"
> instead of running bin-wrappers/git in place, since the latter sets
> the GITPERLLIB environment variable, avoiding trouble.
>
> Reported-by: Jonathan Koren 
> Signed-off-by: Jonathan Nieder 
> ---
> Jonathan Nieder wrote:
>> Hi,
>>
>> Ævar Arnfjörð Bjarmason wrote:
>>
>> > +++ b/Makefile
>> [...]
>> > -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
>> > -$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES 
>> > GIT-VERSION-FILE
>> > +PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
>> > +$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
>> >$(QUIET_GEN)$(RM) $@ $@+ && \
>> > -  INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
>> > instlibdir` && \
>> >INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>> >INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>> >sed -e '1{' \
>> >-e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
>> >-e 'h' \
>> > -  -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
>> > "'"$$INSTLIBDIR"'"));=' \
>> > +  -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
>> > "'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \
>>
>> This appears to have broken a build with INSTLIBDIR set.
>
> Here it is in patch form.
>
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 5c73cd208a..409e8f6ec9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1951,12 +1951,13 @@ $(SCRIPT_PERL_GEN):
>  PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
>  $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
>   $(QUIET_GEN)$(RM) $@ $@+ && \
> + INSTLIBDIR='$(perllibdir_SQ)' && \
>   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>   sed -e '1{' \
>   -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
>   -e 'h' \
> - -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
> "'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \
> + -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
> "'"$$INSTLIBDIR"'"));=' \
>   -e 'H' \
>   -e 'x' \
>   -e '}' \

This obviously makes perfect sense if the intent is to add this lib dir
instead of it being a replacement (as is clear from this being an issue
you're noting).

With the benefit of hindsight in re-reading the commit + this report I
can see that it *should* be that way, but I assumed it was the other way
around when I wrote this up.

Thanks!

Re: Git removes existing folder when cancelling clone

[Jeff King]

On Tue, Jan 02, 2018 at 06:12:35AM -0500, Robert P. J. Day wrote:

> > I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:
> >
> > 1. `mkdir new-folder`
> > 2. `ls` - shows new-folder
> > 3. `git clone [repo] new-folder`
> > 4. Git asks for password
> > 5. I cancel by pressing ctrl+c
> >
> > Result:
> > `ls` no longer shows new-folder.
> >
> > Expected result:
> > `ls` shows new-folder
> >
> > I’m not sure whether this might be a case of ‘works as intended’,
> > but it’s not what I’d expect.
> 
>   i'm *pretty* sure that the optional directory name you supply is
> meant to represent a directory you want git to *create* for you, not
> one that already exists. that means the behaviour you see makes sense
> -- if git assumes it was supposed to create the directory, and you
> cancel the clone, it reasonably assumes it should get rid of it.

Correct. In the early days we required that the "new-folder" directory
not exist, and the initial "git clone" would have bailed in that case.
Any directory we removed would have been one we created.

That changed in 55892d2398 (Allow cloning to an existing empty
directory, 2009-01-11), and we continue to allow only empty directories.

So I don't think there's an urgent data-loss bug here; we will only ever
destroy an empty directory. However, the original intent was to leave
the filesystem as we found it on a failed or aborted clone, and we
obviously don't do that in this case. So it might be nice if we could
restore it to an empty directory.

Restoring the original contents in the general case is hard, since other
parts of the clone may have created arbitrary files. But if we know the
original is just empty, we can simply delete everything in it, but not
the outer directory.

-Peff

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

[Stefan Beller]

On Thu, Dec 28, 2017 at 11:02 AM, Junio C Hamano  wrote:
> Elijah Newren  writes:
>
>> surprised by the branch name, though.  Was 'ew/' a typo,
>
> Blush X-<.  Yes it is a typo.

Note on that series:
I have reviewed the first three patches (which could form an independent series)
that it would warrant a Reviewed-By: Stefan Beller 

While I reviewed the earlier versions of the later patches, I would
prefer if there is another reviewer for these as it seems like a bigger
contribution at a core functionality.

I cc'd some people who were active in some form of rename detection
work earlier; could you review this series, please?

Thanks,
Stefan

[PATCH ab/simplify-perl-makefile] perl: treat PERLLIB_EXTRA as an extra path again (Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules)

[Jonathan Nieder]

Subject: perl: treat PERLLIB_EXTRA as an extra path again

PERLLIB_EXTRA was introduced in v1.9-rc0~88^2 (2013-11-15) as a way
for packagers to add additional directories such as the location of
Subversion's perl bindings to Git's perl path.  Since 20d2a30f
(Makefile: replace perl/Makefile.PL with simple make rules,
2012-12-10) setting that variable breaks perl-based commands instead:

 $ PATH=$HOME/opt/git/bin:$PATH
 $ make install prefix=$HOME/opt/git PERLLIB_EXTRA=anextralibdir
[...]
 $ head -2 $HOME/opt/git/libexec/git-core/git-add--interactive
 #!/usr/bin/perl
 use lib (split(/:/, $ENV{GITPERLLIB} || ":helloiamanextrainstlibdir" || 
"/usr/local/google/home/jrn/opt/git/share/perl5"));
 $ git add -p
 Empty compile time value given to use lib at 
/home/jrn/opt/git/libexec/git-core/git-add--interactive line 2.

Removing the spurious ":" at the beginning of ":$PERLLIB_EXTRA" avoids
the "Empty compile time value" error but with that tweak the problem
still remains: PERLLIB_EXTRA ends up replacing instead of
supplementing the perllibdir that would be passed to 'use lib' if
PERLLIB_EXTRA were not set.

The intent was to simplify, as the commit message to 20d2a30f
explains:

| The scripts themselves will 'use lib' the target directory, but if
| INSTLIBDIR is set it overrides it. It doesn't have to be this way,
| it could be set in addition to INSTLIBDIR, but my reading of
| [v1.9-rc0~88^2] is that this is the desired behavior.

Restore the previous code structure to make PERLLIB_EXTRA work again.

Reproducing this problem requires an invocation of "make install"
instead of running bin-wrappers/git in place, since the latter sets
the GITPERLLIB environment variable, avoiding trouble.

Reported-by: Jonathan Koren 
Signed-off-by: Jonathan Nieder 
---
Jonathan Nieder wrote:
> Hi,
> 
> Ævar Arnfjörð Bjarmason wrote:
> 
> > +++ b/Makefile
> [...]
> > -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
> > -$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES 
> > GIT-VERSION-FILE
> > +PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
> > +$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
> > $(QUIET_GEN)$(RM) $@ $@+ && \
> > -   INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
> > instlibdir` && \
> > INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
> > INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
> > sed -e '1{' \
> > -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
> > -e 'h' \
> > -   -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
> > "'"$$INSTLIBDIR"'"));=' \
> > +   -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
> > "'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \
> 
> This appears to have broken a build with INSTLIBDIR set.

Here it is in patch form.

 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 5c73cd208a..409e8f6ec9 100644
--- a/Makefile
+++ b/Makefile
@@ -1951,12 +1951,13 @@ $(SCRIPT_PERL_GEN):
 PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
 $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
$(QUIET_GEN)$(RM) $@ $@+ && \
+   INSTLIBDIR='$(perllibdir_SQ)' && \
INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
sed -e '1{' \
-e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
-e 'h' \
-   -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
"'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \
+   -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
"'"$$INSTLIBDIR"'"));=' \
-e 'H' \
-e 'x' \
-e '}' \
-- 
2.16.0.rc0.224.g99853183ba

Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf8b602901..ac59524a7f 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>   ie_match_stat(o->src_index, old, , 
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>   update |= CE_UPDATE;
>   }
> + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
> + !verify_uptodate(old, o))
> + update |= CE_UPDATE;
>   add_entry(o, old, update, 0);
>   return 0;
>   }

This is more sensible than the previous one that broke same() by
unconditionally checking the working tree state, even when the
machinery is told not to look at the working tree.

The code in the pre-context of this hunk, (i.e. the original one you
are half-way mimicking), we realize that the original cache entry
and the cache entry we are checking out are exactly the same and we
overwrite when we know that the path in the working tree is dirty,
and we are not told to "skip" that path in the working tree
(i.e. sparse checkout).  That happens only when we are told to
o->update and o->reset.

Shouldn't we be setting the update bit only when o->update is in
effect, just like we can see in the pre-context of this hunk?  I do
not know if we need to require o->reset and/or need to pay attention
to the skip worktree bit in this new case.

Re: [PATCH 2/2] travis-ci: check that all build artifacts are .gitignore-d

[Lars Schneider]

> On 31 Dec 2017, at 17:02, SZEDER Gábor  wrote:
> 
> Every once in a while our explicit .gitignore files get out of sync
> when our build process learns to create new artifacts, like test
> helper executables, but the .gitignore files are not updated
> accordingly.
> 
> Use Travis CI to help catch such issues earlier: check that there are
> no untracked files at the end of any build jobs building Git (i.e. the
> 64 bit Clang and GCC Linux and OSX build jobs, plus the GETTEXT_POISON
> and 32 bit Linux build jobs) or its documentation, and fail the build
> job if there are any present.
> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/lib-travisci.sh   | 10 ++
> ci/run-linux32-docker.sh |  2 ++
> ci/run-tests.sh  |  2 ++
> ci/test-documentation.sh |  6 ++
> 4 files changed, 20 insertions(+)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 1543b7959..07f27c727 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -67,6 +67,16 @@ skip_good_tree () {
>   exit 0
> }
> 
> +check_unignored_build_artifacts ()
> +{
> + ! git ls-files --other --exclude-standard --error-unmatch \
> + -- ':/*' 2>/dev/null ||

What does "-- ':/*'" do? Plus, why do you redirect stderr?

--

Both patches look good to me!

Thanks,
Lars

Re: [PATCH 1/2] travis-ci: don't store P4 and Git LFS in the working tree

[Lars Schneider]

> On 31 Dec 2017, at 17:02, SZEDER Gábor  wrote:
> 
> The Clang and GCC 64 bit Linux build jobs download and store the P4
> and Git LFS executables under the current directory, which is the
> working tree that we are about to build and test.  This means that Git
> commands like 'status' or 'ls-files' would list these files as
> untracked.  The next commit is about to make sure that there are no
> untracked files present after the build, and the downloaded
> executables in the working tree are interfering with those upcoming
> checks.
> 
> Therefore, let's download P4 and Git LFS in the home directory,
> outside of the working tree.

I was concerned for a moment that the executables would not be 
available to the 32-bit build anymore... but we don't use them
in that build anyways.

Looks good to me!

- Lars

> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/lib-travisci.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index bade71617..1543b7959 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -99,8 +99,8 @@ linux-clang|linux-gcc)
>   export LINUX_P4_VERSION="16.2"
>   export LINUX_GIT_LFS_VERSION="1.5.2"
> 
> - P4_PATH="$(pwd)/custom/p4"
> - GIT_LFS_PATH="$(pwd)/custom/git-lfs"
> + P4_PATH="$HOME/custom/p4"
> + GIT_LFS_PATH="$HOME/custom/git-lfs"
>   export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
>   ;;
> osx-clang|osx-gcc)
> -- 
> 2.16.0.rc0.67.g3a46dbca7
> 

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

[Ramsay Jones]

On 02/01/18 15:32, Ramsay Jones wrote:
> On 02/01/18 11:36, Adam Dinwoodie wrote:
>> On Saturday 30 December 2017 at 02:40 pm +, Adam Dinwoodie wrote:
>>> On Saturday 30 December 2017 at 02:21 pm +, Ramsay Jones wrote:
[snip]
>> I'm not able to reproduce this: t5580 is passing on both my Windows 10
>> test systems on v2.16.0-rc0.
[snip]
> I just tried running the test again by hand, and I can't get it to fail!
> 
> Hmm, I have just set off a complete test-suite run, so I won't be able
> to report on that for a while ... ;-)

Yep, as expected, the test-suite passes no problem now! ;-)

> I have an idea: when running the failing tests the other day, I was
> remotely logged in via ssh (I have cygwin sshd running on my win10
> box), but today I was logged in directly. The sshd daemon is run by
> a privileged user, so maybe that could be related ... dunno.

Also, when logged-in remotely it fails consistently, when logged-in
directly it passes consistently. :-D

ATB,
Ramsay Jones

Re: [PATCH 4/4] branch: add '--show-description' option

[SZEDER Gábor]

On Tue, Jan 2, 2018 at 10:32 AM, Johannes Sixt  wrote:
> > Which makes me wonder: Why would --show-description have to error out
> > silently? This is not 'git config' after all.

> I don't have a strong opinion about this, and certainly wouldn't mind
> adding an error message instead.

And it would look like this:

  -- >8 --

Subject: [PATCH 4/4] branch: add '--show-description' option

'git branch' has an option to edit a branch's description, but lacks
the option to show that description.

Therefore, add a new '--show-description' option to do just that, as a
more user-friendly alternative to 'git config --get
branch.$branchname.description':

  - it's shorter to type (both in the number of characters and the
number of TABs if using completion),
  - works on the current branch without explicitly naming it,
  - hides the implementation detail that branch descriptions are
stored in the config file, and
  - errors out with a proper error message when the given branch
doesn't exist or has no description.

Signed-off-by: SZEDER Gábor 
---
 Documentation/git-branch.txt   |  6 +-
 builtin/branch.c   | 39 +++---
 contrib/completion/git-completion.bash |  4 ++--
 t/t3200-branch.sh  | 21 ++
 4 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index b3084c99c..e05c9e193 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 'git branch' (-m | -M) [] 
 'git branch' (-c | -C) [] 
 'git branch' (-d | -D) [-r] ...
-'git branch' --edit-description []
+'git branch' (--edit-description | --show-description) []
 
 DESCRIPTION
 ---
@@ -226,6 +226,10 @@ start-point is either a local or remote-tracking branch.
`request-pull`, and `merge` (if enabled)). Multi-line explanations
may be used.
 
+--show-description::
+   Show the description of the branch previously set using
+   `--edit-description`.
+
 --contains []::
Only list branches which contain the specified commit (HEAD
if not specified). Implies `--list`.
diff --git a/builtin/branch.c b/builtin/branch.c
index 32531aa44..748a1a575 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -573,7 +573,7 @@ static int edit_branch_description(const char *branch_name)
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
-   int reflog = 0, edit_description = 0;
+   int reflog = 0, edit_description = 0, show_description = 0;
int quiet = 0, unset_upstream = 0;
const char *new_upstream = NULL;
enum branch_track track;
@@ -615,6 +615,8 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('l', "create-reflog", , N_("create the branch's 
reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
+   OPT_BOOL(0, "show-description", _description,
+N_("show the description for the branch")),
OPT__FORCE(, N_("force creation, move/rename, deletion")),
OPT_MERGED(, N_("print only branches that are merged")),
OPT_NO_MERGED(, N_("print only branches that are not 
merged")),
@@ -654,7 +656,9 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 0);
 
-   if (!delete && !rename && !copy && !edit_description && !new_upstream 
&& !unset_upstream && argc == 0)
+   if (!delete && !rename && !copy &&
+   !edit_description && !show_description &&
+   !new_upstream && !unset_upstream && argc == 0)
list = 1;
 
if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || 
filter.points_at.nr ||
@@ -662,7 +666,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
list = 1;
 
if (!!delete + !!rename + !!copy + !!new_upstream +
-   list + unset_upstream + edit_description > 1)
+   list + unset_upstream + edit_description + show_description > 1)
usage_with_options(builtin_branch_usage, options);
 
if (filter.abbrev == -1)
@@ -737,6 +741,35 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 
if (edit_branch_description(branch_name))
return 1;
+   } else if (show_description) {
+   const char *branch_name;
+   struct strbuf buf = STRBUF_INIT;
+   char *description = NULL;
+
+   if (!argc) {
+   if (filter.detached)
+   die(_("cannot show description on detached 
HEAD"));
+  

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

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

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

This appears to have broken a build with INSTLIBDIR set.

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

(forgive the hackiness there).

Is there a reason we don't do

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

and

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

?

Thanks,
Jonathan

Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

[Lars Schneider]

> On 01 Jan 2018, at 22:59, tbo...@web.de wrote:
> 
> From: Torsten Bögershausen 
> 
> When calling convert_to_git(), the checksafe parameter has been used to
> check if commit would give a non-roundtrip conversion of EOL.
> 
> When checksafe was introduced, 3 values had been in use:
> SAFE_CRLF_FALSE: no warning
> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip
> 
> Already today the integer value 0 is passed as the parameter checksafe
> instead of the correct enum value SAFE_CRLF_FALSE.
> 
> Turn the whole call chain to use an integer with single bits, which
> can be extended in the next commits:
> - The global configuration variable safe_crlf is now conv_flags_eol.
> - The parameter checksafe is renamed into conv_flags.
> 
> Helped-By: Lars Schneider 
> Signed-off-by: Torsten Bögershausen 
> ---
> This is my suggestion.
> (1) The flag bits had been renamed.
> (2) The (theoretical ?) mix of WARN/FAIL is still there,
>I am not sure if this is a real problem.
> 
> (3) There are 2 reasons that CONV_EOL_RENORMALIZE is set.
>Either in a renormalizing merge, or by running
>git add --renormalize .
>Therefor HASH_RENORMALIZE is not the same as CONV_EOL_RENORMALIZE.
> 
> apply.c|  6 +++---
> combine-diff.c |  2 +-
> config.c   |  7 +--
> convert.c  | 38 +++---
> convert.h  | 17 +++--
> diff.c |  8 
> environment.c  |  2 +-
> sha1_file.c| 12 ++--
> 8 files changed, 46 insertions(+), 46 deletions(-)
> ...
> -static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
> +static void check_conv_flags_eol(const char *path, enum crlf_action 
> crlf_action,
>   struct text_stat *old_stats, struct text_stat 
> *new_stats,
> - enum safe_crlf checksafe)
> + int conv_flags)
> {
>   if (old_stats->crlf && !new_stats->crlf ) {
>   /*
>* CRLFs would not be restored by checkout
>*/
> - if (checksafe == SAFE_CRLF_WARN)
> + if (conv_flags & CONV_EOL_RNDTRP_DIE)
> + die(_("CRLF would be replaced by LF in %s."), path);
> + else if (conv_flags & CONV_EOL_RNDTRP_WARN)

Here we go with CONV_EOL_RNDTRP_DIE if there is garbage in conv_flags.
Garbage example: CONV_EOL_RNDTRP_DIE *and* CONV_EOL_RNDTRP_WARN are set.

Good!

>   ...
> /*
> diff --git a/diff.c b/diff.c
> index fb22b19f09..2470af52b2 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3524,9 +3524,9 @@ int diff_populate_filespec(struct diff_filespec *s, 
> unsigned int flags)
>* demote FAIL to WARN to allow inspecting the situation
>* instead of refusing.
>*/
> - enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
> - ? SAFE_CRLF_WARN
> - : safe_crlf);
> + int conv_flags = (conv_flags_eol == CONV_EOL_RNDTRP_DIE
> + ? CONV_EOL_RNDTRP_WARN
> + : conv_flags_eol);

If there is garbage in conv_flags_eol then we would not demote the DIE
flag here.

How about something like that:
int conv_flags = conv_flags_eol & ~CONV_EOL_RNDTRP_DIE;

---

In general I like the patch as I think the variables are a bit easier to 
understand. 
One thing I stumbled over while reading the patch:

The global variable "conv_flags_eol". I think the Git coding guidelines
have no recommendation for naming global variables. I think a 
"global_conv_flags_eol"
or something would have helped me. I can also see how others might frown upon 
such 
a naming scheme.

If this patch gets accepted then I will rebase my encoding series on top of it:
https://public-inbox.org/git/2017122915.39680-1-lars.schnei...@autodesk.com/


Thanks,
Lars

Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly

[Jeff Hostetler]

On 12/22/2017 12:16 PM, Brandon Williams wrote:

On 12/22, Johannes Schindelin wrote:

[...]

That pattern is violated by OIDMAP_INIT, though. The first call to
oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the
cmpfn function pointer still points to NULL, but since there are no
entries to be compared, cmpfn will not be called. Things break down,
though, as soon as there is even one entry.

Rather than causing confusion, frustration and needless loss of time due
to pointless debugging, let's *rename* OIDMAP_INIT so that developers
who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
that with oidmaps.
  
-#define OIDMAP_INIT { { NULL } }

+/*
+ * This macro initializes the data structure only incompletely, just enough
+ * to call oidmap_get() on an empty map. Use oidmap_init() instead.
+ */
+#define OIDMAP_INIT_INCOMPLETELY { { NULL } }


Alternatively, could we define the macro to an expression
that would cause a compiler error?  Then any new code written
would fail to compile.  And we document the issue in a comment
above the macro so no one changes the macro to "fix" it.

(I suggest this as opposed to simply removing the macro
to prevent someone from re-adding it later, since it is the
standard pattern.)

Just a thought,
Jeff

[PATCH] doc/SubmittingPatches: improve text formatting

[Todd Zullinger]

049e64aa50 ("Documentation: convert SubmittingPatches to AsciiDoc",
2017-11-12) changed the `git blame` and `git shortlog` examples given in
the section on sending your patches.

In order to italicize the `$path` argument the commands are enclosed in
plus characters as opposed to backticks.  The difference between the
quoting methods is that backtick enclosed text is not subject to further
expansion.  This formatting makes reading SubmittingPatches in a git
clone a little more difficult.  In addition to the underscores around
`$path` the `--` chars in `git shortlog --no-merges` must be replaced
with `{litdd}`.

Use backticks to quote these commands.  The italicized `$path` is lost
from the html version but the commands can be read (and copied) more
easily by users reading the text version.  These readers are more likely
to use the commands while submitting patches.  Make it easier for them.

Signed-off-by: Todd Zullinger 
---

I missed this in the initial discussion.  It was mentioned by
Junio and brian explained the reasoning for using it in
<20171031012710.jfemqb6ybiog4...@genre.crustytoothpaste.net>:

> > The +fixed width with _italics_ mixed in+ mark-up is something not
> > exactly new, but it is rarely used in our documentation set, so I
> > had to double check by actually seeing how it got rendered, and it
> > looked alright.
>
> I thought it provided some hint to the reader that this wasn't meant to
> be typed literally.  It's a preference of mine and I think it aids in
> readability, but it can be changed if we want.

That's what I had guessed before I looked back at the list
archives.  I understand the desire to make it clear that $path
isn't a literal value.  I think that it's worth losing that subtle
hint in order to make the plain text SubmittingPatches file a
little easier to read.

Whether the people most likely to be considering sending a patch
for git.git are will read the document from a git clone or the
rendered html output is the main question.  Though even readers of
the installed text file in their packaged git will suffer a bit.

It's great having the document in HTML to aid in sharing it's
guidance with others, no doubt.  I've wanted to symlink to
portions of the document numerous times.  I hope a small change to
make it more legible to those who also like plain text will be
welcome.

I considered using backticks for the commands and then +_$path_+
for the argument.  Maybe that's a reasonable compromise?  I.e.:

-+git blame _$path_+ and +git shortlog {litdd}no-merges _$path_+ would help to
+`git blame` +_$path_+ and `git shortlog --no-merges` +_$path_+ would help to

The text version is still a bit strange with that method, but it
certainly separates "$path" from the rest of the command in both
the text and html output. :)

 Documentation/SubmittingPatches | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 3ef30922ec..a1d0feca36 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -261,7 +261,7 @@ not a text/plain, it's something else.
 
 Send your patch with "To:" set to the mailing list, with "cc:" listing
 people who are involved in the area you are touching (the output from
-+git blame _$path_+ and +git shortlog {litdd}no-merges _$path_+ would help to
+`git blame $path` and `git shortlog --no-merges $path` would help to
 identify them), to solicit comments and reviews.
 
 :1: footnote:[The current maintainer: gits...@pobox.com]
-- 
2.16.0.rc0

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

[Ramsay Jones]

On 02/01/18 11:36, Adam Dinwoodie wrote:
> On Saturday 30 December 2017 at 02:40 pm +, Adam Dinwoodie wrote:
>> On Saturday 30 December 2017 at 02:21 pm +, Ramsay Jones wrote:
>>> Hi Junio, Adam,
>>>
>>> Just a quick note about the failure of the test-suite on cygwin.
>>> In particular, test t5580-clone-push-unc.sh #3, like so:
>>>
>>> 
>>>
>>> Adam, are you running the tests on Windows 10?
>>
>> I'm only routinely testing on Windows 10 x86_64, but between holidays
>> various, I've not had the tests running for the past couple of weeks.
>> I'm kicking off a build now in the name of verifying I see the same
>> problem.
> 
> I'm not able to reproduce this: t5580 is passing on both my Windows 10
> test systems on v2.16.0-rc0.

Hmm, interesting. BTW, I should have noted which version I'm on (just
in case it matters): Windows 10 Home, Version 1709, OS Build 16299.125.

I am reasonably up-to-date on cygwin:

  $ uname -a
  CYGWIN_NT-10.0 satellite 2.9.0(0.318/5/3) 2017-09-12 10:18 x86_64 Cygwin
  $ 

[I only recently updated to the creator's update (I'm not signed up to
the 'insider program'), and so could not try out WSL until now. I would
not recommend it to anyone who wants to develop software - a Linux VM
is an _order of magnitude_ faster, so ... ]

> Looking at your output, it sounds like there's something slightly odd
> with your directory permissions.  I agree the mixed slashes look odd,
> but given the test is passing on both my systems, I don't think that can
> be the problem.

The directory permissions look fine to me (except for //localhost/C$).

> I suspect you're going to need to do some more digging yourself given
> this appears to be a permissions issue on your system.  For a start,
> when you get to the failing `mkdir` with a UNC path, are you able to
> create the equivalent directory using Cygwin's `mkdir` but specifying a
> regular non-UNC path, 

yes, this is not a problem.

or by opening the UNC path in Explorer and
> creating the directory there?

I didn't get to this because ...

I just tried running the test again by hand, and I can't get it to fail!

Hmm, I have just set off a complete test-suite run, so I won't be able
to report on that for a while ... ;-)

I have an idea: when running the failing tests the other day, I was
remotely logged in via ssh (I have cygwin sshd running on my win10
box), but today I was logged in directly. The sshd daemon is run by
a privileged user, so maybe that could be related ... dunno.

I will have to investigate some more. (If you have any ideas ... :-D )

ATB,
Ramsay Jones

Re: Git removes existing folder when cancelling clone

[Robert P. J. Day]

On Tue, 2 Jan 2018, Stephan Janssen wrote:

> Hi,
>
> I hope this mailing list is the right way to communicate this.
>
> I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:
>
> 1. `mkdir new-folder`
> 2. `ls` - shows new-folder
> 3. `git clone [repo] new-folder`
> 4. Git asks for password
> 5. I cancel by pressing ctrl+c
>
> Result:
> `ls` no longer shows new-folder.
>
> Expected result:
> `ls` shows new-folder
>
> I’m not sure whether this might be a case of ‘works as intended’,
> but it’s not what I’d expect.

  i'm *pretty* sure that the optional directory name you supply is
meant to represent a directory you want git to *create* for you, not
one that already exists. that means the behaviour you see makes sense
-- if git assumes it was supposed to create the directory, and you
cancel the clone, it reasonably assumes it should get rid of it.

  i am willing to be corrected.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

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

[Adam Dinwoodie]

On Saturday 30 December 2017 at 02:40 pm +, Adam Dinwoodie wrote:
> On Saturday 30 December 2017 at 02:21 pm +, Ramsay Jones wrote:
> > Hi Junio, Adam,
> > 
> > Just a quick note about the failure of the test-suite on cygwin.
> > In particular, test t5580-clone-push-unc.sh #3, like so:
> > 
> > 
> >
> > Adam, are you running the tests on Windows 10?
> 
> I'm only routinely testing on Windows 10 x86_64, but between holidays
> various, I've not had the tests running for the past couple of weeks.
> I'm kicking off a build now in the name of verifying I see the same
> problem.

I'm not able to reproduce this: t5580 is passing on both my Windows 10
test systems on v2.16.0-rc0.

Looking at your output, it sounds like there's something slightly odd
with your directory permissions.  I agree the mixed slashes look odd,
but given the test is passing on both my systems, I don't think that can
be the problem.

I suspect you're going to need to do some more digging yourself given
this appears to be a permissions issue on your system.  For a start,
when you get to the failing `mkdir` with a UNC path, are you able to
create the equivalent directory using Cygwin's `mkdir` but specifying a
regular non-UNC path, or by opening the UNC path in Explorer and
creating the directory there?

Adam

Git removes existing folder when cancelling clone

[Stephan Janssen]

Hi,

I hope this mailing list is the right way to communicate this.

I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:

1. `mkdir new-folder`
2. `ls` - shows new-folder
3. `git clone [repo] new-folder`
4. Git asks for password
5. I cancel by pressing ctrl+c

Result:
`ls` no longer shows new-folder.

Expected result:
`ls` shows new-folder

I’m not sure whether this might be a case of ‘works as intended’, but it’s not 
what I’d expect.

Kind regards,
Stephan Janssen

Re: [PATCH 4/4] branch: add '--show-description' option

[SZEDER Gábor]

On Tue, Jan 2, 2018 at 10:32 AM, Johannes Sixt  wrote:
> Am 01.01.2018 um 23:54 schrieb SZEDER Gábor:

>>- errors out with a proper error message when the given branch
>>  doesn't exist (but exits quietly with an error code when the
>>  branch does exit but has no description, just like the 'git config'
>>  query does).
>
>
>> +test_expect_success '--show-description with no description errors
>> quietly' '
>> +   git config --unset branch.master.description &&
>> +   test_must_fail git branch --show-description >actual 2>actual.err
>> &&
>> +   test_must_be_empty actual &&
>> +   test_must_be_empty actual.err
>> +'
>
>
> Checking the exact contents of stderr typically fails when tests are run
> under -x. Perhaps
>
> test_i18ngrep ! "fatal: " actual.err &&"
> test_i18ngrep ! "error: " actual.err &&
> test_i18ngrep ! "warning: " actual.err
>
> Which makes me wonder: Why would --show-description have to error out
> silently? This is not 'git config' after all.

I figured it would be beneficial if it were a drop-in replacement for
the original 'git config' query.

I don't have a strong opinion about this, and certainly wouldn't mind
adding an error message instead.

Gábor

Re: [PATCH 4/4] branch: add '--show-description' option

[SZEDER Gábor]

On Tue, Jan 2, 2018 at 6:17 AM, Eric Sunshine  wrote:
> On Mon, Jan 1, 2018 at 5:54 PM, SZEDER Gábor  wrote:

> s/exit/exist/

Thanks.

>> query does).
>>
>> Signed-off-by: SZEDER Gábor 
>> ---
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> @@ -737,6 +741,35 @@ int cmd_branch(int argc, const char **argv, const char 
>> *prefix)
>> +   } else if (show_description) {
>> +   [...]
>> +   if (!argc) {
>> +   if (filter.detached)
>> +   die(_("cannot show description on detached 
>> HEAD"));
>> +   branch_name = head;
>> +   } else if (argc == 1)
>> +   branch_name = argv[0];
>> +   else
>> +   die(_("cannot show description of more than one 
>> branch"));
>
> Aside from paralleling the single branch accepted by
> --edit-description, why this limitation? (Just curious; I don't feel
> strongly one way or the other.)

It's not just '--edit-description', most other options won't accept
multiple branches either.  As far as I can tell only deleting branches
can deal with multiple branches in one go.

Furthermore, branch descriptions are likely more lines long, so we
can't just dump them one after the other, but we we would have to
separate the descriptions of different branches in the output.
Considering that 'git branch' mostly works only with a single branch at
a time, I didn't feel the need to do so.

Gábor

Re: Segfault

[Andrew Tsykhonya]

I made:
$git stash
$git checkout 
$git stash pop - crash here. changes were applied, but index.lock was
not removed.

The issue appeared only once, I haven't seen it before and cannot
reproduce it now.

Thanks, updated git to 2.15.1.

2017-12-31 10:59 GMT+02:00 Eric Sunshine :
> On Fri, Dec 29, 2017 at 4:04 AM, Andrew Tsykhonya
>  wrote:
>> git stash pop resulted in crash:
>> /usr/local/Cellar/git/2.10.2/libexec/git-core/git-stash: line 470:
>> 14946 Segmentation fault: 11 git merge-recursive $b_tree -- $c_tree
>> $w_tree
>> although, the changes have been applied successfully.
>
> Thanks for the problem report. Can you come up with a reproduction
> recipe in order to help debug the problem?
>
> Also, what happens if you update to a newer version of Git? You appear
> to be running 2.10.2, whereas the latest version in Homebrew seems to
> be 2.15.1.

Re: [PATCH 4/4] branch: add '--show-description' option

[Johannes Sixt]

Am 01.01.2018 um 23:54 schrieb SZEDER Gábor:

'git branch' has an option to edit a branch's description, but lacks
the option to show that description.

Therefore, add a new '--show-description' option to do just that, as a
more user-friendly alternative to 'git config --get
branch.$branchname.description':

   - it's shorter to type (both in the number of characters and the
 number of TABs if using completion),
   - works on the current branch without explicitly naming it,
   - hides the implementation detail that branch descriptions are
 stored in the config file, and
   - errors out with a proper error message when the given branch
 doesn't exist (but exits quietly with an error code when the
 branch does exit but has no description, just like the 'git config'
 query does).



+test_expect_success '--show-description with no description errors quietly' '
+   git config --unset branch.master.description &&
+   test_must_fail git branch --show-description >actual 2>actual.err &&
+   test_must_be_empty actual &&
+   test_must_be_empty actual.err
+'


Checking the exact contents of stderr typically fails when tests are run 
under -x. Perhaps


test_i18ngrep ! "fatal: " actual.err &&"
test_i18ngrep ! "error: " actual.err &&
test_i18ngrep ! "warning: " actual.err

Which makes me wonder: Why would --show-description have to error out 
silently? This is not 'git config' after all.


-- Hannes