Re: [PATCH v7 0/7] convert: add support for different encodings

[Jeff King]

On Wed, Feb 28, 2018 at 09:42:27AM -0800, Junio C Hamano wrote:

> > I also think we'd want a plan for this to be used consistently in other
> > diff-like tools. E.g., "git blame" uses textconv for the starting file
> > content, and it would be nice for this to kick in then, too. Ditto for
> > things like grep, pickaxe, etc.
> 
> You probably do not want to limit your thinking to the generation
> side.  It is entirely plausible to have "we deal with diff in this
> encoding X" in addition to "the in-repo encoding for this project is
> this encoding Y" and "the working tree encoding for this path is Z"
> and allow them to interact in "git diff | git apply" pipeline.
> 
> "diff/format-patch --stdout/etc." on the upstream would first iconv
> Y to X and feed the contents in X to xdiff machinery, which is sent
> down the pipe and received by apply, which reads the preimage from
> the disk or from the repository.  If doing "apply" without
> "--cached/--index", the preimage data from the disk would go through
> iconv Z to X.  If doing "apply --cached/--index", the preimage data
> from the repo would go through iconv Y to X.  The incoming patch is
> in X, so we apply, and the resulting postimage will be re-encoded in
> Z in the working tree and Y in the repository.

I agree that would be convenient, but I have to wonder if all the
complexity is worth it to maintain the idea of a distinct in-repo
representation. It seems like it would open up a ton of corner cases.
And I suspect most people would be happy enough with either a
clean/smudge style worktree conversion or a textconv-style view.

So if somebody wants to work on it, I don't want to stop them. But I
think there's room for the simpler solutions in the meantime.

-Peff

Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache

[Jeff King]

On Wed, Feb 28, 2018 at 01:27:03PM -0800, Junio C Hamano wrote:

> Somehow this topic has been hanging without getting further
> attention for too long.  It's time to wrap it up and moving it
> forward.  How about this?
> 
> -- >8 --
> From: Junio C Hamano 
> Date: Wed, 28 Feb 2018 13:21:09 -0800
> Subject: [PATCH] untracked cache: use git_env_bool() not getenv() for 
> customization
> [...]

This looks good to me. Thanks for tying up the loose end.

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:

> > I'll follow up with a patch to
> > address the confusing pattern which Peff mentioned and which fooled me
> > when I prepared v1.
> 
> Here is such a patch on top of the others. I'm not particularly proud of the
> name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
> IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.
> 
> I think this makes the current users a bit more obvious, and should help 
> future
> users get this optimization right.

IMHO the result is easier to follow. Except for one case:

> - if (active_cache_changed || force_write) {
> - if (newfd < 0) {
> - if (refresh_args.flags & REFRESH_QUIET)
> - exit(128);
> - unable_to_lock_die(get_index_file(), lock_error);
> - }
> - if (write_locked_index(_index, _file, COMMIT_LOCK))
> - die("Unable to write new index file");
> + if (newfd < 0 && (active_cache_changed || force_write)) {
> + if (refresh_args.flags & REFRESH_QUIET)
> + exit(128);
> + unable_to_lock_die(get_index_file(), lock_error);
>   }
>  
> - rollback_lock_file(_file);
> + if (write_locked_index(_index, _file,
> +COMMIT_LOCK | (force_write ? 0 : 
> SKIP_IF_UNCHANGED)))
> + die("Unable to write new index file");

where I think the logic just ends up repeating itself. I guess you were
anxious to try to get rid of active_cached_changed, but I don't think
keeping it around is really that big a deal (and certainly another trick
is to just say "the_index.cache_changed").

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Wed, Feb 28, 2018 at 08:07:53PM +0100, Martin Ågren wrote:

> This is v2 of my series to always release locks. As before, there's a
> conflict with pu, where the correct resolution is to take my version of
> the conflicting hunk.
> 
> The only difference to v1 is in patch 3. I'll follow up with a patch to
> address the confusing pattern which Peff mentioned and which fooled me
> when I prepared v1.

This looks good to me. And I'm glad my rambling helped find something
useful. ;)

-Peff

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Wed, Feb 28, 2018 at 05:51:14PM +0100, demerphq wrote:

> I would look into putting it into a module and then using the PERL5OPT
> environment var to have it loaded automagically in any of your perl
> scripts.
> 
> For instance if you put that code into a module called Git/DieTrap.pm
> 
> then you could do:
> 
> PERL5OPT=-MGit::DieTrap
> 
> In your test setup code assuming you have some. Then you don't need to
> change any of your scripts just the test runner framework.

That's a clever trick.

It's not clear to me though if we just want to tweak the programs run in
the test scripts in order to get test_must_fail to stop complaining, or
if we consider the unusual exit codes from our perl-based Git programs
to be an error that should be fixed for real use, too.

-Peff

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Wed, Feb 28, 2018 at 05:46:22PM +0100, demerphq wrote:

> > You're right. I cut down my example too much and dropped the necessary
> > eval magic. Try this:
> >
> > -- >8 --
> > SIG{__DIE__} = sub {
> >   CORE::die @_ if $^S || !defined($^S);
> >   print STDERR "fatal: @_";
> >   exit 128;
> > };
> 
> FWIW, this doesn't need to use CORE::die like that unless you have
> code that overrides die() or CORE::GLOBAL::die, which would be pretty
> unusual.
> 
> die() within $SIG{__DIE__} is special cased not to trigger $SIG{__DIE__} 
> again.
> 
> Of course it doesn't hurt, but it might make a perl hacker do a double
> take why you are doing it. Maybe add a comment like
> 
> # using CORE::die to armor against overridden die()

Thanks, I agree it should just be "die". I pulled this from an old
error-handling pattern I used in some of my scripts (which _does_
override die). I screwed it up when cutting it down the first time, but
then I didn't cut enough the second. :)

-Peff

Re: The case for two trees in a commit ("How to make rebase less modal")

[Jacob Keller]

On Wed, Feb 28, 2018 at 3:30 PM, Stefan Beller  wrote:
> $ git hash-object --stdin -w -t commit < tree c70b4a33a0089f15eb3b38092832388d75293e86
> parent 105d5b91138ced892765a84e771a061ede8d63b8
> author Stefan Beller  1519859216 -0800
> committer Stefan Beller  1519859216 -0800
> tree 5495266479afc9a4bd9560e9feac465ed43fa63a
> test commit
> EOF
> 19abfc3bf1c5d782045acf23abdf7eed81e16669
> $ git fsck |grep 19abfc3bf1c5d782045acf23abdf7eed81e16669
> $
>
> So it is technically possible to create a commit with two tree entries
> and fsck is not complaining.
>
> But why would I want to do that?
>
> There are multiple abstraction levels in Git, I think of them as follows:
> * data structures / object model
> * plumbing
> * porcelain commands to manipulate the repo "at small scale", e.g.
> create a commit/tag
> * porcelain to modify the repo "at larger scale", such as rebase,
> cherrypicking, reverting
>   involving more than 1 commit.
>
> These large scale operations involving multiple commits however
> are all modal in its nature. Before doing anything else, you have to
> finish or abort the rebase or you need expert knowledge how to
> go otherwise.
>
> During the rebase there might be a hard to resolve conflict, which
> you may not want to resolve right now, but defer to later.  Deferring a
> conflict is currently impossible, because precisely one tree is recorded.
>

How does this let you defer a conflict? A future commit which modified
blobs in that tree wouldn't know what version of the trees/blobs to
actually use? Clearly future commits could record their own trees, but
how would they generate the "correct" tree?

Maybe I am missing something here?

Thanks,
Jake

> If we had multiple trees possible in a commit, then all these large scale
> operations would stop being modal and you could just record the unresolved
> merge conflict instead; to come back later and fix it up later.
>
> I'd be advocating for having multiple trees in a commit
> possible locally; it might be a bad idea to publish such trees.
>
> Opinions or other use cases?
>
> Thanks,
> Stefan

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

On 1 March 2018 at 00:20, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> A further upshot of this patch is that `active_cache_changed`, which is
>> defined as `the_index.cache_changed`, now only has a few users left.
>
> I am undecided if this is a *good* thing.  In a few codepaths where
> we make a speculative update to the on-disk index file, I find that
> the explicit mention of active_cache_changed clarifies what is going
> on.  Perhaps once my (and other old timers') eyes get used to this
> new SKIP_IF_UNCHANGED symbol, it would start serving the same
> purpose ;-)

Right, you might say that this trades one symbol for another. What I
meant was, we only have a few "active_cache_changed" and soon (TM) they
might all be "the_index.cache_changed" or "index->cache_changed" and the
macro could be retired. My understanding of the history is limited, but
I was under the impression that this was like a transition macro (albeit
a very old one!).

Martin

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Sergey Organov]

Hi Igor,

Igor Djordjevic  writes:

> Hi Sergey,
>
> On 28/02/2018 06:19, Sergey Organov wrote:
>> 
>> > > (3) ---X1---o---o---o---o---o---X2
>> > >|\   |\
>> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
>> > >| \  |
>> > >|  M |
>> > >| /  |
>> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
>> > >
>> >
>> > Meh, I hope I`m rushing it now, but for example, if we had decided to 
>> > drop commit A2 during an interactive rebase (so losing A2' from 
>> > diagram above), wouldn`t U2' still introduce those changes back, once 
>> > U1' and U2' are merged, being incorrect/unwanted behavior...? :/
>> 
>> I think the method will handle this nicely.
>
> That`s what I thought as well. And then I made a test. And then the 
> test broke... Now, might be the test was flawed in the first place, 
> but thinking about it a bit more, it does seem to make sense not to 
> handle this case nicely :/

Yeah, I now see it myself. I'm sorry for being lazy and not inspecting
this more carefully in the first place.

[...]

> So while your original proposal currently seems like it could be 
> working nicely for non-interactive rebase (and might be some simpler 
> interactive ones), now hitting/acknowledging its first real use 
> limit, my additional quick attempt[1] just tries to aid pretty 
> interesting case of complicated interactive rebase, too, where we 
> might be able to do better as well, still using you original proposal 
> as a base idea :)

Yes, thank you for pushing me back to reality! :-) The work and thoughts
you are putting into solving the puzzle are greatly appreciated!

Thinking about it overnight, I now suspect that original proposal had a
mistake in the final merge step. I think that what you did is a way to
fix it, and I want to try to figure what exactly was wrong in the
original proposal and to find simpler way of doing it right.

The likely solution is to use original UM as a merge-base for final
3-way merge of U1' and U2', but I'm not sure yet. Sounds pretty natural
though, as that's exactly UM from which both U1' and U2' have diverged
due to rebasing and other history editing.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Sergey Organov]

Igor Djordjevic  writes:

[...]
>> > Hmm, still rushing it, but what about adding an additional step, 
>> > something like this:
>> 
>> I think it's unneeded, as it should work fine without it, see another
>> reply.
>
> Unfortunately, I have a broken test case saying different - it could 
> very well be a flawed test, too, but let`s elaborate in that 
> other sub-thread[1], indeed.

Yeah, I was too fast to reply and I was wrong, sorry about it.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Sergey Organov]

Hi Igor,

Igor Djordjevic  writes:

> On 28/02/2018 21:25, Igor Djordjevic wrote:
>> 
>> But U1' and U2' are really to be expected to stay the same in 
>> non-interactive rebase case only...
>
> Just to rephrase to "could be expected" here, meaning still not 
> necessarily in this case, either - I`ve just witnessed 
> non-interactive rebase Johannes previously described[1], merge with 
> `-s ours` (dropping B* patches), plus B1 cherry-picked between 
> X1..X2, eventually coming up with different U1' and U2', too (which 
> would produce a wrong end result, if continued).

Even non-interactive rebase may bring arbitrary asymmetric changes on
both sides of the merge, especially likely when resolving conflicts
needs to take place. OTOH, interactive history editing session could
have merges that we don't intend to edit at all. Overall, neither
non-interactive case is in fact much simpler, nor interactive case is so
much different.

> But I guess this should go to the "complex history" pile, good thing 
> still being that diff safety check between U1' and U2' works as 
> expected, thus automatic merge rebase can be aborted and command 
> given back to the user for closer inspection.

Exactly. This fact hopefully won't stop us from looking for more
suitable automated handling of the general case though. It should still
be our goal to reduce the number of such aborts and to suggest better
merge result for amendment when we are still aborting.

Your proposal hopefully is such a valuable improvement.

-- Sergey

Re: The case for two trees in a commit ("How to make rebase less modal")

[Jeff King]

On Wed, Feb 28, 2018 at 03:30:27PM -0800, Stefan Beller wrote:

> During the rebase there might be a hard to resolve conflict, which
> you may not want to resolve right now, but defer to later.  Deferring a
> conflict is currently impossible, because precisely one tree is recorded.
> 
> If we had multiple trees possible in a commit, then all these large scale
> operations would stop being modal and you could just record the unresolved
> merge conflict instead; to come back later and fix it up later.
> 
> I'd be advocating for having multiple trees in a commit
> possible locally; it might be a bad idea to publish such trees.
> 
> Opinions or other use cases?

What benefit does it have over adding a new header "unresolved-tree" or
similar? I do not think you are getting any backwards compatibility
here. For instance, "prune" will not traverse it with existing versions
of git, nor "pack-objects" include it in a pack (I didn't actually test
it, so I could be wrong; but those are all based around parse_commit,
which should look at only the first tree).

-Peff

2018 Award

[marzia]

We want to officially Announce to you and also congratulate you for Emerging as 
One of our lucky winner on our on going promotion of Instagram Mega millions of 
United state Lottery 2018

Division 2018.

BATCH NO:
Draw Results

MEGA PLIER   0
Match:  + Jackpot Winner Estimated Jackpot
Ticket NO:  2018/3883920/11
Trns Code:  00984

contact our Agent for more briefing

Name:  James Harry
E-MAIL:   instagrampromoawa...@gmail.com

Congratulations Once Again!!!

Re: The case for two trees in a commit ("How to make rebase less modal")

[Brandon Williams]

On 03/01, Ramsay Jones wrote:
>
>
> On 28/02/18 23:30, Stefan Beller wrote:
> > $ git hash-object --stdin -w -t commit < > tree c70b4a33a0089f15eb3b38092832388d75293e86
> > parent 105d5b91138ced892765a84e771a061ede8d63b8
> > author Stefan Beller  1519859216 -0800
> > committer Stefan Beller  1519859216 -0800
> > tree 5495266479afc9a4bd9560e9feac465ed43fa63a
> > test commit
> > EOF
> > 19abfc3bf1c5d782045acf23abdf7eed81e16669
> > $ git fsck |grep 19abfc3bf1c5d782045acf23abdf7eed81e16669
> > $
> >
> > So it is technically possible to create a commit with two tree entries
> > and fsck is not complaining.
>
> Hmm, it's a while since I looked at that code, but I don't think
> you have a commit with two trees - the second 'tree ' line
> is just part of the commit message, isn't it?
>
> ATB,
> Ramsay Jones
>

Actually it doesn't look like it.  The commit msg doesn't start till
after an empty newline so that commit has an empty commit msg.  Here's
one which you can see the msg when passed to show:

git hash-object --stdin -w -t commit < 1519859216 -0800
committer Brandon Williams  1519859216 -0800
tree 76d269b57d3c4283922216f84a2850e99f561ccc

This is a test commit with multiple trees
EOF

Of course the extra tree is ignored, but fsck doesn't complain and show
happily shows what it knows about.

--
Brandon Williams

Re: The case for two trees in a commit ("How to make rebase less modal")

[Ramsay Jones]

On 28/02/18 23:30, Stefan Beller wrote:
> $ git hash-object --stdin -w -t commit < tree c70b4a33a0089f15eb3b38092832388d75293e86
> parent 105d5b91138ced892765a84e771a061ede8d63b8
> author Stefan Beller  1519859216 -0800
> committer Stefan Beller  1519859216 -0800
> tree 5495266479afc9a4bd9560e9feac465ed43fa63a
> test commit
> EOF
> 19abfc3bf1c5d782045acf23abdf7eed81e16669
> $ git fsck |grep 19abfc3bf1c5d782045acf23abdf7eed81e16669
> $
> 
> So it is technically possible to create a commit with two tree entries
> and fsck is not complaining.

Hmm, it's a while since I looked at that code, but I don't think
you have a commit with two trees - the second 'tree ' line
is just part of the commit message, isn't it?

ATB,
Ramsay Jones

Re: [PATCH v2] commit: run git gc --auto just before the pre-commit hook

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

>> Ævar Arnfjörð Bjarmason   writes:
>>
>>> Change the behavior of git-commit back to what it was back in
>>> d4bb43ee27 ("Invoke "git gc --auto" from commit, merge, am and
>>> rebase.", 2007-09-05) when it was git-commit.sh.
>>
>> ... which was to run it just before post-commit.  Do I retitle this
>> patch before queuing?
>
> Do'h. Of course I screw up something simple like that, sorry. This v2
> fixes it, and I also rephrased the commit message a bit (more commas &
> full-stops).

I guess I still need to retitle it ;-) But that can happen tomorrow
(I have the previous one with local fixes that pretty much matches
v2 modulo the body of the log message on 'pu', ready to be pushed
out).

> I wonder if it would also be a good idea to run git gc --auto on "git
> push". It itself won't create any objects, but it would be a nice
> proxy in many cases for picking up anything else we missed due to
> various object writing commands that won't run --auto.

Before "push" starts producing a pack might be a good optimization,
as reading from a packed repository is often more performant than a
repository full of loose objects.

The case for two trees in a commit ("How to make rebase less modal")

[Stefan Beller]

$ git hash-object --stdin -w -t commit < 1519859216 -0800
committer Stefan Beller  1519859216 -0800
tree 5495266479afc9a4bd9560e9feac465ed43fa63a
test commit
EOF
19abfc3bf1c5d782045acf23abdf7eed81e16669
$ git fsck |grep 19abfc3bf1c5d782045acf23abdf7eed81e16669
$

So it is technically possible to create a commit with two tree entries
and fsck is not complaining.

But why would I want to do that?

There are multiple abstraction levels in Git, I think of them as follows:
* data structures / object model
* plumbing
* porcelain commands to manipulate the repo "at small scale", e.g.
create a commit/tag
* porcelain to modify the repo "at larger scale", such as rebase,
cherrypicking, reverting
  involving more than 1 commit.

These large scale operations involving multiple commits however
are all modal in its nature. Before doing anything else, you have to
finish or abort the rebase or you need expert knowledge how to
go otherwise.

During the rebase there might be a hard to resolve conflict, which
you may not want to resolve right now, but defer to later.  Deferring a
conflict is currently impossible, because precisely one tree is recorded.

If we had multiple trees possible in a commit, then all these large scale
operations would stop being modal and you could just record the unresolved
merge conflict instead; to come back later and fix it up later.

I'd be advocating for having multiple trees in a commit
possible locally; it might be a bad idea to publish such trees.

Opinions or other use cases?

Thanks,
Stefan

[PATCH v4 18/35] 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 | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 850382f55..695fafe06 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -332,11 +332,25 @@ 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) {
+   if (refspecs[i].pattern)
+   argv_array_push(_patterns, refspecs[i].src);
+   else
+   expand_ref_pattern(_patterns, 
refspecs[i].src);
+   }
+   }
+
+   remote_refs = transport_get_remote_refs(transport, _patterns);
+
+   argv_array_clear(_patterns);
 
if (refspec_count) {
struct refspec *fetch_refspec;
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 26/35] 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.16.2.395.g2e18187dfd-goog

[PATCH v4 32/35] http: don't always add Git-Protocol header

[Brandon Williams]

Instead of always sending the Git-Protocol header with the configured
version with every http request, explicitly send it when discovering
refs and then only send it on subsequent http requests if the server
understood the version requested.

Signed-off-by: Brandon Williams 
---
 http.c| 17 -
 remote-curl.c | 33 +
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/http.c b/http.c
index e1757d62b..8f1129ac7 100644
--- a/http.c
+++ b/http.c
@@ -904,21 +904,6 @@ static void set_from_env(const char **var, const char 
*envname)
*var = val;
 }
 
-static void protocol_http_header(void)
-{
-   if (get_protocol_version_config() > 0) {
-   struct strbuf protocol_header = STRBUF_INIT;
-
-   strbuf_addf(_header, GIT_PROTOCOL_HEADER ": 
version=%d",
-   get_protocol_version_config());
-
-
-   extra_http_headers = curl_slist_append(extra_http_headers,
-  protocol_header.buf);
-   strbuf_release(_header);
-   }
-}
-
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
char *low_speed_limit;
@@ -949,8 +934,6 @@ void http_init(struct remote *remote, const char *url, int 
proactive_auth)
if (remote)
var_override(_proxy_authmethod, 
remote->http_proxy_authmethod);
 
-   protocol_http_header();
-
pragma_header = curl_slist_append(http_copy_default_headers(),
"Pragma: no-cache");
no_pragma_header = curl_slist_append(http_copy_default_headers(),
diff --git a/remote-curl.c b/remote-curl.c
index c54035843..b4e9db85b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -291,6 +291,19 @@ static int show_http_message(struct strbuf *type, struct 
strbuf *charset,
return 0;
 }
 
+static int get_protocol_http_header(enum protocol_version version,
+   struct strbuf *header)
+{
+   if (version > 0) {
+   strbuf_addf(header, GIT_PROTOCOL_HEADER ": version=%d",
+   version);
+
+   return 1;
+   }
+
+   return 0;
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
struct strbuf exp = STRBUF_INIT;
@@ -299,6 +312,8 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
struct strbuf buffer = STRBUF_INIT;
struct strbuf refs_url = STRBUF_INIT;
struct strbuf effective_url = STRBUF_INIT;
+   struct strbuf protocol_header = STRBUF_INIT;
+   struct string_list extra_headers = STRING_LIST_INIT_DUP;
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
struct http_get_options http_options;
@@ -318,11 +333,16 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
strbuf_addf(_url, "service=%s", service);
}
 
+   /* Add the extra Git-Protocol header */
+   if (get_protocol_http_header(get_protocol_version_config(), 
_header))
+   string_list_append(_headers, protocol_header.buf);
+
memset(_options, 0, sizeof(http_options));
http_options.content_type = 
http_options.charset = 
http_options.effective_url = _url;
http_options.base_url = 
+   http_options.extra_headers = _headers;
http_options.initial_request = 1;
http_options.no_cache = 1;
http_options.keep_error = 1;
@@ -389,6 +409,8 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
strbuf_release();
strbuf_release(_url);
strbuf_release();
+   strbuf_release(_header);
+   string_list_clear(_headers, 0);
last_discovery = last;
return last;
 }
@@ -425,6 +447,7 @@ struct rpc_state {
char *service_url;
char *hdr_content_type;
char *hdr_accept;
+   char *protocol_header;
char *buf;
size_t alloc;
size_t len;
@@ -611,6 +634,10 @@ static int post_rpc(struct rpc_state *rpc)
headers = curl_slist_append(headers, needs_100_continue ?
"Expect: 100-continue" : "Expect:");
 
+   /* Add the extra Git-Protocol header */
+   if (rpc->protocol_header)
+   headers = curl_slist_append(headers, rpc->protocol_header);
+
 retry:
slot = get_active_slot();
 
@@ -751,6 +778,11 @@ static int rpc_service(struct rpc_state *rpc, struct 
discovery *heads)
strbuf_addf(, "Accept: application/x-%s-result", svc);
rpc->hdr_accept = strbuf_detach(, NULL);
 
+   if (get_protocol_http_header(heads->version, ))
+   rpc->protocol_header = strbuf_detach(, NULL);
+   else
+   rpc->protocol_header = NULL;
+
while (!err) {
int n = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 
0);
if (!n)

[PATCH v4 27/35] transport-helper: introduce stateless-connect

[Brandon Williams]

Introduce the transport-helper capability 'stateless-connect'.  This
capability indicates that the transport-helper can be requested to run
the 'stateless-connect' command which should attempt to make a
stateless connection with a remote end.  Once established, the
connection can be used by the git client to communicate with
the remote end natively in a stateless-rpc manner as supported by
protocol v2.  This means that the client must send everything the server
needs in a single request as the client must not assume any
state-storing on the part of the server or transport.

If a stateless 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.

A future patch will implement the 'stateless-connect' capability in our
http remote-helper (remote-curl) so that protocol v2 can be used using
the http transport.

Signed-off-by: Brandon Williams 
---
 Documentation/gitremote-helpers.txt | 32 +
 transport-helper.c  | 11 ++
 transport.c |  1 +
 transport.h |  6 ++
 4 files changed, 50 insertions(+)

diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 4a584f3c5..a8361ed95 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -102,6 +102,14 @@ Capabilities for Pushing
 +
 Supported commands: 'connect'.
 
+'stateless-connect'::
+   Experimental; for internal use only.
+   Can attempt to connect to a remote server for communication
+   using git's wire-protocol version 2.  This establishes a
+   stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.
+
 'push'::
Can discover remote refs and push local commits and the
history leading up to them to new or existing remote refs.
@@ -136,6 +144,14 @@ Capabilities for Fetching
 +
 Supported commands: 'connect'.
 
+'stateless-connect'::
+   Experimental; for internal use only.
+   Can attempt to connect to a remote server for communication
+   using git's wire-protocol version 2.  This establishes a
+   stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.
+
 'fetch'::
Can discover remote refs and transfer objects reachable from
them to the local object store.
@@ -375,6 +391,22 @@ Supported if the helper has the "export" capability.
 +
 Supported if the helper has the "connect" capability.
 
+'stateless-connect' ::
+   Experimental; for internal use only.
+   Connects to the given remote service for communication using
+   git's wire-protocol version 2.  This establishes a stateless,
+   half-duplex connection.  Valid replies to this command are empty
+   line (connection established), 'fallback' (no smart transport
+   support, fall back to dumb transports) and just exiting with
+   error message printed (can't connect, don't bother trying to
+   fall back).  After line feed terminating the positive (empty)
+   response, the output of the service starts.  Messages (both
+   request and response) must be terminated with a single flush
+   packet, allowing the remote helper to properly act as a proxy.
+   After the connection ends, the remote helper exits.
++
+Supported if the helper has the "stateless-connect" capability.
+
 If a fatal error occurs, the program writes the error message to
 stderr and exits. The caller should expect that a suitable error
 message has been printed if the child closes the connection without
diff --git a/transport-helper.c b/transport-helper.c
index c032a2a87..e20a5076e 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -12,6 +12,7 @@
 #include "argv-array.h"
 #include "refs.h"
 #include "transport-internal.h"
+#include "protocol.h"
 
 static int debug;
 
@@ -26,6 +27,7 @@ struct helper_data {
option : 1,
push : 1,
connect : 1,
+   stateless_connect : 1,
signed_tags : 1,
check_connectivity : 1,
no_disconnect_req : 1,
@@ -188,6 +190,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, "stateless-connect")) {
+   data->stateless_connect = 1;
} else if (!strcmp(capname, "signed-tags")) {
data->signed_tags = 1;
} else if (skip_prefix(capname, "export-marks ", )) {
@@ -612,6 +616,13 @@ static int process_connect_service(struct transport 
*transport,
if (data->connect) {
strbuf_addf(, "connect %s\n", name);
 

[PATCH v4 24/35] connect: don't request v2 when pushing

[Brandon Williams]

In order to be able to ship protocol v2 with only supporting fetch, we
need clients to not issue a request to use protocol v2 when pushing
(since the client currently doesn't know how to push using protocol v2).
This allows a client to have protocol v2 configured in
`protocol.version` and take advantage of using v2 for fetch and falling
back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when
pushing right now.  This is because currently a server will ignore a request to
use v2 when contacting the 'receive-pack' endpoint and fall back to
using v0, but when push v2 is rolled out to servers, the 'receive-pack'
endpoint will start responding using v2.  So we don't want to get into a
state where a client is requesting to push with v2 before they actually
know how to push using v2.

Signed-off-by: Brandon Williams 
---
 connect.c  |  8 
 t/t5702-protocol-v2.sh | 24 
 2 files changed, 32 insertions(+)

diff --git a/connect.c b/connect.c
index a0bfcdf4f..32284d050 100644
--- a/connect.c
+++ b/connect.c
@@ -1218,6 +1218,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
enum protocol protocol;
enum protocol_version version = get_protocol_version_config();
 
+   /*
+* NEEDSWORK: If we are trying to use protocol v2 and we are planning
+* to perform a push, then fallback to v0 since the client doesn't know
+* how to push yet using v2.
+*/
+   if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
+   version = protocol_v0;
+
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 4365ac273..e3a7c09d4 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -95,6 +95,30 @@ test_expect_success 'pull with git:// using protocol v2' '
grep "fetch< version 2" log
 '
 
+test_expect_success 'push with git:// and a config of v2 does not request v2' '
+   test_when_finished "rm -f log" &&
+
+   # Till v2 for push is designed, make sure that if a client has
+   # protocol.version configured to use v2, that the client instead falls
+   # back and uses v0.
+
+   test_commit -C daemon_child three &&
+
+   # Push to another branch, as the target repository has the
+   # master branch checked out and we cannot push into it.
+   GIT_TRACE_PACKET="$(pwd)/log" git -C daemon_child -c protocol.version=2 
\
+   push origin HEAD:client_branch &&
+
+   git -C daemon_child log -1 --format=%s >actual &&
+   git -C "$daemon_parent" log -1 --format=%s client_branch >expect &&
+   test_cmp expect actual &&
+
+   # Client requested to use protocol v2
+   ! grep "push> .*\\\0\\\0version=2\\\0$" log &&
+   # Server responded using protocol v2
+   ! grep "push< version 2" log
+'
+
 stop_git_daemon
 
 # Test protocol v2 with 'file://' transport
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 28/35] 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 87a24bd17..5223e24e2 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(data, len, 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 3f836f01a..4f97ae3e5 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.16.2.395.g2e18187dfd-goog

[PATCH v4 30/35] remote-curl: store the protocol version the server responded with

[Brandon Williams]

Store the protocol version the server responded with when performing
discovery.  This will be used in a future patch to either change the
'Git-Protocol' header sent in subsequent requests or to determine if a
client needs to fallback to using a different protocol version.

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

diff --git a/remote-curl.c b/remote-curl.c
index 4086aa733..c54035843 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,7 +185,8 @@ 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");
break;
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 31/35] http: allow providing extra headers for http requests

[Brandon Williams]

Add a way for callers to request that extra headers be included when
making http requests.

Signed-off-by: Brandon Williams 
---
 http.c | 8 
 http.h | 7 +++
 2 files changed, 15 insertions(+)

diff --git a/http.c b/http.c
index 597771271..e1757d62b 100644
--- a/http.c
+++ b/http.c
@@ -1723,6 +1723,14 @@ static int http_request(const char *url,
 
headers = curl_slist_append(headers, buf.buf);
 
+   /* Add additional headers here */
+   if (options && options->extra_headers) {
+   const struct string_list_item *item;
+   for_each_string_list_item(item, options->extra_headers) {
+   headers = curl_slist_append(headers, item->string);
+   }
+   }
+
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
diff --git a/http.h b/http.h
index f7bd3b26b..4df4a25e1 100644
--- a/http.h
+++ b/http.h
@@ -172,6 +172,13 @@ struct http_get_options {
 * for details.
 */
struct strbuf *base_url;
+
+   /*
+* If not NULL, contains additional HTTP headers to be sent with the
+* request. The strings in the list must not be freed until after the
+* request has completed.
+*/
+   struct string_list *extra_headers;
 };
 
 /* Return values for http_get_*() */
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 20/35] upload-pack: introduce fetch server command

[Brandon Williams]

Introduce the 'fetch' server command.

Signed-off-by: Brandon Williams 
---
 Documentation/technical/protocol-v2.txt | 128 
 serve.c |   2 +
 t/t5701-git-serve.sh|   1 +
 upload-pack.c   | 267 
 upload-pack.h   |   5 +
 5 files changed, 403 insertions(+)

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index 7f50e6462..99c70a1e4 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -205,3 +205,131 @@ The output of ls-refs is as follows:
 ref-attribute = (symref | peeled)
 symref = "symref-target:" symref-target
 peeled = "peeled:" obj-id
+
+ fetch
+---
+
+`fetch` is the command used to fetch a packfile in v2.  It can be looked
+at as a modified version of the v1 fetch where the ref-advertisement is
+stripped out (since the `ls-refs` command fills that role) and the
+message format is tweaked to eliminate redundancies and permit easy
+addition of future extensions.
+
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features, e.g.  "=
+".
+
+A `fetch` request can take the following arguments:
+
+want 
+   Indicates to the server an object which the client wants to
+   retrieve.  Wants can be anything and are not limited to
+   advertised objects.
+
+have 
+   Indicates to the server an object which the client has locally.
+   This allows the server to make a packfile which only contains
+   the objects that the client needs. Multiple 'have' lines can be
+   supplied.
+
+done
+   Indicates to the server that negotiation should terminate (or
+   not even begin if performing a clone) and that the server should
+   use the information supplied in the request to construct the
+   packfile.
+
+thin-pack
+   Request that a thin pack be sent, which is a pack with deltas
+   which reference base objects not contained within the pack (but
+   are known to exist at the receiving end). This can reduce the
+   network traffic significantly, but it requires the receiving end
+   to know how to "thicken" these packs by adding the missing bases
+   to the pack.
+
+no-progress
+   Request that progress information that would normally be sent on
+   side-band channel 2, during the packfile transfer, should not be
+   sent.  However, the side-band channel 3 is still used for error
+   responses.
+
+include-tag
+   Request that annotated tags should be sent if the objects they
+   point to are being sent.
+
+ofs-delta
+   Indicate that the client understands PACKv2 with delta referring
+   to its base by position in pack rather than by an oid.  That is,
+   they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
+
+The response of `fetch` is broken into a number of sections separated by
+delimiter packets (0001), with each section beginning with its section
+header.
+
+output = *section
+section = (acknowledgments | packfile)
+ (flush-pkt | delim-pkt)
+
+acknowledgments = PKT-LINE("acknowledgments" LF)
+ (nak | *ack)
+ (ready)
+ready = PKT-LINE("ready" LF)
+nak = PKT-LINE("NAK" LF)
+ack = PKT-LINE("ACK" SP obj-id LF)
+
+packfile = PKT-LINE("packfile" LF)
+  [PACKFILE]
+
+
+acknowledgments section
+   * Always begins with the section header "acknowledgments"
+
+   * The server will respond with "NAK" if none of the object ids sent
+ as have lines were common.
+
+   * The server will respond with "ACK obj-id" for all of the
+ object ids sent as have lines which are common.
+
+   * A response cannot have both "ACK" lines as well as a "NAK"
+ line.
+
+   * The server will respond with a "ready" line indicating that
+ the server has found an acceptable common base and is ready to
+ make and send a packfile (which will be found in the packfile
+ section of the same response)
+
+   * If the client determines that it is finished with negotiations
+ by sending a "done" line, the acknowledgments sections MUST be
+ omitted from the server's response.
+
+   * If the server has found a suitable cut point and has decided
+ to send a "ready" line, then the server can decide to (as an
+ optimization) omit any "ACK" lines it would have sent during
+ its response.  This is because the server will have already
+ determined the objects it plans to send to the client and no
+ further negotiation is needed.
+
+
+packfile section
+   * Always begins with the section header "packfile"
+
+   

[PATCH v4 16/35] 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 | 12 +++-
 6 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 284651797..6e77d993f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1121,7 +1121,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..daea4770c 100644
--- a/transport.h
+++ b/transport.h
@@ -178,7 +178,17 @@ 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);
+/*
+ * Retrieve refs from a remote.
+ *
+ * Optionally a list of ref patterns can be provided which can be sent to the
+ * server (when communicating using protocol v2) to enable it to limit the ref
+ * advertisement.  Since ref filtering is done on the server's end (and only
+ * when using protocol v2), this can return refs which don't match the provided
+ * ref_patterns.
+ */
+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.16.2.395.g2e18187dfd-goog

[PATCH v4 17/35] 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| 12 ++--
 refs.c | 14 ++
 refs.h |  7 +++
 t/t5702-protocol-v2.sh | 26 ++
 4 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c6e9847c5..083ba8b29 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "transport.h"
 #include "remote.h"
+#include "refs.h"
 
 static const char * const ls_remote_usage[] = {
N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n"
@@ -43,6 +44,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 +76,14 @@ 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]);
+
+   if (strchr(argv[i], '*'))
+   argv_array_push(_patterns, argv[i]);
+   else
+   expand_ref_pattern(_patterns, argv[i]);
+   }
}
 
remote = remote_get(dest);
@@ -96,7 +104,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/refs.c b/refs.c
index 20ba82b43..58e9f88fb 100644
--- a/refs.c
+++ b/refs.c
@@ -13,6 +13,7 @@
 #include "tag.h"
 #include "submodule.h"
 #include "worktree.h"
+#include "argv-array.h"
 
 /*
  * List of all available backends
@@ -501,6 +502,19 @@ int refname_match(const char *abbrev_name, const char 
*full_name)
return 0;
 }
 
+/*
+ * Given a 'pattern' expand it by the rules in 'ref_rev_parse_rules' and add
+ * the results to 'patterns'
+ */
+void expand_ref_pattern(struct argv_array *patterns, const char *pattern)
+{
+   const char **p;
+   for (p = ref_rev_parse_rules; *p; p++) {
+   int len = strlen(pattern);
+   argv_array_pushf(patterns, *p, len, pattern);
+   }
+}
+
 /*
  * *string and *len will only be substituted, and *string returned (for
  * later free()ing) if the string passed in is a magic short-hand form
diff --git a/refs.h b/refs.h
index 01be5ae32..292ca35ce 100644
--- a/refs.h
+++ b/refs.h
@@ -139,6 +139,13 @@ int resolve_gitlink_ref(const char *submodule, const char 
*refname,
  */
 int refname_match(const char *abbrev_name, const char *full_name);
 
+/*
+ * Given a 'pattern' expand it by the rules in 'ref_rev_parse_rules' and add
+ * the results to 'patterns'
+ */
+struct argv_array;
+void expand_ref_pattern(struct argv_array *patterns, const char *pattern);
+
 int expand_ref(const char *str, int len, struct object_id *oid, char **ref);
 int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
 int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index dc5f813be..562610fd2 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -32,6 +32,19 @@ test_expect_success 'list refs with git:// using protocol 
v2' '
test_cmp actual expect
 '
 
+test_expect_success 'ref advertisment is filtered with ls-remote using 
protocol v2' '
+   test_when_finished "rm -f log" &&
+
+   GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+   ls-remote "$GIT_DAEMON_URL/parent" master >actual &&
+
+   cat >expect <<-EOF &&
+   $(git -C "$daemon_parent" rev-parse refs/heads/master)$(printf 
"\t")refs/heads/master
+   EOF
+
+   test_cmp actual expect
+'
+
 stop_git_daemon
 
 # Test protocol v2 with 'file://' transport
@@ -54,4 +67,17 @@ 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' '
+   test_when_finished "rm -f log" &&
+
+   GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+   ls-remote 

[PATCH v4 25/35] 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.16.2.395.g2e18187dfd-goog

[PATCH v4 23/35] connect: refactor git_connect to only get the protocol version once

[Brandon Williams]

Instead of having each builtin transport asking for which protocol
version the user has configured in 'protocol.version' by calling
`get_protocol_version_config()` multiple times, factor this logic out
so there is just a single call at the beginning of `git_connect()`.

This will be helpful in the next patch where we can have centralized
logic which determines if we need to request a different protocol
version than what the user has configured.

Signed-off-by: Brandon Williams 
---
 connect.c | 27 +++
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/connect.c b/connect.c
index 66a9443c8..a0bfcdf4f 100644
--- a/connect.c
+++ b/connect.c
@@ -1035,6 +1035,7 @@ static enum ssh_variant determine_ssh_variant(const char 
*ssh_command,
  */
 static struct child_process *git_connect_git(int fd[2], char *hostandport,
 const char *path, const char *prog,
+enum protocol_version version,
 int flags)
 {
struct child_process *conn;
@@ -1073,10 +1074,10 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
target_host, 0);
 
/* If using a new version put that stuff here after a second null byte 
*/
-   if (get_protocol_version_config() > 0) {
+   if (version > 0) {
strbuf_addch(, '\0');
strbuf_addf(, "version=%d%c",
-   get_protocol_version_config(), '\0');
+   version, '\0');
}
 
packet_write(fd[1], request.buf, request.len);
@@ -1092,14 +1093,14 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
  */
 static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 enum ssh_variant variant, const char *port,
-int flags)
+enum protocol_version version, int flags)
 {
if (variant == VARIANT_SSH &&
-   get_protocol_version_config() > 0) {
+   version > 0) {
argv_array_push(args, "-o");
argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
-get_protocol_version_config());
+version);
}
 
if (flags & CONNECT_IPV4) {
@@ -1152,7 +1153,8 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
 
 /* Prepare a child_process for use by Git's SSH-tunneled transport. */
 static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
- const char *port, int flags)
+ const char *port, enum protocol_version version,
+ int flags)
 {
const char *ssh;
enum ssh_variant variant;
@@ -1186,14 +1188,14 @@ static void fill_ssh_args(struct child_process *conn, 
const char *ssh_host,
argv_array_push(, ssh);
argv_array_push(, "-G");
push_ssh_options(, _array,
-VARIANT_SSH, port, flags);
+VARIANT_SSH, port, version, flags);
argv_array_push(, ssh_host);
 
variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;
}
 
argv_array_push(>args, ssh);
-   push_ssh_options(>args, >env_array, variant, port, flags);
+   push_ssh_options(>args, >env_array, variant, port, version, 
flags);
argv_array_push(>args, ssh_host);
 }
 
@@ -1214,6 +1216,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
char *hostandport, *path;
struct child_process *conn;
enum protocol protocol;
+   enum protocol_version version = get_protocol_version_config();
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
@@ -1228,7 +1231,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
-   conn = git_connect_git(fd, hostandport, path, prog, flags);
+   conn = git_connect_git(fd, hostandport, path, prog, version, 
flags);
} else {
struct strbuf cmd = STRBUF_INIT;
const char *const *var;
@@ -1271,12 +1274,12 @@ struct child_process *git_connect(int fd[2], const char 
*url,
strbuf_release();
return NULL;
}
-   fill_ssh_args(conn, ssh_host, port, flags);
+   fill_ssh_args(conn, ssh_host, port, version, flags);
} else {

[PATCH v4 22/35] fetch-pack: support shallow requests

[Brandon Williams]

Enable shallow clones and deepen requests using protocol version 2 if
the server 'fetch' command supports the 'shallow' feature.

Signed-off-by: Brandon Williams 
---
 connect.c| 22 
 connect.h|  2 ++
 fetch-pack.c | 71 +++-
 3 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 6203ce576..66a9443c8 100644
--- a/connect.c
+++ b/connect.c
@@ -82,6 +82,28 @@ int server_supports_v2(const char *c, int die_on_error)
return 0;
 }
 
+int server_supports_feature(const char *c, const char *feature,
+   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++) == '=')) {
+   if (parse_feature_request(out, feature))
+   return 1;
+   else
+   break;
+   }
+   }
+
+   if (die_on_error)
+   die("server doesn't support feature '%s'", feature);
+
+   return 0;
+}
+
 static void process_capabilities_v2(struct packet_reader *reader)
 {
while (packet_reader_read(reader) == PACKET_READ_NORMAL)
diff --git a/connect.h b/connect.h
index 8898d4495..0e69c6709 100644
--- a/connect.h
+++ b/connect.h
@@ -17,5 +17,7 @@ struct packet_reader;
 extern enum protocol_version discover_version(struct packet_reader *reader);
 
 extern int server_supports_v2(const char *c, int die_on_error);
+extern int server_supports_feature(const char *c, const char *feature,
+  int die_on_error);
 
 #endif
diff --git a/fetch-pack.c b/fetch-pack.c
index dffcfd66a..837e1fd21 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1008,6 +1008,26 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
return ref;
 }
 
+static void add_shallow_requests(struct strbuf *req_buf,
+const struct fetch_pack_args *args)
+{
+   if (is_repository_shallow())
+   write_shallow_commits(req_buf, 1, NULL);
+   if (args->depth > 0)
+   packet_buf_write(req_buf, "deepen %d", args->depth);
+   if (args->deepen_since) {
+   timestamp_t max_age = approxidate(args->deepen_since);
+   packet_buf_write(req_buf, "deepen-since %"PRItime, max_age);
+   }
+   if (args->deepen_not) {
+   int i;
+   for (i = 0; i < args->deepen_not->nr; i++) {
+   struct string_list_item *s = args->deepen_not->items + 
i;
+   packet_buf_write(req_buf, "deepen-not %s", s->string);
+   }
+   }
+}
+
 static void add_wants(const struct ref *wants, struct strbuf *req_buf)
 {
for ( ; wants ; wants = wants->next) {
@@ -1093,6 +1113,12 @@ static int send_fetch_request(int fd_out, const struct 
fetch_pack_args *args,
if (prefer_ofs_delta)
packet_buf_write(_buf, "ofs-delta");
 
+   /* Add shallow-info and deepen request */
+   if (server_supports_feature("fetch", "shallow", 0))
+   add_shallow_requests(_buf, args);
+   else if (is_repository_shallow() || args->deepen)
+   die(_("Server does not support shallow requests"));
+
/* add wants */
add_wants(wants, _buf);
 
@@ -1122,7 +1148,7 @@ static int process_section_header(struct packet_reader 
*reader,
int ret;
 
if (packet_reader_peek(reader) != PACKET_READ_NORMAL)
-   die("error reading packet");
+   die("error reading section header '%s'", section);
 
ret = !strcmp(reader->line, section);
 
@@ -1177,6 +1203,43 @@ static int process_acks(struct packet_reader *reader, 
struct oidset *common)
return received_ready ? 2 : (received_ack ? 1 : 0);
 }
 
+static void receive_shallow_info(struct fetch_pack_args *args,
+struct packet_reader *reader)
+{
+   process_section_header(reader, "shallow-info", 0);
+   while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+   const char *arg;
+   struct object_id oid;
+
+   if (skip_prefix(reader->line, "shallow ", )) {
+   if (get_oid_hex(arg, ))
+   die(_("invalid shallow line: %s"), 
reader->line);
+   register_shallow();
+   continue;
+   }
+   if (skip_prefix(reader->line, "unshallow ", )) {
+   if (get_oid_hex(arg, ))
+   die(_("invalid unshallow line: %s"), 
reader->line);
+   if (!lookup_object(oid.hash))
+   die(_("object not found: %s"), reader->line);
+   /* make sure that it is parsed 

[PATCH v4 29/35] 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.

Currently, all service names are string constants, but a subsequent
patch will introduce service names from external sources.

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.16.2.395.g2e18187dfd-goog

[PATCH v4 21/35] 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 
---
 Documentation/technical/protocol-v2.txt |  68 +-
 builtin/fetch-pack.c|   2 +-
 fetch-pack.c| 270 +++-
 fetch-pack.h|   4 +-
 serve.c |   2 +-
 t/t5701-git-serve.sh|   2 +-
 t/t5702-protocol-v2.sh  |  97 +
 transport.c |   7 +-
 upload-pack.c   | 141 ++---
 upload-pack.h   |   3 +
 10 files changed, 548 insertions(+), 48 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index 99c70a1e4..0d63456fc 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -262,12 +262,43 @@ A `fetch` request can take the following arguments:
to its base by position in pack rather than by an oid.  That is,
they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
 
+shallow 
+   A client must notify the server of all commits for which it only
+   has shallow copies (meaning that it doesn't have the parents of
+   a commit) by supplying a 'shallow ' line for each such
+   object so that the server is aware of the limitations of the
+   client's history.  This is so that the server is aware that the
+   client may not have all objects reachable from such commits.
+
+deepen 
+   Requests that the fetch/clone should be shallow having a commit
+   depth of  relative to the remote side.
+
+deepen-relative
+   Requests that the semantics of the "deepen" command be changed
+   to indicate that the depth requested is relative to the client's
+   current shallow boundary, instead of relative to the requested
+   commits.
+
+deepen-since 
+   Requests that the shallow clone/fetch should be cut at a
+   specific time, instead of depth.  Internally it's equivalent to
+   doing "git rev-list --max-age=". Cannot be used with
+   "deepen".
+
+deepen-not 
+   Requests that the shallow clone/fetch should be cut at a
+   specific revision specified by '', instead of a depth.
+   Internally it's equivalent of doing "git rev-list --not ".
+   Cannot be used with "deepen", but can be used with
+   "deepen-since".
+
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
 header.
 
 output = *section
-section = (acknowledgments | packfile)
+section = (acknowledgments | shallow-info | packfile)
  (flush-pkt | delim-pkt)
 
 acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -277,6 +308,11 @@ header.
 nak = PKT-LINE("NAK" LF)
 ack = PKT-LINE("ACK" SP obj-id LF)
 
+shallow-info = PKT-LINE("shallow-info" LF)
+  *PKT-LINE((shallow | unshallow) LF)
+shallow = "shallow" SP obj-id
+unshallow = "unshallow" SP obj-id
+
 packfile = PKT-LINE("packfile" LF)
   [PACKFILE]
 
@@ -309,6 +345,36 @@ header.
  determined the objects it plans to send to the client and no
  further negotiation is needed.
 
+
+shallow-info section
+   If the client has requested a shallow fetch/clone, a shallow
+   client requests a fetch or the server is shallow then the
+   server's response may include a shallow-info section.  The
+   shallow-info section will be included if (due to one of the
+   above conditions) the server needs to inform the client of any
+   shallow boundaries or adjustments to the clients already
+   existing shallow boundaries.
+
+   * Always begins with the section header "shallow-info"
+
+   * If a positive depth is requested, the server will compute the
+ set of commits which are no deeper than the desired depth.
+
+   * The server sends a "shallow obj-id" line for each commit whose
+ parents will not be sent in the following packfile.
+
+   * The server sends an "unshallow obj-id" line for each commit
+ which the client has indicated is shallow, but is no longer
+ shallow as a result of the fetch (due to its parents being
+ sent in the following packfile).
+
+   * The server MUST NOT send any "unshallow" lines for anything
+ which the client has not indicated was shallow as a part of
+ its request.
+
+   * This section is only included if a packfile section is also
+ included in the response.
+
 
 packfile section
* Always begins with the section header "packfile"
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index b2374ddbb..f9d7d0b5a 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -212,7 

[PATCH v4 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

Teach remote-curl the 'stateless-connect' command which is used to
establish a stateless 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  | 205 -
 t/t5702-protocol-v2.sh |  45 +
 2 files changed, 249 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 66a53f74b..3f882d766 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
heads->version = discover_version();
switch (heads->version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   /*
+* Do nothing.  This isn't a list of refs but rather a
+* capability advertisement.  Client would have run
+* 'stateless-connect' so we'll dump this capability listing
+* and let them request the refs themselves.
+*/
break;
case protocol_v1:
case protocol_v0:
@@ -1085,6 +1090,200 @@ static void parse_push(struct strbuf *buf)
free(specs);
 }
 
+/*
+ * Used to represent the state of a connection to an HTTP server when
+ * communicating using git's wire-protocol version 2.
+ */
+struct proxy_state {
+   char *service_name;
+   char *service_url;
+   struct curl_slist *headers;
+   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,
+enum protocol_version version)
+{
+   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);
+
+   p->headers = http_copy_default_headers();
+
+   strbuf_addf(, "Content-Type: application/x-%s-request", 
p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   strbuf_addf(, "Accept: application/x-%s-result", p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   p->headers = curl_slist_append(p->headers, "Transfer-Encoding: 
chunked");
+
+   /* Add the Git-Protocol header */
+   if (get_protocol_http_header(version, ))
+   p->headers = curl_slist_append(p->headers, buf.buf);
+
+   packet_reader_init(>reader, p->in, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF);
+
+   strbuf_release();
+}
+
+static void proxy_state_clear(struct proxy_state *p)
+{
+   free(p->service_name);
+   free(p->service_url);
+   curl_slist_free_all(p->headers);
+   strbuf_release(>request_buffer);
+}
+
+/*
+ * CURLOPT_READFUNCTION callback function.
+ * Attempts to copy over a single packet-line at a time into the
+ * curl provided buffer.
+ */
+static size_t proxy_in(char *buffer, size_t eltsize,
+  size_t nmemb, void *userdata)
+{
+   size_t max;
+   struct proxy_state *p = userdata;
+   size_t avail = p->request_buffer.len - p->pos;
+
+
+   if (eltsize != 1)
+   BUG("curl read callback called with size = %zu != 1", eltsize);
+   max = nmemb;
+
+   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("unexpected EOF when reading 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(buffer, p->request_buffer.buf + p->pos, avail);
+   p->pos += avail;
+   return avail;
+}
+
+static size_t proxy_out(char *buffer, size_t eltsize,
+  

[PATCH v4 33/35] http: eliminate "# service" line when using protocol v2

[Brandon Williams]

When an http info/refs request is made, requesting that protocol v2 be
used, don't send a "# service" line since this line is not part of the
v2 spec.

Signed-off-by: Brandon Williams 
---
 http-backend.c | 8 ++--
 remote-curl.c  | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index f3dc218b2..5d241e910 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -10,6 +10,7 @@
 #include "url.h"
 #include "argv-array.h"
 #include "packfile.h"
+#include "protocol.h"
 
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
@@ -466,8 +467,11 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
hdr_str(hdr, content_type, buf.buf);
end_headers(hdr);
 
-   packet_write_fmt(1, "# service=git-%s\n", svc->name);
-   packet_flush(1);
+
+   if (determine_protocol_version_server() != protocol_v2) {
+   packet_write_fmt(1, "# service=git-%s\n", svc->name);
+   packet_flush(1);
+   }
 
argv[0] = svc->name;
run_service(argv, 0);
diff --git a/remote-curl.c b/remote-curl.c
index b4e9db85b..66a53f74b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -396,6 +396,9 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
;
 
last->proto_git = 1;
+   } else if (maybe_smart &&
+  last->len > 5 && starts_with(last->buf + 4, "version 2")) {
+   last->proto_git = 1;
}
 
if (last->proto_git)
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

In order to be able to ship protocol v2 with only supporting fetch, we
need clients to not issue a request to use protocol v2 when pushing
(since the client currently doesn't know how to push using protocol v2).
This allows a client to have protocol v2 configured in
`protocol.version` and take advantage of using v2 for fetch and falling
back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when
pushing right now.  This is because currently a server will ignore a request to
use v2 when contacting the 'receive-pack' endpoint and fall back to
using v0, but when push v2 is rolled out to servers, the 'receive-pack'
endpoint will start responding using v2.  So we don't want to get into a
state where a client is requesting to push with v2 before they actually
know how to push using v2.

Signed-off-by: Brandon Williams 
---
 remote-curl.c  | 11 ++-
 t/t5702-protocol-v2.sh | 24 
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 3f882d766..379ab9b21 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -322,6 +322,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
struct http_get_options http_options;
+   enum protocol_version version = get_protocol_version_config();
 
if (last && !strcmp(service, last->service))
return last;
@@ -338,8 +339,16 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
strbuf_addf(_url, "service=%s", service);
}
 
+   /*
+* NEEDSWORK: If we are trying to use protocol v2 and we are planning
+* to perform a push, then fallback to v0 since the client doesn't know
+* how to push yet using v2.
+*/
+   if (version == protocol_v2 && !strcmp("git-receive-pack", service))
+   version = protocol_v0;
+
/* Add the extra Git-Protocol header */
-   if (get_protocol_http_header(get_protocol_version_config(), 
_header))
+   if (get_protocol_http_header(version, _header))
string_list_append(_headers, protocol_header.buf);
 
memset(_options, 0, sizeof(http_options));
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 124063c2c..56f7c3c32 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -244,6 +244,30 @@ test_expect_success 'fetch with http:// using protocol v2' 
'
grep "git< version 2" log
 '
 
+test_expect_success 'push with http:// and a config of v2 does not request v2' 
'
+   test_when_finished "rm -f log" &&
+   # Till v2 for push is designed, make sure that if a client has
+   # protocol.version configured to use v2, that the client instead falls
+   # back and uses v0.
+
+   test_commit -C http_child three &&
+
+   # Push to another branch, as the target repository has the
+   # master branch checked out and we cannot push into it.
+   GIT_TRACE_PACKET="$(pwd)/log" git -C http_child -c protocol.version=2 \
+   push origin HEAD:client_branch &&
+
+   git -C http_child log -1 --format=%s >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
client_branch >expect &&
+   test_cmp expect actual &&
+
+   # Client didnt request to use protocol v2
+   ! grep "Git-Protocol: version=2" log &&
+   # Server didnt respond using protocol v2
+   ! grep "git< version 2" log
+'
+
+
 stop_httpd
 
 test_done
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 15/35] 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 |  9 -
 transport.c  | 16 +---
 3 files changed, 20 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..36fcee437 100644
--- a/transport-internal.h
+++ b/transport-internal.h
@@ -3,6 +3,7 @@
 
 struct ref;
 struct transport;
+struct argv_array;
 
 struct transport_vtable {
/**
@@ -17,11 +18,17 @@ struct transport_vtable {
 * the transport to try to share connections, for_push is a
 * hint as to whether the ultimate operation is a push or a fetch.
 *
+* If communicating using protocol v2 a list of patterns can be
+* provided to be sent to the server to enable it to limit the ref
+* advertisement.  Since ref filtering is done on the server's end,
+* this can return refs which don't match the provided ref_patterns.
+*
 * If the transport is able to determine the remote hash for
 * 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 

[PATCH v4 13/35] 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 |  36 ++
 Makefile|   1 +
 ls-refs.c   | 144 
 ls-refs.h   |   9 ++
 serve.c |   8 ++
 t/t5701-git-serve.sh| 115 +++
 6 files changed, 313 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 b02eefc21..7f50e6462 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -169,3 +169,39 @@ printable ASCII characters except space (i.e., the byte 
range 32 < x <
 "git/1.8.3.1"). The agent strings are purely informative for statistics
 and debugging purposes, and MUST NOT be used to programmatically assume
 the presence or absence of particular features.
+
+ ls-refs
+-
+
+`ls-refs` is the command used to request a reference advertisement in v2.
+Unlike the current reference advertisement, ls-refs takes in arguments
+which can be used to limit the refs sent from the server.
+
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features, e.g.  "=
+".
+
+ls-refs takes in the following arguments:
+
+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 one of the provided
+   patterns are displayed.  A pattern is either a valid refname
+   (e.g.  refs/heads/master), in which a ref must match the pattern
+   exactly, or a prefix of a ref followed by a single '*' wildcard
+   character (e.g. refs/heads/*), in which a ref must have a prefix
+   equal to the pattern up to the wildcard character.
+
+The output of ls-refs is as follows:
+
+output = *ref
+flush-pkt
+ref = PKT-LINE(obj-id SP refname *(SP ref-attribute) LF)
+ref-attribute = (symref | peeled)
+symref = "symref-target:" symref-target
+peeled = "peeled:" obj-id
diff --git a/Makefile b/Makefile
index 18c255428..e50927cfb 100644
--- a/Makefile
+++ b/Makefile
@@ -825,6 +825,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..91d7deb34
--- /dev/null
+++ b/ls-refs.c
@@ -0,0 +1,144 @@
+#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 ref_pattern {
+   char *pattern;
+   int wildcard_pos; /* If > 0, indicates the position of the wildcard */
+};
+
+struct pattern_list {
+   struct ref_pattern *patterns;
+   int nr;
+   int alloc;
+};
+
+static void add_pattern(struct pattern_list *patterns, const char *pattern)
+{
+   struct ref_pattern p;
+   const char *wildcard;
+
+   p.pattern = strdup(pattern);
+
+   wildcard = strchr(pattern, '*');
+   if (wildcard) {
+   p.wildcard_pos = wildcard - pattern;
+   } else {
+   p.wildcard_pos = -1;
+   }
+
+   ALLOC_GROW(patterns->patterns,
+  patterns->nr + 1,
+  patterns->alloc);
+   patterns->patterns[patterns->nr++] = p;
+}
+
+static void clear_patterns(struct pattern_list *patterns)
+{
+   int i;
+   for (i = 0; i < patterns->nr; i++)
+   free(patterns->patterns[i].pattern);
+   FREE_AND_NULL(patterns->patterns);
+   patterns->nr = 0;
+   patterns->alloc = 0;
+}
+
+/*
+ * 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 pattern_list *patterns, const char *refname)
+{
+   int i;
+
+   if (!patterns->nr)
+   return 1; /* no restriction */
+
+   for (i = 0; i < patterns->nr; i++) {
+   const struct ref_pattern *p = >patterns[i];
+
+   /* No wildcard, exact match expected */
+   if (p->wildcard_pos < 0) {
+   if (!strcmp(refname, p->pattern))
+   return 1;
+   } else {
+   /* 

[PATCH v4 19/35] 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 | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index dfc603b36..bf7ba6879 100644
--- a/transport.c
+++ b/transport.c
@@ -1026,11 +1026,35 @@ 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++) {
+   const char *pattern = NULL;
+
+   if (tmp_rs[i].dst)
+   pattern = tmp_rs[i].dst;
+   else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
+   pattern = tmp_rs[i].src;
+
+   if (pattern) {
+   if (tmp_rs[i].pattern)
+   argv_array_push(_patterns, pattern);
+   else
+   expand_ref_pattern(_patterns, 
pattern);
+   }
+   }
+
+   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.16.2.395.g2e18187dfd-goog

[PATCH v4 14/35] 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 
---
 builtin/upload-pack.c  |  10 +--
 connect.c  | 138 +++--
 connect.h  |   2 +
 remote.h   |   6 ++
 t/t5702-protocol-v2.sh |  57 +
 transport.c|   2 +-
 6 files changed, 204 insertions(+), 11 deletions(-)
 create mode 100755 t/t5702-protocol-v2.sh

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 8d53e9794..a757df8da 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "protocol.h"
 #include "upload-pack.h"
+#include "serve.h"
 
 static const char * const upload_pack_usage[] = {
N_("git upload-pack [] "),
@@ -16,6 +17,7 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
const char *dir;
int strict = 0;
struct upload_pack_options opts = { 0 };
+   struct serve_options serve_opts = SERVE_OPTIONS_INIT;
struct option options[] = {
OPT_BOOL(0, "stateless-rpc", _rpc,
 N_("quit after a single request/response exchange")),
@@ -48,11 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
 
switch (determine_protocol_version_server()) {
case protocol_v2:
-   /*
-* fetch support for protocol v2 has not been implemented yet,
-* so ignore the request to use v2 and fallback to using v0.
-*/
-   upload_pack();
+   serve_opts.advertise_capabilities = opts.advertise_refs;
+   serve_opts.stateless_rpc = opts.stateless_rpc;
+   serve(_opts);
break;
case protocol_v1:
/*
diff --git a/connect.c b/connect.c
index 4b89b984c..6203ce576 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 char *server_capabilities_v1;
+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."));
 }
 
+/* Checks if the server supports the capability 'c' */
+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("expected flush after capabilities");
+}
+
 enum protocol_version discover_version(struct packet_reader *reader)
 {
enum protocol_version version = protocol_unknown_version;
@@ -84,7 +113,7 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
 
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 */
@@ -128,7 +157,7 @@ static void parse_one_symref_info(struct string_list 
*symref, const char *val, i
 static void annotate_refs_with_symref_info(struct ref *ref)
 {
struct string_list symref = STRING_LIST_INIT_DUP;
-   const char *feature_list = server_capabilities;
+   const char *feature_list = server_capabilities_v1;
 
while (feature_list) {
int len;
@@ -157,7 +186,7 @@ static void process_capabilities(const char *line, int *len)
int nul_location = strlen(line);
if (nul_location == *len)
return;
-   server_capabilities = xstrdup(line + nul_location + 1);
+   server_capabilities_v1 = xstrdup(line + nul_location + 1);
*len = nul_location;
 }
 
@@ -292,6 +321,105 @@ struct ref **get_remote_heads(struct packet_reader 
*reader,
return list;
 }
 
+/* Returns 1 when a valid ref has been added to `list`, 0 otherwise */
+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;
+   

[PATCH v4 08/35] 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| 27 ++-
 connect.h|  3 +++
 remote-curl.c| 20 ++--
 remote.h |  5 +++--
 transport.c  | 24 +++-
 7 files changed, 83 insertions(+), 29 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 c82c90b7c..0b111e62d 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;
 
@@ -233,7 +233,7 @@ enum get_remote_heads_state {
 /*
  * 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)
@@ -241,24 +241,17 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
struct ref **orig_list = list;
int len = 0;
enum get_remote_heads_state 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;
 
while 

[PATCH v4 12/35] 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 | 171 
 Makefile|   2 +
 builtin.h   |   1 +
 builtin/serve.c |  30 +++
 git.c   |   1 +
 serve.c | 250 
 serve.h |  15 ++
 t/t5701-git-serve.sh|  60 ++
 9 files changed, 531 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
 create mode 100755 t/t5701-git-serve.sh

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..b02eefc21
--- /dev/null
+++ b/Documentation/technical/protocol-v2.txt
@@ -0,0 +1,171 @@
+ 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
+  * 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
+  * Designed with http and stateless-rpc in mind.  With clear flush
+semantics the http remote helper can simply act as a proxy
+
+ Detailed Design
+=
+
+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.
+
+ Packet-Line Framing
+-
+
+All communication is done using packet-line framing, just as in v1.  See
+`Documentation/technical/pack-protocol.txt` and
+`Documentation/technical/protocol-common.txt` for more information.
+
+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
+
+ Initial Client Request
+
+
+In general a client can request to speak protocol v2 by sending
+`version=2` through the respective side-channel for the transport being
+used which inevitably sets `GIT_PROTOCOL`.  More information can be
+found in `pack-protocol.txt` and `http-protocol.txt`.  In all cases the
+response from the server is the capability advertisement.
+
+ Git Transport
+~~~
+
+When using the git:// transport, you can request to use protocol v2 by
+sending "version=2" as an extra parameter:
+
+   003egit-upload-pack /project.git\0host=myserver.com\0\0version=2\0
+
+ SSH and File Transport
+
+
+When using either the ssh:// or file:// transport, the GIT_PROTOCOL
+environment variable must be set explicitly to include "version=2".
+
+ HTTP Transport
+
+
+When using the http:// or https:// transport a client makes a "smart"
+info/refs request as described in `http-protocol.txt` and requests that
+v2 be used by supplying "version=2" in the `Git-Protocol` header.
+
+   C: Git-Protocol: version=2
+   C:
+   C: GET $GIT_URL/info/refs?service=git-upload-pack HTTP/1.0
+
+A v2 server would reply:
+
+   S: 200 OK
+   S: 
+   S: ...
+   S:
+   S: 000eversion 2\n
+   S: 
+
+Subsequent requests are then made directly to the 

[PATCH v4 10/35] 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   | 2 ++
 builtin/receive-pack.c | 6 ++
 builtin/send-pack.c| 3 +++
 builtin/upload-pack.c  | 7 +++
 connect.c  | 3 +++
 protocol.c | 2 ++
 protocol.h | 1 +
 remote-curl.c  | 3 +++
 transport.c| 9 +
 9 files changed, 36 insertions(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 85d4faf76..b2374ddbb 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -201,6 +201,8 @@ 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");
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/builtin/upload-pack.c b/builtin/upload-pack.c
index 2cb5cb35b..8d53e9794 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -47,6 +47,13 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
die("'%s' does not appear to be a git repository", dir);
 
switch (determine_protocol_version_server()) {
+   case protocol_v2:
+   /*
+* fetch support for protocol v2 has not been implemented yet,
+* so ignore the request to use v2 and fallback to using v0.
+*/
+   upload_pack();
+   break;
case protocol_v1:
/*
 * v1 is just the original protocol with a version string,
diff --git a/connect.c b/connect.c
index 0b111e62d..4b89b984c 100644
--- a/connect.c
+++ b/connect.c
@@ -83,6 +83,9 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
}
 
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 

[PATCH v4 11/35] test-pkt-line: introduce a packet-line test helper

[Brandon Williams]

Introduce a packet-line test helper which can either pack or unpack an
input stream into packet-lines and writes out the result to stdout.

Signed-off-by: Brandon Williams 
---
 Makefile |  1 +
 t/helper/test-pkt-line.c | 64 
 2 files changed, 65 insertions(+)
 create mode 100644 t/helper/test-pkt-line.c

diff --git a/Makefile b/Makefile
index b7ccc05fa..3b849c060 100644
--- a/Makefile
+++ b/Makefile
@@ -669,6 +669,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-pkt-line
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-write-cache
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
new file mode 100644
index 0..0f19e53c7
--- /dev/null
+++ b/t/helper/test-pkt-line.c
@@ -0,0 +1,64 @@
+#include "pkt-line.h"
+
+static void pack_line(const char *line)
+{
+   if (!strcmp(line, "") || !strcmp(line, "\n"))
+   packet_flush(1);
+   else if (!strcmp(line, "0001") || !strcmp(line, "0001\n"))
+   packet_delim(1);
+   else
+   packet_write_fmt(1, "%s", line);
+}
+
+static void pack(int argc, const char **argv)
+{
+   if (argc) { /* read from argv */
+   int i;
+   for (i = 0; i < argc; i++)
+   pack_line(argv[i]);
+   } else { /* read from stdin */
+   char line[LARGE_PACKET_MAX];
+   while (fgets(line, sizeof(line), stdin)) {
+   pack_line(line);
+   }
+   }
+}
+
+static void unpack(void)
+{
+   struct packet_reader reader;
+   packet_reader_init(, 0, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF |
+  PACKET_READ_CHOMP_NEWLINE);
+
+   while (packet_reader_read() != PACKET_READ_EOF) {
+   switch (reader.status) {
+   case PACKET_READ_EOF:
+   break;
+   case PACKET_READ_NORMAL:
+   printf("%s\n", reader.line);
+   break;
+   case PACKET_READ_FLUSH:
+   printf("\n");
+   break;
+   case PACKET_READ_DELIM:
+   printf("0001\n");
+   break;
+   }
+   }
+}
+
+int cmd_main(int argc, const char **argv)
+{
+   if (argc < 2)
+   die("too few arguments");
+
+   if (!strcmp(argv[1], "pack"))
+   pack(argc - 2, argv + 2);
+   else if (!strcmp(argv[1], "unpack"))
+   unpack();
+   else
+   die("invalid argument '%s'", argv[1]);
+
+   return 0;
+}
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 06/35] 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.16.2.395.g2e18187dfd-goog

[PATCH v4 05/35] 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 2ad73a98b..1e8a9e1ca 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -724,6 +724,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 = (int)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;
@@ -745,49 +814,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 v4 03/35] 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.

Documentation for how this packet will be used in protocol v2 will
included in a future patch.

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

diff --git a/pkt-line.c b/pkt-line.c
index 6307fa4a3..87a24bd17 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";
@@ -298,6 +310,9 @@ enum packet_read_status packet_read_with_status(int fd, 
char **src_buffer,
} else if (!len) {
packet_trace("", 4, 0);
return PACKET_READ_FLUSH;
+   } else if (len == 1) {
+   packet_trace("0001", 4, 0);
+   return PACKET_READ_DELIM;
} else if (len < 4) {
die("protocol error: bad line length %d", len);
}
@@ -331,6 +346,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;
@@ -443,6 +459,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 f2edfae9a..3f836f01a 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,
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 02/35] pkt-line: allow peeking a packet line without consuming it

[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 | 58 +
 2 files changed, 117 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 08e5ba44c..6307fa4a3 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -404,3 +404,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 0be691116..f2edfae9a 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -112,6 +112,64 @@ 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.
+ *
+ * Peeking multiple times without calling 'packet_reader_read()' will return
+ * the same result.
+ */
+extern enum packet_read_status 

[PATCH v4 09/35] 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.16.2.395.g2e18187dfd-goog

[PATCH v4 01/35] 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 | 16 
 2 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 2827ca772..08e5ba44c 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -280,28 +280,34 @@ 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)
+
+   if (len < 0) {
die("protocol error: bad line length character: %.4s", linelen);
-   if (!len) {
+   } else if (!len) {
packet_trace("", 4, 0);
-   return 0;
+   return PACKET_READ_FLUSH;
+   } else if (len < 4) {
+   die("protocol error: bad line length %d", len);
}
+
len -= 4;
-   if (len >= size)
+   if ((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 +315,28 @@ 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)
+{
+   int pktlen;
+
+   switch (packet_read_with_status(fd, src_buffer, src_len, buffer, size,
+   , options)) {
+   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..0be691116 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -65,6 +65,22 @@ 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.16.2.395.g2e18187dfd-goog

[PATCH v4 07/35] 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 | 173 ++
 1 file changed, 95 insertions(+), 78 deletions(-)

diff --git a/connect.c b/connect.c
index c3a014c5b..c82c90b7c 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,40 @@ 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;
+   }
+
+   switch (version) {
+   case protocol_v1:
+   /* Read the peeked version line */
+   packet_reader_read(reader);
+   break;
+   case protocol_v0:
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
+
+   return version;
+}
+
 static void parse_one_symref_info(struct string_list *symref, const char *val, 
int len)
 {
char *sym, *target;
@@ -109,60 +149,21 @@ 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)
+static void process_capabilities(const char *line, int *len)
 {
-   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");
-   }
-}
-
-static void process_capabilities(int *len)
-{
-   int nul_location = strlen(packet_buffer);
+   int nul_location = strlen(line);
if (nul_location == *len)
return;
-   server_capabilities = xstrdup(packet_buffer + nul_location + 1);
+   server_capabilities = xstrdup(line + nul_location + 1);
*len = nul_location;
 }
 
-static int process_dummy_ref(void)
+static int process_dummy_ref(const char *line)
 {
struct object_id oid;
const char *name;
 
-   if (parse_oid_hex(packet_buffer, , ))
+   if (parse_oid_hex(line, , ))
return 0;
if (*name != ' ')
return 0;
@@ -171,20 +172,20 @@ static int process_dummy_ref(void)
return !oidcmp(_oid, ) && 

[PATCH v4 04/35] 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 +
 builtin/upload-pack.c |  67 ++
 git.c |   1 +
 upload-pack.c | 107 ++
 upload-pack.h |  13 +
 6 files changed, 109 insertions(+), 83 deletions(-)
 create mode 100644 builtin/upload-pack.c
 create mode 100644 upload-pack.h

diff --git a/Makefile b/Makefile
index 1a9b23b67..b7ccc05fa 100644
--- a/Makefile
+++ b/Makefile
@@ -639,7 +639,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
@@ -909,6 +908,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
@@ -1026,6 +1026,7 @@ BUILTIN_OBJS += builtin/update-index.o
 BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
 BUILTIN_OBJS += builtin/upload-archive.o
+BUILTIN_OBJS += builtin/upload-pack.o
 BUILTIN_OBJS += builtin/var.o
 BUILTIN_OBJS += builtin/verify-commit.o
 BUILTIN_OBJS += builtin/verify-pack.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/builtin/upload-pack.c b/builtin/upload-pack.c
new file mode 100644
index 0..2cb5cb35b
--- /dev/null
+++ b/builtin/upload-pack.c
@@ -0,0 +1,67 @@
+#include "cache.h"
+#include "builtin.h"
+#include "exec_cmd.h"
+#include "pkt-line.h"
+#include "parse-options.h"
+#include "protocol.h"
+#include "upload-pack.h"
+
+static const char * const upload_pack_usage[] = {
+   N_("git upload-pack [] "),
+   NULL
+};
+
+int cmd_upload_pack(int argc, const char **argv, const char *prefix)
+{
+   const char *dir;
+   int strict = 0;
+   struct upload_pack_options opts = { 0 };
+   struct option options[] = {
+   OPT_BOOL(0, "stateless-rpc", _rpc,
+N_("quit after a single request/response exchange")),
+   OPT_BOOL(0, "advertise-refs", _refs,
+N_("exit immediately after initial ref 
advertisement")),
+   OPT_BOOL(0, "strict", ,
+N_("do not try /.git/ if  is no 
Git directory")),
+   OPT_INTEGER(0, "timeout", ,
+   N_("interrupt transfer after  seconds of 
inactivity")),
+   OPT_END()
+   };
+
+   packet_trace_identity("upload-pack");
+   check_replace_refs = 0;
+
+   argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(upload_pack_usage, options);
+
+   if (opts.timeout)
+   opts.daemon_mode = 1;
+
+   setup_path();
+
+   dir = argv[0];
+
+   if (!enter_repo(dir, strict))
+   die("'%s' does not appear to be a git repository", dir);
+
+   switch (determine_protocol_version_server()) {
+   case protocol_v1:
+   /*
+* v1 is just the original protocol with a version string,
+* so just fall through after writing the version string.
+*/
+   if (opts.advertise_refs || !opts.stateless_rpc)
+   packet_write_fmt(1, "version 1\n");
+
+   /* fallthrough */
+   case protocol_v0:
+   upload_pack();
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
+
+   return 0;
+}
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 },

[PATCH v4 00/35] protocol version 2

[Brandon Williams]

Lots of changes since v3 (well more than between v2 and v3).  Thanks for
all of the reviews on the last round, the series is getting more
polished.

 * Eliminated the "# service" line from the response from an HTTP
   server.  This means that the response to a v2 request is exactly the
   same regardless of which transport you use!  Docs for this have been
   added as well.
 * Changed how ref-patterns work with the `ls-refs` command.  Instead of
   using wildmatch all patterns must either match exactly or they can
   contain a single '*' character at the end to mean that the prefix
   must match.  Docs for this have also been added.
 * Lots of updates to the docs.  Including documenting the
   `stateless-connect` remote-helper command used by remote-curl to
   handle the http transport.
 * Fixed a number of bugs with the `fetch` command, one of which didn't
   use objects from configured alternates.

Brandon Williams (35):
  pkt-line: introduce packet_read_with_status
  pkt-line: allow peeking a packet line without consuming it
  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
  test-pkt-line: introduce a packet-line test helper
  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
  fetch-pack: support shallow requests
  connect: refactor git_connect to only get the protocol version once
  connect: don't request v2 when pushing
  transport-helper: remove name parameter
  transport-helper: refactor process_connect_service
  transport-helper: introduce stateless-connect
  pkt-line: add packet_buf_write_len function
  remote-curl: create copy of the service name
  remote-curl: store the protocol version the server responded with
  http: allow providing extra headers for http requests
  http: don't always add Git-Protocol header
  http: eliminate "# service" line when using protocol v2
  remote-curl: implement stateless-connect command
  remote-curl: don't request v2 when pushing

 .gitignore  |   1 +
 Documentation/gitremote-helpers.txt |  32 ++
 Documentation/technical/protocol-v2.txt | 401 +++
 Makefile|   7 +-
 builtin.h   |   2 +
 builtin/clone.c |   2 +-
 builtin/fetch-pack.c|  20 +-
 builtin/fetch.c |  18 +-
 builtin/ls-remote.c |  12 +-
 builtin/receive-pack.c  |   6 +
 builtin/remote.c|   2 +-
 builtin/send-pack.c |  20 +-
 builtin/serve.c |  30 ++
 builtin/upload-pack.c   |  74 +++
 connect.c   | 364 ++
 connect.h   |   7 +
 fetch-pack.c| 339 -
 fetch-pack.h|   4 +-
 git.c   |   2 +
 http-backend.c  |   8 +-
 http.c  |  25 +-
 http.h  |   7 +
 ls-refs.c   | 144 ++
 ls-refs.h   |   9 +
 pkt-line.c  | 147 +-
 pkt-line.h  |  78 +++
 protocol.c  |   2 +
 protocol.h  |   1 +
 refs.c  |  14 +
 refs.h  |   7 +
 remote-curl.c   | 278 ++-
 remote.h|  11 +-
 serve.c | 260 ++
 serve.h |  15 +
 t/helper/test-pkt-line.c|  64 +++
 t/t5701-git-serve.sh| 176 +++
 t/t5702-protocol-v2.sh  | 273 +++
 transport-helper.c  |  87 ++--
 transport-internal.h|   9 +-
 transport.c | 125 +++--
 transport.h |  18 +-
 upload-pack.c   | 616 ++--
 upload-pack.h   |  21 +
 43 files changed, 3370 insertions(+), 368 deletions(-)
 create 

Re: [PATCH v2 0/5] roll back locks in various code paths

[Junio C Hamano]

Martin Ågren  writes:

> A further upshot of this patch is that `active_cache_changed`, which is
> defined as `the_index.cache_changed`, now only has a few users left.

I am undecided if this is a *good* thing.  In a few codepaths where
we make a speculative update to the on-disk index file, I find that
the explicit mention of active_cache_changed clarifies what is going
on.  Perhaps once my (and other old timers') eyes get used to this
new SKIP_IF_UNCHANGED symbol, it would start serving the same
purpose ;-)

> We could have made the new flag behave the other way round
> ("FORCE_WRITE"), but that could break existing users behind their backs.

Yes, that would break topics in flight that add new calls to
write_locked_index(); they want to write out the index even when
active_cache_changed says there is no update.

Looking at a typical pre/post image of this change like this place:

>   hold_locked_index(, LOCK_DIE_ON_ERROR);
>   refresh_cache(REFRESH_QUIET);
> - if (active_cache_changed &&
> - write_locked_index(_index, , COMMIT_LOCK))
> + if (write_locked_index(_index, ,
> +COMMIT_LOCK | SKIP_IF_UNCHANGED))
>   return error(_("Unable to write index."));
> - rollback_lock_file();

this certainly simplifies what the caller needs to do.

> @@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
>   * With `COMMIT_LOCK`, the lock is always committed or rolled back.
>   * Without it, the lock is closed, but neither committed nor rolled
>   * back.
> + *
> + * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
> + * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
>   */
>  extern int write_locked_index(struct index_state *, struct lock_file *lock, 
> unsigned flags);

Good.

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jonathan Nieder]

Randall S. Becker wrote:

> The original thread below has details of what the original issue was and
> why. It hit three tests specifically on this platform where die was invoked
> (at least on one of them). Perl was invoked under the covers and the
> completion code of 169 propagated back through git to the test framework.
> https://public-inbox.org/git/499fb29f-ca34-8d28-256d-896107c29...@kdbg.org/T
> /#m0b30f0857feae2044f38e04dc6b8565b68b7d52b

That is:

test_must_fail git send-email --dump-aliases --to=jan...@example.com -1 
refs/heads/accounting

Which matches the case described elsewhere in this thread.  It's
git-send-email.perl.  test_must_fail is doing its job correctly by
diagnosing a real bug in git-send-email.perl, which we're grateful for
you having discovered. :)

Thanks,
Jonathan

[PATCH v2] commit: run git gc --auto just before the pre-commit hook

[Ævar Arnfjörð Bjarmason]

Change the behavior of git-commit back to what it was back in
d4bb43ee27 ("Invoke "git gc --auto" from commit, merge, am and
rebase.", 2007-09-05) when it was git-commit.sh.

Shortly afterwards in f5bbc3225c ("Port git commit to C.", 2007-11-08)
when it was ported to C, the "git gc --auto" invocation went away.

Before this, git gc --auto only ran for
git-{am,merge,fetch,receive-pack}. Therefore it was possible to write
a script that would "git commit" a lot of data locally, and gc would
never run.

One such repository that was locally committing generated zone file
changes had grown to a size of ~60GB before a daily cronjob was added
to "git gc", bringing it down to less than 1GB. This will make such
cases work without intervention.

I think fixing such pathological cases where the repository will grow
forever is a worthwhile trade-off for spending a couple of
milliseconds calling "git gc --auto" (in the common cases where it
doesn't do anything).

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

On Wed, Feb 28 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Change the behavior of git-commit back to what it was back in
>> d4bb43ee27 ("Invoke "git gc --auto" from commit, merge, am and
>> rebase.", 2007-09-05) when it was git-commit.sh.
>
> ... which was to run it just before post-commit.  Do I retitle this
> patch before queuing?

Do'h. Of course I screw up something simple like that, sorry. This v2
fixes it, and I also rephrased the commit message a bit (more commas &
full-stops).

> So this is a decade late regression fix?  As they say, better late
> than never, probably.

Yup.

I wonder if it would also be a good idea to run git gc --auto on "git
push". It itself won't create any objects, but it would be a nice
proxy in many cases for picking up anything else we missed due to
various object writing commands that won't run --auto.

 builtin/commit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index e8e8d13be4..a16a056f6a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1402,6 +1402,7 @@ int run_commit_hook(int editor_is_used, const char 
*index_file, const char *name
 
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
+   const char *argv_gc_auto[] = {"gc", "--auto", NULL};
static struct wt_status s;
static struct option builtin_commit_options[] = {
OPT__QUIET(, N_("suppress summary after successful 
commit")),
@@ -1607,6 +1608,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 "not exceeded, and then \"git reset HEAD\" to recover."));
 
rerere(0);
+   run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
if (amend && !no_post_rewrite) {
commit_post_rewrite(current_head, );
-- 
2.15.1.424.g9478a66081

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

Le 28/02/2018 à 23:32, Junio C Hamano a écrit :
> Eric Sunshine  writes:
> 
>> On Wed, Feb 28, 2018 at 5:17 PM, Alban Gruin  wrote:
>>> Yes, but I can split the line like that:
>>>
>>> % cat >baz.go<<\EOF
>>> package baz
>>> func baz(arg1 int64,
>>> arg2 int64) {
>>> }
>>> EOF
>>> % go build baz.go
>>>
>>> This complies to the standard formatting (at least, gofmt doesn't change
>>> it), but making the regex strict about the brace would cause it to
>>> ignore those funcs, although I don't know how common they are.
>>
>> Makes sense. Thanks for the clarifying example. I wouldn't be at all
>> surprised it such formatting exists in the wild, so keeping the regex
>> as-is seems a good idea.
> 
> Does input like that appear in the tests the patch adds?  If not, it
> probably is a good idea to have it somewhere in the commit (even if
> there is no test addition, having it in the log message to explain
> why the regex is done like so would be a good idea).
> 
> Thanks.
> 

Yes, it's in a file called "golang-long-func".  I'll send another patch
later to fix the typo in the commit message and the indentation in the
tests. I'll clarify the regex in the message, too.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Junio C Hamano]

Eric Sunshine  writes:

> On Wed, Feb 28, 2018 at 5:17 PM, Alban Gruin  wrote:
>> Yes, but I can split the line like that:
>>
>> % cat >baz.go<<\EOF
>> package baz
>> func baz(arg1 int64,
>> arg2 int64) {
>> }
>> EOF
>> % go build baz.go
>>
>> This complies to the standard formatting (at least, gofmt doesn't change
>> it), but making the regex strict about the brace would cause it to
>> ignore those funcs, although I don't know how common they are.
>
> Makes sense. Thanks for the clarifying example. I wouldn't be at all
> surprised it such formatting exists in the wild, so keeping the regex
> as-is seems a good idea.

Does input like that appear in the tests the patch adds?  If not, it
probably is a good idea to have it somewhere in the commit (even if
there is no test addition, having it in the log message to explain
why the regex is done like so would be a good idea).

Thanks.

Re: [PATCH] commit: run git gc --auto just before the pre-commit hook

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Change the behavior of git-commit back to what it was back in
> d4bb43ee27 ("Invoke "git gc --auto" from commit, merge, am and
> rebase.", 2007-09-05) when it was git-commit.sh.

... which was to run it just before post-commit.  Do I retitle this
patch before queuing?

The code seems to run it "after" post-commit.  We need to explain
why we chose to run it differently from the old practice, when we
claim we resurrect the old behaviour with this change.

> Shortly afterwards in f5bbc3225c ("Port git commit to C.", 2007-11-08)
> when it was ported to C the "git gc --auto" invocation went away.

So this is a decade late regression fix?  As they say, better late
than never, probably.

> @@ -1608,6 +1609,7 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>  
>   rerere(0);
>   run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
> + run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>   if (amend && !no_post_rewrite) {
>   commit_post_rewrite(current_head, );
>   }

Re: [PATCH v2] hooks/pre-auto-gc-battery: allow gc to run on non-laptops

[Junio C Hamano]

Adam Borowski  writes:

> Desktops and servers tend to have no power sensor, thus on_ac_power returns
> 255 ("unknown").  Thus, let's take any answer other than 1 ("battery") as
> no contraindication to run gc.
>
> If that tool returns "unknown", there's no point in querying other sources
> as it already queried them, and is smarter than us (can handle multiple
> adapters).
>
> Reported by: Xin Li 
> Signed-off-by: Adam Borowski 
> ---
> v2: improved commit message

That makes the patch and the log consistent so that people who know
the area can reason about it ;-)

Will queue.  Thanks.


>  contrib/hooks/pre-auto-gc-battery | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/hooks/pre-auto-gc-battery 
> b/contrib/hooks/pre-auto-gc-battery
> index 6a2cdebdb..7ba78c4df 100755
> --- a/contrib/hooks/pre-auto-gc-battery
> +++ b/contrib/hooks/pre-auto-gc-battery
> @@ -17,7 +17,7 @@
>  # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
>  #hooks/pre-auto-gc
>  
> -if test -x /sbin/on_ac_power && /sbin/on_ac_power
> +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)
>  then
>   exit 0
>  elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Eric Sunshine]

On Wed, Feb 28, 2018 at 5:17 PM, Alban Gruin  wrote:
> Yes, but I can split the line like that:
>
> % cat >baz.go<<\EOF
> package baz
> func baz(arg1 int64,
> arg2 int64) {
> }
> EOF
> % go build baz.go
>
> This complies to the standard formatting (at least, gofmt doesn't change
> it), but making the regex strict about the brace would cause it to
> ignore those funcs, although I don't know how common they are.

Makes sense. Thanks for the clarifying example. I wouldn't be at all
surprised it such formatting exists in the wild, so keeping the regex
as-is seems a good idea.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

Le 28/02/2018 à 23:00, Eric Sunshine a écrit :
> On Wed, Feb 28, 2018 at 4:31 PM, Alban Gruin  wrote:
 diff --git a/userdiff.c b/userdiff.c
 @@ -38,6 +38,15 @@ IPATTERN("fortran",
 +PATTERNS("golang",
 +/* Functions */
 +"^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
>>>
>>> Why is the brace (and possible following whitespace) optional?
>>> Considering that the language demands that the brace be on the same
>>> line, I'd think the brace should be mandatory.
>>
>> I did this to support non-standard formatting. It's a niche case though,
>> maybe we could only support the standard formatting and modify the doc
>> to reflect this change.
> 
> As noted, unlike 'struct' and 'interface', the brace for a 'func'
> _must_ appear on the same line; that's a requirement of the language.
> Placing it on a line is not an option.
> 
> % cat >foo.go<<\EOF
> package foo
> func foo() {
> }
> EOF
> % go build foo.go
> 
> Versus:
> 
> % cat >bar.go<<\EOF
> package bar
> func bar()
> {
> }
> EOF
> % go build bar.go
> ./bar.go:2:6: missing function body
> ./bar.go:3:1: syntax error: unexpected semicolon or newline before {
> 
> So, the regex probably ought to be strict about expecting the brace on
> the same line as 'func'.
> 

Yes, but I can split the line like that:

% cat >baz.go<<\EOF
package baz
func baz(arg1 int64,
arg2 int64) {
}
EOF
% go build baz.go

This complies to the standard formatting (at least, gofmt doesn't change
it), but making the regex strict about the brace would cause it to
ignore those funcs, although I don't know how common they are.

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Igor Djordjevic]

On 28/02/2018 21:25, Igor Djordjevic wrote:
> 
> But U1' and U2' are really to be expected to stay the same in 
> non-interactive rebase case only...

Just to rephrase to "could be expected" here, meaning still not 
necessarily in this case, either - I`ve just witnessed 
non-interactive rebase Johannes previously described[1], merge with 
`-s ours` (dropping B* patches), plus B1 cherry-picked between 
X1..X2, eventually coming up with different U1' and U2', too (which 
would produce a wrong end result, if continued).

But I guess this should go to the "complex history" pile, good thing 
still being that diff safety check between U1' and U2' works as 
expected, thus automatic merge rebase can be aborted and command 
given back to the user for closer inspection.

[1] 
https://public-inbox.org/git/nycvar.qro.7.76.6.1802272330290...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/

[PATCH] commit: run git gc --auto just before the pre-commit hook

[Ævar Arnfjörð Bjarmason]

Change the behavior of git-commit back to what it was back in
d4bb43ee27 ("Invoke "git gc --auto" from commit, merge, am and
rebase.", 2007-09-05) when it was git-commit.sh.

Shortly afterwards in f5bbc3225c ("Port git commit to C.", 2007-11-08)
when it was ported to C the "git gc --auto" invocation went away.

Before this git gc --auto only ran for
git-{am,merge,fetch,receive-pack}, therefore it was possible to write
a script that would "git commit" a lot of data locally, and gc would
never run.

One such repository that was locally committing generated zone file
changes had grown to a size of ~60GB before a daily cronjob was added
to "git gc", bringing it down to less than 1GB. This will make such
cases work transparently.

I think fixing such pathological cases where the repository will grow
forever is a worthwhile trade-off for spending a couple of
milliseconds calling "git gc --auto" (in the common cases where it
doesn't do anything).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/commit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index e8e8d13be4..b671367840 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1402,6 +1402,7 @@ int run_commit_hook(int editor_is_used, const char 
*index_file, const char *name
 
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
+   const char *argv_gc_auto[] = {"gc", "--auto", NULL};
static struct wt_status s;
static struct option builtin_commit_options[] = {
OPT__QUIET(, N_("suppress summary after successful 
commit")),
@@ -1608,6 +1609,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 
rerere(0);
run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
+   run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
if (amend && !no_post_rewrite) {
commit_post_rewrite(current_head, );
}
-- 
2.15.1.424.g9478a66081

[PATCH v2] hooks/pre-auto-gc-battery: allow gc to run on non-laptops

[Adam Borowski]

Desktops and servers tend to have no power sensor, thus on_ac_power returns
255 ("unknown").  Thus, let's take any answer other than 1 ("battery") as
no contraindication to run gc.

If that tool returns "unknown", there's no point in querying other sources
as it already queried them, and is smarter than us (can handle multiple
adapters).

Reported by: Xin Li 
Signed-off-by: Adam Borowski 
---
v2: improved commit message

 contrib/hooks/pre-auto-gc-battery | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/hooks/pre-auto-gc-battery 
b/contrib/hooks/pre-auto-gc-battery
index 6a2cdebdb..7ba78c4df 100755
--- a/contrib/hooks/pre-auto-gc-battery
+++ b/contrib/hooks/pre-auto-gc-battery
@@ -17,7 +17,7 @@
 # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
 #  hooks/pre-auto-gc
 
-if test -x /sbin/on_ac_power && /sbin/on_ac_power
+if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)
 then
exit 0
 elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1
-- 
2.16.2

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 3:04 PM, Jonathan Nieder wrote:
> On February 28, 2018 1:52 PM, Jonathan Nieder wrote:
> > Randall S. Becker wrote:
> > > On February 28, 2018 12:44 PM, Jonathan Nieder wrote:
> > >> Randall S. Becker wrote:
> >
> > >>> The problem is actually in git code in its test suite that uses
> > >>> perl inline, not in my test code itself.
> > [...]
> > >> Can you elaborate with an example?  My understanding was that
> > >> test_must_fail is only for running git.
> > [...]
> > > Have a look at a recent t1404 as a sample. Line 615 is the one
> > > causing the platform grief, because it triggers a 'die'. However,
> > > the particular test case #54, had no difference on platform with
> > > test_must_fail or !, which has the same underlying EBADF completion
> after
> > digging and digging.
> >
> > Sorry to be dense: what I see on that line is
> >
> > test_must_fail git update-ref -d $prefix/foo >out 2>err &&
> 
> My bad, I think. I'm going to go looking through my notes and get back on
> which line in the test was the issue. I assumed from your response that it
> might have been the test_must_fail, which is throughout the git test
suite.
> Obviously it isn't the line failing in this case. Stay tuned.

The original thread below has details of what the original issue was and
why. It hit three tests specifically on this platform where die was invoked
(at least on one of them). Perl was invoked under the covers and the
completion code of 169 propagated back through git to the test framework.
https://public-inbox.org/git/499fb29f-ca34-8d28-256d-896107c29...@kdbg.org/T
/#m0b30f0857feae2044f38e04dc6b8565b68b7d52b

While removing test_must_fail is laudable, it won't seemingly solve the
underlying cause that I am trying to work through, which is inserting the
$SIGdie reporting 169 on platform and inserting:

 SIG{__DIE__} = sub {
   CORE::die @_ if $^S || !defined($^S);
   print STDERR "fatal: @_";
   exit 128;
 };

I just don't know the framework well enough (or perl for that matter) to
know the exact spot to place this so that it would work properly and be
acceptable to the committers (you know who you are :-) ). 

I hope that provides info on what is going on and why I am motivated to fix
this by (nearly) whatever means necessary.

Cheers,
Randall

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

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Eric Sunshine]

On Wed, Feb 28, 2018 at 4:31 PM, Alban Gruin  wrote:
>>> diff --git a/userdiff.c b/userdiff.c
>>> @@ -38,6 +38,15 @@ IPATTERN("fortran",
>>> +PATTERNS("golang",
>>> +/* Functions */
>>> +"^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
>>
>> Why is the brace (and possible following whitespace) optional?
>> Considering that the language demands that the brace be on the same
>> line, I'd think the brace should be mandatory.
>
> I did this to support non-standard formatting. It's a niche case though,
> maybe we could only support the standard formatting and modify the doc
> to reflect this change.

As noted, unlike 'struct' and 'interface', the brace for a 'func'
_must_ appear on the same line; that's a requirement of the language.
Placing it on a line is not an option.

% cat >foo.go<<\EOF
package foo
func foo() {
}
EOF
% go build foo.go

Versus:

% cat >bar.go<<\EOF
package bar
func bar()
{
}
EOF
% go build bar.go
./bar.go:2:6: missing function body
./bar.go:3:1: syntax error: unexpected semicolon or newline before {

So, the regex probably ought to be strict about expecting the brace on
the same line as 'func'.

Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops

[Junio C Hamano]

Adam Borowski  writes:

> 0 usually means a laptop on AC power, 255 is for a typical desktop.
> The current code can't return 2 or any other unexpected value, but if it
> ever does, an unknown error should probably be treated same as 255 unknown.
> Thus, gc should be avoided only if the return code is 1.

In short, your answer to my question is "What the code does is the
more correct version between the two---the log message was lying."

Then please do not talk about 255 but explain why "only if it is 1"
is the right thing in the log message.  That would make the result
consistent.

> As for the second paragraph,...

That paragraph reads just fine.

Re: Obsolete instruction in SubmittingPatches?

[Junio C Hamano]

Andrei Rybak  writes:

> Is this part of guidelines obsolete, or am I not understanding this
> correctly?

I am merely being nice (but only on "time-permitting" basis).

Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops

[Adam Borowski]

On Wed, Feb 28, 2018 at 10:16:21AM -0800, Junio C Hamano wrote:
> Adam Borowski  writes:
> 
> > Desktops and servers tend to have no power sensor, thus on_ac_power returns
> > 255 ("unknown").
> >
> > If that tool returns "unknown", there's no point in querying other sources
> > as it already queried them, and is smarter than us (can handle multiple
> > adapters).
> 
> The explanation talks about the exit status 255 being special and
> serves to signal "there is no point continuing, and it is OK to
> assume we are not on batttery", while the code says that anything
> but exit status 1 can be treated as such.  Which is correct?

As the man page says:

# EXIT STATUS
#   0 (true)  System is on mains power
#   1 (false) System is not on mains power
#   255 (false)Power status could not be determined

0 usually means a laptop on AC power, 255 is for a typical desktop.
The current code can't return 2 or any other unexpected value, but if it
ever does, an unknown error should probably be treated same as 255 unknown.
Thus, gc should be avoided only if the return code is 1.

As for the second paragraph, I meant that on_ac_power already queried all
sources this hook knows about (other than /usr/bin/pmset which is OSX
only[1]), thus if the answer is "unknown", continuing to query is redundant.

If that's unclear, do you have some other wording in mind?

Also, it's good to trust on_ac_power, as it'll get updated whenever new
quirks of power management get known: I heard allegations that some boards
say "USB" instead of "Mains", which should count the same for our
purposes[2].  It's not reasonable to update consumers such as git instead of
a single system-provided tool.

One worry is that, if on_ac_power is not installed, other sources known by
this hook likewise assume that unknown means battery.  And for example on
Debian, powermgmt-base (which is where on_ac_power lives) is no longer
installed by default.  This suggests this patch needs to be extended to
cover the other sources as well, but let's discuss this first.  Extra
commits are cheap...

> > Reported by: Xin Li 
> > Signed-off-by: Adam Borowski 
> > ---
> >  contrib/hooks/pre-auto-gc-battery | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/contrib/hooks/pre-auto-gc-battery 
> > b/contrib/hooks/pre-auto-gc-battery
> > index 6a2cdebdb..7ba78c4df 100755
> > --- a/contrib/hooks/pre-auto-gc-battery
> > +++ b/contrib/hooks/pre-auto-gc-battery
> > @@ -17,7 +17,7 @@
> >  # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
> >  #  hooks/pre-auto-gc
> >  
> > -if test -x /sbin/on_ac_power && /sbin/on_ac_power
> > +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)
> >  then
> > exit 0
> >  elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1
> 


[1]. I don't know if there's an implementation of on_ac_power for OSX, but
if there is, it is reasonable to assume it uses or emulates pmset.

[2]. Technically, that's _dc_ not ac power, but as batteries use a different
interface, in the vast majority of cases USB power can be considered
non-rationed.  You can power it from an unplugged laptop or from a
powerbank, but that's no different from "mains" that come from an unplugged
UPS with no or unsupported control link.
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢠⠒⠀⣿⡁ A dumb species has no way to open a tuna can.
⢿⡄⠘⠷⠚⠋⠀ A smart species invents a can opener.
⠈⠳⣄ A master species delegates.

Re: [PATCH v8 3/7] utf8: add function to detect prohibited UTF-16/32 BOM

[Lars Schneider]

> On 27 Feb 2018, at 06:17, Eric Sunshine  wrote:
> 
> On Sun, Feb 25, 2018 at 6:35 AM, Lars Schneider
>  wrote:
>>> On 25 Feb 2018, at 04:41, Eric Sunshine  wrote:
>>> Is this interpretation correct? When I read [1], I interpret it as
>>> saying that no BOM _of any sort_ should be present when the encoding
>>> is declared as one of UTF-16BE, UTF-16LE, UTF-32BE, or UTF-32LE.
>> 
>> Correct!
>> 
>>> This code, on the other hand, only checks for BOMs corresponding
>>> to the declared size (16 or 32 bits).
>> 
>> Hmm. Interesting thought. You are saying that my code won't complain if
>> a document declared as UTF-16LE has a UTF32-LE BOM, correct?
> 
> Well, not specifically that case since UTF-16LE BOM is a subset of UTF32-LE 
> BOM.

Correct - bad example on my part!


> My observation was more general in that [1] seems to say that there
> should be _no_ BOM whatsoever if one of UTF-16BE, UTF-16LE, UTF-32BE,
> or UTF-32LE is declared.

You are saying that a document declared as UTF-16LE must not start 
with feff (UTF-32BE BOM)? I interpreted that situation as a "feff"
in the middle of a file and therefore the BOM should be treated as
ZWNBSP as explained here: http://unicode.org/faq/utf_bom.html#bom6

Plus, if "_no_ BOM whatsoever" is allowed then wouldn't we need to check
for UTF-1, UTF-7, and UTF-8 BOM's too?

I dunno.


>> I would say
>> this is correct behavior in context of this function. This function assumes
>> that the document is proper UTF-16/UTF-16BE/UTF-16LE but it is wrongly
>> declared with respect to its BOM in the .gitattributes. Would this
>> comment make it more clear to you?
>>/*
>> * If a data stream is declared as UTF-16BE or UTF-16LE, then a UTF-16
>> * BOM must not be used [1]. The same applies for the UTF-32 
>> equivalents.
>> * The function returns true if this rule is violated.
>> *
>> * [1] http://unicode.org/faq/utf_bom.html#bom10
>> */
>> I think what you are referring to is a different class of error and
>> would therefore warrant its own checker function. Would you agree?
> 
> I don't understand to what different class of error you refer. The
> FAQ[1] seems pretty clear to me in that if one of those declarations
> is used explicitly, then there should be _no_ BOM, period. It doesn't
> say anything about allowing a BOM for a differently-sized encoding (16
> vs 32).
> 
> If I squint very hard, I _guess_ I can see how you interpret [1] with
> the more narrow meaning of the restriction applying only to a BOM of
> the same size as the declared encoding, though reading it that way
> doesn't come easily to me.

For me it is somewhat the other way around :-)
Since I am not sure what is right, I decided to ask the Internet:
https://stackoverflow.com/questions/49038872/is-a-utf-32be-bom-valid-in-an-utf-16le-declared-data-stream

Let's see if someone has a good answer.

- Lars

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

>> diff --git a/userdiff.c b/userdiff.c
>> @@ -38,6 +38,15 @@ IPATTERN("fortran",
>> +PATTERNS("golang",
>> +/* Functions */
>> +"^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
> 
> Why is the brace (and possible following whitespace) optional?
> Considering that the language demands that the brace be on the same
> line, I'd think the brace should be mandatory.
> 

I did this to support non-standard formatting. It's a niche case though,
maybe we could only support the standard formatting and modify the doc
to reflect this change.

> I suppose you made whitespace after 'func' optional to be able to
> recognize a method (which hasn't been gofmt'd):
> 
> func(x *X) name {
> 
> rather than the more typical:
> 
> func (x *X) name {
> 
> I wonder if it would make sense to tighten the expression to recognize
> functions and methods as distinct cases:
> 
> function: mandatory whitespace following 'func'
> method: optional whitespace but mandatory '(' following 'func'
> 
> Your current expression could accidentally match:
> 
> /*
>   Fish like to have
>   functors for lunch {
>   just like eels}.
> */
> 
> but, even the suggested tighter expression could "accidentally" match
> example code in a comment block anyhow, so I guess it probably doesn't
> matter much in practice.
> 

Same as before, I did this to support non-standard formatting.

Obsolete instruction in SubmittingPatches?

[Andrei Rybak]

Hello,

It seems to me that this part of Documentation/SubmittingPatches is
not actual nowadays:

  After the list reached a consensus that it is a good idea to apply the
  patch, re-send it with "To:" set to the maintainer{1} and "cc:" the
  list{2} for inclusion.

>From what I observe in the mailing list, most patches are sent with
"To:" set to mailing list (or re-sent with increasing version) , as per
previous paragraph in the guidelines [1].  Then, after the topic is
reviewed and the [PATCH v] series receives a thumbs up from a
number of people, the maintainer--Junio C Hamano--replies with an email
containing something along the lines of "This change is in good
shape.  Thanks, will queue.", which makes me think that the re-send
is not actually needed.

Is this part of guidelines obsolete, or am I not understanding this
correctly?

[1] "Send your patch with "To:" set to the mailing list, with "cc:"
listing people who are involved in the area you are touching"

--
Best regards, Andrei Rybak

Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache

[Junio C Hamano]

Jeff King  writes:

> A minor nit, but please use something like:
>
>   if (git_env_bool("GIT_TEST_UNTRACKED_CACHE", 0) && ...
>
> so that:
>
>   GIT_TEST_UNTRACKED_CACHE=false
>
> does what one might expect, and not the opposite.
>
> Two other thoughts:
>
>   - it may be worth memo-izing it with a static variable to avoid
> repeatedly calling the possibly-slow getenv()
>
>   - I agree with the sentiment elsewhere that something like
> GIT_FORCE_UNTRACKED_CACHE is probably a better name
>
> (The idea itself seems sound to me, but it's not really my area).

Somehow this topic has been hanging without getting further
attention for too long.  It's time to wrap it up and moving it
forward.  How about this?

-- >8 --
From: Junio C Hamano 
Date: Wed, 28 Feb 2018 13:21:09 -0800
Subject: [PATCH] untracked cache: use git_env_bool() not getenv() for 
customization

GIT_DISABLE_UNTRACKED_CACHE and GIT_TEST_UNTRACKED_CACHE are only
sensed for their presense by using getenv(); use git_env_bool()
instead so that GIT_DISABLE_UNTRACKED_CACHE=false would work as
naïvely expected.

Also rename GIT_TEST_UNTRACKED_CACHE to GIT_FORCE_UNTRACKED_CACHE
to express what it does more honestly.  Forcing its use may be one
useful thing to do while testing the feature, but testing does not
have to be the only use of the knob.

While at it, avoid repeated calls to git_env_bool() by capturing the
return value from the first call in a static variable.

Signed-off-by: Junio C Hamano 
---
 dir.c | 14 --
 t/t7063-status-untracked-cache.sh |  4 ++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index da93374f0c..d445d77e62 100644
--- a/dir.c
+++ b/dir.c
@@ -2164,8 +2164,13 @@ static struct untracked_cache_dir 
*validate_untracked_cache(struct dir_struct *d
  const struct pathspec 
*pathspec)
 {
struct untracked_cache_dir *root;
+   static int untracked_cache_disabled = -1;
 
-   if (!dir->untracked || getenv("GIT_DISABLE_UNTRACKED_CACHE"))
+   if (!dir->untracked)
+   return NULL;
+   if (untracked_cache_disabled < 0)
+   untracked_cache_disabled = 
git_env_bool("GIT_DISABLE_UNTRACKED_CACHE", 0);
+   if (untracked_cache_disabled)
return NULL;
 
/*
@@ -2287,7 +2292,12 @@ int read_directory(struct dir_struct *dir, struct 
index_state *istate,
}
 
if (dir->untracked) {
+   static int force_untracked_cache = -1;
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
+
+   if (force_untracked_cache < 0)
+   force_untracked_cache =
+   git_env_bool("GIT_FORCE_UNTRACKED_CACHE", 0);
trace_printf_key(_untracked_stats,
 "node creation: %u\n"
 "gitignore invalidation: %u\n"
@@ -2297,7 +2307,7 @@ int read_directory(struct dir_struct *dir, struct 
index_state *istate,
 dir->untracked->gitignore_invalidated,
 dir->untracked->dir_invalidated,
 dir->untracked->dir_opened);
-   if (getenv("GIT_TEST_UNTRACKED_CACHE") &&
+   if (force_untracked_cache &&
dir->untracked == istate->untracked &&
(dir->untracked->dir_opened ||
 dir->untracked->gitignore_invalidated ||
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index 6ef520e823..9cb16ca36d 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -14,8 +14,8 @@ test_description='test untracked cache'
 # See <20160803174522.5571-1-pclo...@gmail.com> if you want to know
 # more.
 
-GIT_TEST_UNTRACKED_CACHE=true
-export GIT_TEST_UNTRACKED_CACHE
+GIT_FORCE_UNTRACKED_CACHE=true
+export GIT_FORCE_UNTRACKED_CACHE
 
 sync_mtime () {
find . -type d -ls >/dev/null
-- 
2.16.2-325-g2fc74f41c5

Re: [PATCH v8 5/7] convert: add 'working-tree-encoding' attribute

[Eric Sunshine]

On Tue, Feb 27, 2018 at 6:16 AM, Lars Schneider
 wrote:
>> On 25 Feb 2018, at 08:15, Eric Sunshine  wrote:
>> On Sat, Feb 24, 2018 at 11:27 AM,   wrote:
>> The above paragraph is giving an example of the scenario described in
>> the paragraph above it. It might, therefore, be clearer to start this
>> paragraph with "For example,...". Also, as an aid to non-Windows
>> developers, it might be helpful to introduce ".proj" files as
>> "Microsoft Visual Studio `*.proj` files...".
>
> Agreed. How about this?
>
>   For example, Microsoft Visual Studio resources files (`*.rc`) or
>   PowerShell script files (`*.ps1`) are sometimes encoded in UTF-16.
>   If you declare `*.ps1` as files as UTF-16 and you add `foo.ps1` with
>   a `working-tree-encoding` enabled Git client, then `foo.ps1` will be
>   stored as UTF-8 internally. A client without `working-tree-encoding`
>   support will checkout `foo.ps1` as UTF-8 encoded file. This will
>   typically cause trouble for the users of this file.

Since "*.proj" is mentioned a number of times in paragraphs below this
one, perhaps just stick with it instead of introducing "*.rc" and
"*.ps1", which don't necessarily add value. Alternately, if you use
.rc and .ps1, then change the remaining .proj references to follow
suit.

>>> diff --git a/convert.c b/convert.c
>>> +   } else if (is_missing_required_utf_bom(enc->name, src, src_len)) {
>>> +   const char *error_msg = _(
>>> +   "BOM is required in '%s' if encoded as %s");
>>> +   const char *advise_msg = _(
>>> +   "The file '%s' is missing a byte order mark (BOM). "
>>> +   "Please use %sBE or %sLE (depending on the byte 
>>> order) "
>>> +   "as working-tree-encoding.");
>>> +   advise(advise_msg, path, enc->name, enc->name);
>>> +   if (conv_flags & CONV_WRITE_OBJECT)
>>> +   die(error_msg, path, enc->name);
>>> +   else
>>> +   error(error_msg, path, enc->name);
>>> +   }
>>
>> For a function which presumably is agnostic about the working-tree
>> encoding (supporting whatever iconv does), it nevertheless has
>> unusually intimate knowledge about UTF BOMs, in general, and the
>> internals of has_prohibited_utf_bom() and
>> is_missing_required_utf_bom(), in particular. It might be cleaner, and
>> would simplify this function, if all this UTF BOM-specific gunk was
>> moved elsewhere and generalized into a "validate conversion" function.
>
> Agreed! How about this?
>
> /*
>  * If we convert to an UTF encoding, then check for common BOM errors.
>  */
> if (!memcmp("UTF-", enc, 4))
> if (has_utf_bom_error(path, enc, src, src_len, die_on_error))
> return 0;

Better. The comment merely repeats the code, which is clear in its
intent, thus doesn't really add value (but that's a minor point).

I'd probably generalize it further to a "validate conversion" function
(which itself would have this UTF-specific knowledge), but that's a
matter of taste and can be done later when/if other types of
validation requirements arise.

Re: [PATCH v2] sha1_name: fix uninitialized memory errors

[Junio C Hamano]

Derrick Stolee  writes:

>> I do not think they are wrong, but aren't the latter two somewhat
>> redundant?  "num" is p->num_objects, and we call (first+1)th element
>> only after we see (first < num - 1), i.e. first+1 < num, and the
>> access to (first-1)th is done only when first > 0.  The first one,
>> i.e. when first points at where we _would_ find it if it existed,
>> can access "first" that could be p->num_objects, so the change there
>> makes sense, though.
>
> Yes. But I'd rather keep the blocks consistent and use the return
> value of nth_packed_object_oid() when possible.

Sure, I do not think anybody minds; I just wanted a sanity check.

Thansk.

Re: [PATCH v2] sha1_name: fix uninitialized memory errors

[Derrick Stolee]

On 2/28/2018 3:50 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24dd..a041d8d24f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
 */
mad->init_len = 0;
if (!match) {
-   nth_packed_object_oid(, p, first);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first))
+   extend_abbrev_len(, mad);
} else if (first < num - 1) {
-   nth_packed_object_oid(, p, first + 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first + 1))
+   extend_abbrev_len(, mad);
}
if (first > 0) {
-   nth_packed_object_oid(, p, first - 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first - 1))
+   extend_abbrev_len(, mad);
}
mad->init_len = mad->cur_len;
  }

I do not think they are wrong, but aren't the latter two somewhat
redundant?  "num" is p->num_objects, and we call (first+1)th element
only after we see (first < num - 1), i.e. first+1 < num, and the
access to (first-1)th is done only when first > 0.  The first one,
i.e. when first points at where we _would_ find it if it existed,
can access "first" that could be p->num_objects, so the change there
makes sense, though.



Yes. But I'd rather keep the blocks consistent and use the return value 
of nth_packed_object_oid() when possible.


Thanks,
-Stolee

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Igor Djordjevic]

Hi Sergey,

On 28/02/2018 07:14, Sergey Organov wrote:
> 
> > > Would additional step as suggested in [1] (using R1 and R2 to "catch" 
> > > interactive rebase additions/amendments/drops, on top of U1' and 
> > > U2'), make more sense (or provide an additional clue, at least)?
> > > 
> > > [1] 
> > > https://public-inbox.org/git/8829c395-fb84-2db0-9288-f7b28fa0d...@gmail.com/
> > 
> > Anyway, from (yet another) ad hoc test, additional step mentioned in 
> > [1] above seems to handle this case, too (merge with `-s ours` 
> > dropping B* patches, plus B1 cherry-picked between X1..X2)
> > 
> > ...
> > 
> > And not just that - it worked with additional interactive rebase 
> > adding, amending and removing commits, on top of all this still 
> > preserving original `-s ours` merge commit evil-merge amendment, too, 
> > all as expected (or so it seems) :P
> 
> Great! I do believe that once we start from some sensible approach, many
> kinds of improvements are possible. I'll definitely need to take close
> look at what you came up with, thanks!
> 
> I'd like to say though that nobody should expect miracles from automated
> rebasing of merges when we get to complex history editing. It will need
> to retreat to manual merge, sooner or later.

I agree, and as I really liked "the feeling" of the original approach 
you described, it felt bad to (presumably) see it failing in what 
doesn`t seem to be neither too complex nor rare situation of dropping 
a commit during an interactive rebase, thus motivation to try to 
improve it, if possible, wasn`t lacking :)
 
Eh, might be I`m just naively ignorant at the moment as well, but I`m 
trying to work my way through it... ;)

Regards, Buga

Re: [PATCH v2] sha1_name: fix uninitialized memory errors

[Junio C Hamano]

Derrick Stolee  writes:

> diff --git a/sha1_name.c b/sha1_name.c
> index 611c7d24dd..a041d8d24f 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git 
> *p,
>*/
>   mad->init_len = 0;
>   if (!match) {
> - nth_packed_object_oid(, p, first);
> - extend_abbrev_len(, mad);
> + if (nth_packed_object_oid(, p, first))
> + extend_abbrev_len(, mad);
>   } else if (first < num - 1) {
> - nth_packed_object_oid(, p, first + 1);
> - extend_abbrev_len(, mad);
> + if (nth_packed_object_oid(, p, first + 1))
> + extend_abbrev_len(, mad);
>   }
>   if (first > 0) {
> - nth_packed_object_oid(, p, first - 1);
> - extend_abbrev_len(, mad);
> + if (nth_packed_object_oid(, p, first - 1))
> + extend_abbrev_len(, mad);
>   }
>   mad->init_len = mad->cur_len;
>  }

I do not think they are wrong, but aren't the latter two somewhat
redundant?  "num" is p->num_objects, and we call (first+1)th element
only after we see (first < num - 1), i.e. first+1 < num, and the
access to (first-1)th is done only when first > 0.  The first one,
i.e. when first points at where we _would_ find it if it existed,
can access "first" that could be p->num_objects, so the change there
makes sense, though.

Re: [PATCH v7 0/7] convert: add support for different encodings

[Lars Schneider]

> On 27 Feb 2018, at 22:25, Jeff King  wrote:
> 
> On Tue, Feb 27, 2018 at 10:05:17PM +0100, Torsten Bögershausen wrote:
> 
> Of the three solutions, I think the relative merits are something like
> this:
> 
>  1. baked-in textconv (my patch)
> 
> - reuses an existing diff feature, so minimal code and not likely to
>   break things
> 
> - requires people to add a .gitattributes entry
> 
> - needs work to make bare-repo .gitattributes work (though I think
>   this would be useful for other features, too)
> 
> - has a run-time cost at each diff to do the conversion
> 
> - may sometimes annoy people when it doesn't kick in (e.g.,
>   emailed patches from format-patch won't have a readable diff)
> 
> - doesn't combine with other custom-diff config (e.g., utf-16
>   storing C code should still use diff=c funcname rules, but
>   wouldn't with my patch)
> 
>  2. auto-detect utf-16 (your patch)
> - Just Works for existing repositories storing utf-16
> 
> - carries some risk of kicking in when people would like it not to
>   (e.g., when they really do want a binary patch that can be
>   applied).
> 
>   I think it would probably be OK if this kicked in only when
>   ALLOW_TEXTCONV is set (the default for porcelain), and --binary
>   is not (i.e., when we would otherwise just say "binary
>   files differ").
> 
> - similar to (1), carries a run-time cost for each diff, and users
>   may sometimes still see binary diffs
> 
>  3. w-t-e (Lars's patch)
> 
> - requires no server-side modifications; the diff is plain vanilla
> 
> - works everywhere you diff, plumbing and porcelain
> 
> - does require people to add a .gitattributes entry
> 
> - run-time cost is per-checkout, not per-diff
> 
> So I can see room for (3) to co-exist alongside the others. Between (1)
> and (2), I think (2) is probably the better direction.

Thanks for the great summary! I agree they could co-exist and people
could use whatever works best for them.

I'll incorporate Eric's feedback and send a w-t-e v9 soonish.

- Lars

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Igor Djordjevic]

Hi Sergey,

On 28/02/2018 06:19, Sergey Organov wrote:
> 
> > > (3) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> > >| \  |
> > >|  M |
> > >| /  |
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> > >
> >
> > Meh, I hope I`m rushing it now, but for example, if we had decided to 
> > drop commit A2 during an interactive rebase (so losing A2' from 
> > diagram above), wouldn`t U2' still introduce those changes back, once 
> > U1' and U2' are merged, being incorrect/unwanted behavior...? :/
> 
> I think the method will handle this nicely.

That`s what I thought as well. And then I made a test. And then the 
test broke... Now, might be the test was flawed in the first place, 
but thinking about it a bit more, it does seem to make sense not to 
handle this case nicely :/

> When you "drop" A2, will A3' change, or stay intact?

When you "drop" commit A2 from rebasing, patch (diff) A3' does stay 
the same - but resulting tree (snapshot) after applying it does not.

By removing commit A2 from your rebase, you`re saying that changes 
introduced in that commit shouldn`t ever happen in the rebased tree, 
so trees/snapshots of all rebased commits coming after dropped A2 
will have these changes missing, in comparison to trees of original 
commits they`re being rebased from.

> If it changes, say, to A3'', the U1' will change to U1'', and the method
> will propagate the change automatically.
> 
> If it A3' doesn't change, then there are no changes to take.

All true, but note what I wrote in the original message - the issue 
of dropping A2 is not U1, but U2, and that one doesn`t change.

In this case, U1' will correctly represent U1 rebased on top of A3' 
(where A2 is now missing), no problem there.

But on the other end, as U2 holds changes between original merge and 
B3 (being A1, A2, A3 and evil-merge M), it will also still hold 
changes introduced by original A2.

Rebasing it onto B3' brings all these changes along, and once merged 
with U1' to produce "rebased" merge it unexpectedly introduces 
supposedly dropped commit A2 changes in their full glory.

Yes, considering this situation a conflict, as originally proposed, 
by simply noticing that U1' and U2' differ, helps this to fail 
loudly without doing the wrong thing.

But U1' and U2' are really to be expected to stay the same in 
non-interactive rebase case only, where it doesn`t help interactive 
rebase case at all if it is to fail most of the time (where U1' and 
U2' don`t have to be, but should be expected to possibly be different).

So while your original proposal currently seems like it could be 
working nicely for non-interactive rebase (and might be some simpler 
interactive ones), now hitting/acknowledging its first real use 
limit, my additional quick attempt[1] just tries to aid pretty 
interesting case of complicated interactive rebase, too, where we 
might be able to do better as well, still using you original proposal 
as a base idea :)

Regards, Buga

[1] https://public-inbox.org/git/8829c395-fb84-2db0-9288-f7b28fa0d...@gmail.com/

Re: [PATCH v3 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:
> Hi,
> 
> Brandon Williams wrote:
> 
> > Teach remote-curl the 'stateless-connect' command which is used to
> > establish a stateless 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.
> 
> Cool!  I better look at the spec for that first.
> 
> *looks at the previous patch*
> 
> Oh, there is no documented spec. :/  I'll muddle through this instead,
> then.

I'll make sure to add one :)

> 
> [...]
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -188,7 +188,10 @@ static struct ref *parse_git_refs(struct discovery 
> > *heads, int for_push)
> > heads->version = discover_version();
> > switch (heads->version) {
> > case protocol_v2:
> > -   die("support for protocol v2 not implemented yet");
> > +   /*
> > +* Do nothing.  Client should run 'stateless-connect' and
> > +* request the refs themselves.
> > +*/
> > break;
> 
> This is the 'list' command, right?  Since we expect the client to run
> 'stateless-connect' instead, can we make it error out?

Yes and no.  Remote-curl will run this when trying to establish a
stateless-connection.  If the response is v2 then this is a capability
list and not refs.  So the capabilities will be dumped to the client and
they will be able to request the refs themselves at a later point.  The
comment here is just misleading, so i'll make sure to fix it.

> 
> [...]
> > @@ -1082,6 +1085,184 @@ static void parse_push(struct strbuf *buf)
> > free(specs);
> >  }
> >  
> > +struct proxy_state {
> > +   char *service_name;
> > +   char *service_url;
> > +   struct curl_slist *headers;
> > +   struct strbuf request_buffer;
> > +   int in;
> > +   int out;
> > +   struct packet_reader reader;
> > +   size_t pos;
> > +   int seen_flush;
> > +};
> 
> Can this have a comment describing what it is/does?  It's not obvious
> to me at first glance.
> 
> It doesn't have to go in a lot of detail since this is an internal
> implementation detail, but something saying e.g. that this represents
> a connection to an HTTP server (that's an example; I'm not saying
> that's what it represents :)) would help.

Always making new code have higher standards than the existing code ;)
Haha, I'll add a simple comment explaining it.

> 
> > +
> > +static void proxy_state_init(struct proxy_state *p, const char 
> > *service_name,
> > +enum protocol_version version)
> [...]
> > +static void proxy_state_clear(struct proxy_state *p)
> 
> Looks sensible.
> 
> [...]
> > +static size_t proxy_in(char *buffer, size_t eltsize,
> > +  size_t nmemb, void *userdata)
> 
> Can this have a comment describing the interface?  (E.g. does it read
> a single pkt_line?  How is the caller expected to use it?  Does it
> satisfy the interface of some callback?)

I'll add a comment that its used as a READFUNCTION callback for curl and
that it tries to copy over a packet-line at a time.

> 
> libcurl's example https://curl.haxx.se/libcurl/c/ftpupload.html just
> calls this read_callback.  Such a name plus a pointer to
> CURLOPT_READFUNCTION should do the trick; bonus points if the comment 
> says what our implementation of the callback does.
> 
> Is this about having peek ability?

No its just that Curl only requests a set about of data at a time so you
need to be able to buffer the data that can't be read yet.

> 
> > +   struct proxy_state *p = userdata;
> > +   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("unexpected EOF when reading 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(buffer, p->request_buffer.buf + p->pos, avail);
> > +   p->pos += avail;
> > +   return avail;
> 
> This is a number of bytes, but CURLOPT_READFUNCTION expects a number
> of items, fread-style.  That is:
> 
>  

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 1:52 PM, Jonathan Nieder wrote:
> Randall S. Becker wrote:
> > On February 28, 2018 12:44 PM, Jonathan Nieder wrote:
> >> Randall S. Becker wrote:
> 
> >>> The problem is actually in git code in its test suite that uses perl
> >>> inline, not in my test code itself.
> [...]
> >> Can you elaborate with an example?  My understanding was that
> >> test_must_fail is only for running git.
> [...]
> > Have a look at a recent t1404 as a sample. Line 615 is the one causing
> > the platform grief, because it triggers a 'die'. However, the
> > particular test case #54, had no difference on platform with
> > test_must_fail or !, which has the same underlying EBADF completion
after
> digging and digging.
> 
> Sorry to be dense: what I see on that line is
> 
>   test_must_fail git update-ref -d $prefix/foo >out 2>err &&

My bad, I think. I'm going to go looking through my notes and get back on
which line in the test was the issue. I assumed from your response that it
might have been the test_must_fail, which is throughout the git test suite.
Obviously it isn't the line failing in this case. Stay tuned.

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

> I'll follow up with a patch to
> address the confusing pattern which Peff mentioned and which fooled me
> when I prepared v1.

Here is such a patch on top of the others. I'm not particularly proud of the
name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.

I think this makes the current users a bit more obvious, and should help future
users get this optimization right.

Martin

-- >8 --
Subject: write_locked_index(): add flag to avoid writing unchanged index

We have several callers like

if (active_cache_changed && write_locked_index(...))
handle_error();
rollback_lock_file(...);

where the final rollback is needed because "!active_cache_changed"
shortcuts the if-expression. There are also a few variants of this,
including some if-else constructs that make it more clear when the
explicit rollback is really needed.

Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and
simplify the callers. The most complicated of the callers is in
builtin/update-index.c, where we need to check with `force_write` to
decide whether we should pass the new flag or not.

A further upshot of this patch is that `active_cache_changed`, which is
defined as `the_index.cache_changed`, now only has a few users left.

We could have made the new flag behave the other way round
("FORCE_WRITE"), but that could break existing users behind their backs.
Let's take the more conservative approach. We can still migrate existing
callers to use our new flag. Later we might even be able to flip the
default, possibly without entirely ignoring the risk to in-flight or
out-of-tree topics.

Suggested-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 cache.h|  4 
 builtin/add.c  |  7 +++
 builtin/commit.c   | 10 +++---
 builtin/merge.c| 15 ++-
 builtin/mv.c   |  4 ++--
 builtin/rm.c   |  7 +++
 builtin/update-index.c | 16 +++-
 merge-recursive.c  |  5 ++---
 read-cache.c   |  6 ++
 rerere.c   |  8 +++-
 sequencer.c| 11 +--
 11 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..905d8eb6cc 100644
--- a/cache.h
+++ b/cache.h
@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK(1 << 0)
+#define SKIP_IF_UNCHANGED  (1 << 1)
 
 /*
  * Write the index while holding an already-taken lock. Close the lock,
@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.
+ *
+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 
diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e28..9e5a80cc6f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char 
*prefix)
unplug_bulk_checkin();
 
 finish:
-   if (active_cache_changed) {
-   if (write_locked_index(_index, _file, COMMIT_LOCK))
-   die(_("Unable to write new index file"));
-   }
+   if (write_locked_index(_index, _file,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("Unable to write new index file"));
 
UNLEAK(pathspec);
UNLEAK(dir);
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..8d9b4081c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
if (active_cache_changed
|| !cache_tree_fully_valid(active_cache_tree))
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (active_cache_changed) {
-   if (write_locked_index(_index, _lock,
-  COMMIT_LOCK))
-   die(_("unable to write new_index file"));
-   } else {
-   rollback_lock_file(_lock);
-   }
+   if (write_locked_index(_index, _lock,
+  COMMIT_LOCK | SKIP_IF_UNCHANGED))
+   die(_("unable to write new_index file"));
commit_style = COMMIT_AS_IS;
ret = get_index_file();
goto out;
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..30c65dfeff 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -651,10 +651,9 @@ static int try_merge_strategy(const 

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Igor Djordjevic]

Hi Sergey,

On 28/02/2018 06:44, Sergey Organov wrote:
> 
> > > Here`s our starting position:
> > >
> > > (0) ---X1---o---o---o---o---o---X2 (master)
> > >|\
> > >| A1---A2---A3
> > >| \
> > >|  M (topic)
> > >| /
> > >\-B1---B2---B3
> > >
> > >
> > > Now, we want to rebase merge commit M from X1 onto X2. First, rebase
> > > merge commit parents as usual:
> > >
> > > (1) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3   | A1'--A2'--A3'
> > >| \  |
> > >|  M |
> > >| /  |
> > >\-B1---B2---B3   \-B1'--B2'--B3'
> > >
> > >
> > > That was commonly understandable part.
> >
> > Good. Let's assume that I want to do this interactively (because let's
> > face it, rebase is boring unless we shake up things a little). And let's
> > assume that A1 is my only change to the README, and that I realized that
> > it was incorrect and I do not want the world to see it, so I drop A1'.
> >
> > Let's see how things go from here:
> >
> > > Now, for "rebasing" the merge commit (keeping possible amendments), we
> > > do some extra work. First, we make two temporary commits on top of old
> > > merge parents, by using exact tree (snapshot) of commit M:
> > >
> > > (2) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'
> > >| \  |
> > >|  M |
> > >| /  |
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'
> >
> > Okay, everything would still be the same except that I still have dropped
> > A1'.
> >
> > > So here, in terms of _snapshots_ (trees, not diffs), U1 = U2 = M.
> > >
> > > Now, we rebase these temporary commits, too:
> > >
> > > (3) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> > >| \  |
> > >|  M |
> > >| /  |
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> >
> > I still want to drop A1 in this rebase, so A1' is still missing.
> >
> > And now it starts to get interesting.
> >
> > The diff between A3 and U1 does not touch the README, of course, as I said
> > that only A1 changed the README.  But the diff between B3 and U2 does
> > change the README, thanks to M containing A1 change.
> >
> > Therefore, the diff between B3' and U2' will also have this change to the
> > README. That change that I wanted to drop.
> >
> > > As a next step, we merge these temporary commits to produce our
> > > "rebased" merged commit M:
> > >
> > > (4) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> > >| \  |  \
> > >|  M |   M'
> > >| /  |  /
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> >
> > And here, thanks to B3'..U2' changing the README, M' will also have that
> > change that I wanted to see dropped.
> >
> > Note that A1' is still dropped in my example.
> >
> > > Finally, we drop temporary commits, and record rebased commits A3' 
> > > and B3' as our "rebased" merge commit parents instead (merge commit 
> > > M' keeps its same tree/snapshot state, just gets parents replaced):
> > >
> > > (5) ---X1---o---o---o---o---o---X2
> > >|\   |\
> > >| A1---A2---A3---U1  | A1'--A2'--A3'
> > >| \  | \
> > >|  M |  M'
> > >| /  | /
> > >\-B1---B2---B3---U2  \-B1'--B2'--B3'
> >
> > Now, thanks to U2' being dropped (and A1' *still* being dropped), the
> > change in the README that is still in M' is really only in M'. No other
> > rebased commit has it. That makes it look as if M' introduced this change
> > in addition to the changes that were merged between the merge parents.
> 
> Except that original proposal suggests to cosider this a conflict and
> to stop for amendment, as U1' and U2' now differ.

Thanks for bringing this one up again - I focused on a mere example 
of how the approach could work, not stressing enough the simplicity 
of recognizing when it really doesn`t, which is an important aspect 
to know as well, indeed, supporting much needed trust that no bad 
things could happen behind our back.

Either work as expected, or fail loudly, yes.

Regards, Buga

I wait for your prompt response.

[SAM AZADA]

Good day,

I am Mr. Sam Azada from Burkina Faso  a Minister confide on me to look
for foreign partner who will assist him to invest the sum of  Fifty
Million  Dollars  ($50,000,000) in your country.

He has investment interest in mining, exotic properties for commercial
resident, development properties, hotels and any other viable
investment opportunities in your country based on your recommendation
will be highly welcomed.

Hence your co -operation is highly needed to actualize this investment project

I wait for your prompt response.

Sincerely yours

Mr Sam Azada.

Re: [PATCH 00/11] Moving global state into the repository object (part 2)

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Wed, Feb 28, 2018 at 9:57 AM, Junio C Hamano  wrote:

>> Wait a minute.  Is that topic ever shown to work well together with
>> other topics in flight and are now ready to be built upon?  I had an
>> impression that it is just starting to get serious reviews.
>
> And I had the impression the serious reviews were done and fine;
> the only issue would be demonstrating its working fine with other
> series, where I was also worrying about conflicts with
> brians series. And to address that, I'd just send series in small sizes.

Some of the patches looked cooked to me and others still do not look
cooked yet.  I marked the former with Reviewed-by.  In general, a few
things can help to make the process easier for me:

 1. Giving a quick reply to a review to say how the comments were
resolved, sometimes even with a resend of that one patch to
illustrate.  That way the conversation can continue and the
individual patch can get to a reviewed state faster, without
having to chase between different rerolls of the entire series.

This also has an effect of making the review process more
collaborative: perhaps after seeing how you address their
comments, a reviewer may have another idea that they suggest via a
patch to squash in, etc.

 2. In a reroll, summarizing the result of previous reviews by
including acks as appropriate and Reviewed-by if a reviewer
granted it.  This helps with reviewing the reroll since it tells
people where to focus their attention.

[...]
> Is there anything that a contributor can help with that eases
> refactoring series in flight?

For helping reviewers, see above.

For helping Junio, what I've seen people occasionally do is to locally
run a "trial merge" against next and pu and see what semantic or
lexical conflicts arise.  In the cover letter you can describe these
and give Junio advice to make applying the patch easier for him.

[...]
> Sorry for the miscommunication, though,

FWIW, even though part 1 doesn't look done to me yet, it looks *close*
to done, and I was happy to see the sneak peek at part 2.

Thanks,
Jonathan

Re: [PATCH 00/11] Moving global state into the repository object (part 2)

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Feb 28, 2018 at 9:59 AM, Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>>
>>> Looking at the full-series diff though, it makes me wonder if we
>>> should keep prepare_packed_git() private (i.e. how we initialize the
>>> object store, packfile included, is a private matter). How about
>>> something like this on top?
>>
>> Yup, that looks cleaner.
>
> I agree that it looks cleaner. So we plan on just putting
> it on top of that series?

We tend to avoid "oops, that was wrong and here is a band aid on
top" for things that are still mushy, so it would be preferrable to
get it fixed inline, especially if there are more changes to the
other parts of the series coming.

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Igor Djordjevic]

Hi Sergey,

On 28/02/2018 06:21, Sergey Organov wrote:
> 
> > > > > (3) ---X1---o---o---o---o---o---X2
> > > > >|\   |\
> > > > >| A1---A2---A3---U1  | A1'--A2'--A3'--U1'
> > > > >| \  |
> > > > >|  M |
> > > > >| /  |
> > > > >\-B1---B2---B3---U2  \-B1'--B2'--B3'--U2'
> > > > >
> > > >
> > > > Meh, I hope I`m rushing it now, but for example, if we had decided to
> > > > drop commit A2 during an interactive rebase (so losing A2' from
> > > > diagram above), wouldn`t U2' still introduce those changes back, once
> > > > U1' and U2' are merged, being incorrect/unwanted behavior...? :/
> > >
> > > In that case, the method won't work well at all, so I think we need a
> > > different approach.
> > >
> >
> > Hmm, still rushing it, but what about adding an additional step, 
> > something like this:
> 
> I think it's unneeded, as it should work fine without it, see another
> reply.

Unfortunately, I have a broken test case saying different - it could 
very well be a flawed test, too, but let`s elaborate in that 
other sub-thread[1], indeed.

Regards, Buga

[1] https://public-inbox.org/git/87606hoflx@javad.com/

Re: [PATCH 00/11] Moving global state into the repository object (part 2)

[Stefan Beller]

On Wed, Feb 28, 2018 at 11:02 AM, Junio C Hamano  wrote:

> OK, so I finally picked up the last round, which wasn't even in my
> private build.  I had the previous round but hadn't convinced myself
> that my conflict resolution with other topics in flight that were
> still mushy was correct, so it was out of 'pu', but at least it was
> in my tree, so that is where my impression came from.
>
> I saw that the way a list-head in a repository is initialized has
> been revamped in v4, which looked sensible.  Will merge it to 'pu'
> so that I can pick up the ignore-env removal from Duy.

Thanks,
Stefan

Re: [PATCH v3 28/35] transport-helper: introduce stateless-connect

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > Introduce the transport-helper capability 'stateless-connect'.  This
> > capability indicates that the transport-helper can be requested to run
> > the 'stateless-connect' command which should attempt to make a
> > stateless connection with a remote end.  Once established, the
> > connection can be used by the git client to communicate with
> > the remote end natively in a stateless-rpc manner as supported by
> > protocol v2.  This means that the client must send everything the server
> > needs in a single request as the client must not assume any
> > state-storing on the part of the server or transport.
> >
> > If a stateless 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(+)
> 
> Please add documentation for this command to
> Documentation/gitremote-helpers.txt.
> 
> That helps reviewers, since it means reviewers can get a sense of what
> the interface is meant to be.  It helps remote helper implementers as
> well: it tells them what they can rely on and what can't rely on in
> this interface.  For the same reason it helpers remote helper callers
> as well.
> 
> Thanks,
> Jonathan

Thanks for reminding me.  I had intended to at some point but had
forgotten to do so.  I'm going to mark this it as experimental and for
internal use only so that we can still tweak the interface if we want
before it becomes stable.

-- 
Brandon Williams

[PATCH v2 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`

[Martin Ågren]

If we return early, or if `active_cache_changed` is false, we forget to
roll back the lockfile.

Signed-off-by: Martin Ågren 
---
 merge-recursive.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cc5fa0a949..002fb82cf1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2198,12 +2198,15 @@ int merge_recursive_generic(struct merge_options *o,
hold_locked_index(, LOCK_DIE_ON_ERROR);
clean = merge_recursive(o, head_commit, next_commit, ca,
result);
-   if (clean < 0)
+   if (clean < 0) {
+   rollback_lock_file();
return clean;
+   }
 
if (active_cache_changed &&
write_locked_index(_index, , COMMIT_LOCK))
return err(o, _("Unable to write index."));
+   rollback_lock_file();
 
return clean ? 0 : 1;
 }
-- 
2.16.2.246.ga4ee8f

[PATCH v2 4/5] merge: always roll back lock in `checkout_fast_forward()`

[Martin Ågren]

This function originated in builtin/merge.c. It was moved to merge.c in
commit db699a8a1f (Move try_merge_command and checkout_fast_forward to
libgit.a, 2012-10-26), but was used from sequencer.c even before that.

If a problem occurs, the function returns without rolling back the
lockfile. Teach it to do so.

Signed-off-by: Martin Ågren 
---
 merge.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/merge.c b/merge.c
index 195b578700..f06a4773d4 100644
--- a/merge.c
+++ b/merge.c
@@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head,
setup_unpack_trees_porcelain(, "merge");
 
trees[nr_trees] = parse_tree_indirect(head);
-   if (!trees[nr_trees++])
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
return -1;
+   }
trees[nr_trees] = parse_tree_indirect(remote);
-   if (!trees[nr_trees++])
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
return -1;
+   }
for (i = 0; i < nr_trees; i++) {
parse_tree(trees[i]);
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
-   if (unpack_trees(nr_trees, t, ))
+   if (unpack_trees(nr_trees, t, )) {
+   rollback_lock_file(_file);
return -1;
+   }
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
-- 
2.16.2.246.ga4ee8f

[PATCH v2 5/5] sequencer: do not roll back lockfile unnecessarily

[Martin Ågren]

If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is
no need to call `rollback_lock_file()` on the lockfile. It doesn't hurt
either, but it does make different callers in this file inconsistent,
which might be confusing.

While at it, remove a trailing '.' from a recurring error message.

Signed-off-by: Martin Ågren 
---
 sequencer.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e6bac4692a..0e6d04e4ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,10 +303,8 @@ static int write_message(const void *buf, size_t len, 
const char *filename,
rollback_lock_file(_file);
return error_errno(_("could not write eol to '%s'"), filename);
}
-   if (commit_lock_file(_file) < 0) {
-   rollback_lock_file(_file);
-   return error(_("failed to finalize '%s'."), filename);
-   }
+   if (commit_lock_file(_file) < 0)
+   return error(_("failed to finalize '%s'"), filename);
 
return 0;
 }
@@ -1585,10 +1583,8 @@ static int save_head(const char *head)
ssize_t written;
 
fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
-   if (fd < 0) {
-   rollback_lock_file(_lock);
+   if (fd < 0)
return error_errno(_("could not lock HEAD"));
-   }
strbuf_addf(, "%s\n", head);
written = write_in_full(fd, buf.buf, buf.len);
strbuf_release();
@@ -1597,10 +1593,8 @@ static int save_head(const char *head)
return error_errno(_("could not write to '%s'"),
   git_path_head_file());
}
-   if (commit_lock_file(_lock) < 0) {
-   rollback_lock_file(_lock);
-   return error(_("failed to finalize '%s'."), 
git_path_head_file());
-   }
+   if (commit_lock_file(_lock) < 0)
+   return error(_("failed to finalize '%s'"), 
git_path_head_file());
return 0;
 }
 
@@ -1724,7 +1718,7 @@ static int save_todo(struct todo_list *todo_list, struct 
replay_opts *opts)
todo_list->buf.len - offset) < 0)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(_lock) < 0)
-   return error(_("failed to finalize '%s'."), todo_path);
+   return error(_("failed to finalize '%s'"), todo_path);
 
if (is_rebase_i(opts)) {
const char *done_path = rebase_path_done();
-- 
2.16.2.246.ga4ee8f

[PATCH v2 2/5] sequencer: always roll back lock in `do_recursive_merge()`

[Martin Ågren]

If we return early, we forget to roll back the lockfile. Do so.

Signed-off-by: Martin Ågren 
---
 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 90807c4559..e6bac4692a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
fputs(o.obuf.buf, stdout);
strbuf_release();
diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
-   if (clean < 0)
+   if (clean < 0) {
+   rollback_lock_file(_lock);
return clean;
+   }
 
if (active_cache_changed &&
write_locked_index(_index, _lock, COMMIT_LOCK))
-- 
2.16.2.246.ga4ee8f

[PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

This is v2 of my series to always release locks. As before, there's a
conflict with pu, where the correct resolution is to take my version of
the conflicting hunk.

The only difference to v1 is in patch 3. I'll follow up with a patch to
address the confusing pattern which Peff mentioned and which fooled me
when I prepared v1.

Martin

Martin Ågren (5):
  sequencer: make lockfiles non-static
  sequencer: always roll back lock in `do_recursive_merge()`
  merge-recursive: always roll back lock in `merge_recursive_generic()`
  merge: always roll back lock in `checkout_fast_forward()`
  sequencer: do not roll back lockfile unnecessarily

 merge-recursive.c |  5 -
 merge.c   | 12 +---
 sequencer.c   | 32 ++--
 3 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.16.2.246.ga4ee8f

[PATCH v2 1/5] sequencer: make lockfiles non-static

[Martin Ågren]

After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can have lockfiles on the stack.

One of these functions fails to always roll back the lock. That will be
fixed in the next commit.

Signed-off-by: Martin Ågren 
---
 sequencer.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4d3f60594c..90807c4559 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -290,7 +290,7 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
 static int write_message(const void *buf, size_t len, const char *filename,
 int append_eol)
 {
-   static struct lock_file msg_file;
+   struct lock_file msg_file = LOCK_INIT;
 
int msg_fd = hold_lock_file_for_update(_file, filename, 0);
if (msg_fd < 0)
@@ -436,7 +436,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
struct tree *result, *next_tree, *base_tree, *head_tree;
int clean;
char **xopt;
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
 
if (hold_locked_index(_lock, LOCK_REPORT_ON_ERROR) < 0)
return -1;
@@ -1183,7 +1183,7 @@ static int prepare_revs(struct replay_opts *opts)
 
 static int read_and_refresh_cache(struct replay_opts *opts)
 {
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
int index_fd = hold_locked_index(_lock, 0);
if (read_index_preload(_index, NULL) < 0) {
rollback_lock_file(_lock);
@@ -1577,7 +1577,7 @@ static int create_seq_dir(void)
 
 static int save_head(const char *head)
 {
-   static struct lock_file head_lock;
+   struct lock_file head_lock = LOCK_INIT;
struct strbuf buf = STRBUF_INIT;
int fd;
ssize_t written;
@@ -1702,7 +1702,7 @@ int sequencer_rollback(struct replay_opts *opts)
 
 static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 {
-   static struct lock_file todo_lock;
+   struct lock_file todo_lock = LOCK_INIT;
const char *todo_path = get_todo_path(opts);
int next = todo_list->current, offset, fd;
 
-- 
2.16.2.246.ga4ee8f

Re: [PATCH 00/11] Moving global state into the repository object (part 2)

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> This applies on top of origin/sb/object-store and is the continuation of
>> that series, adding the repository as a context argument to functions.
>
> Wait a minute.  Is that topic ever shown to work well together with
> other topics in flight and are now ready to be built upon?  I had an
> impression that it is just starting to get serious reviews.  
>
> Sorry, but I am behind ;-)

OK, so I finally picked up the last round, which wasn't even in my
private build.  I had the previous round but hadn't convinced myself
that my conflict resolution with other topics in flight that were
still mushy was correct, so it was out of 'pu', but at least it was
in my tree, so that is where my impression came from.

I saw that the way a list-head in a repository is initialized has
been revamped in v4, which looked sensible.  Will merge it to 'pu'
so that I can pick up the ignore-env removal from Duy.

Re: [PATCH 00/11] Moving global state into the repository object (part 2)

[Stefan Beller]

On Wed, Feb 28, 2018 at 9:59 AM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> Looking at the full-series diff though, it makes me wonder if we
>> should keep prepare_packed_git() private (i.e. how we initialize the
>> object store, packfile included, is a private matter). How about
>> something like this on top?
>
> Yup, that looks cleaner.

I agree that it looks cleaner. So we plan on just putting
it on top of that series?

Thanks for the help on refactoring,
Stefan

Re: [PATCH v3 0/4] Delete ignore_env member in struct repository

[Junio C Hamano]

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

> v3 fixes comment style. Also since Brandon raised a question about
> shared_root, it's obviously not a good name, so I renamed it to
> commondir.
>
> I still keep the delete patch 2/4, but I move the repo_setup_env()
> deletion back to 1/4 so all env logic is in one patch (the
> introduction of new helper functions in 1/4 and deletion in 2/4 are
> still diff noise if 2/4 is completely merged back).

Thanks.  After replacing sb/object-store with the last one posted
and re-tweaking resolution of conflicts with ds/commit-graph that is
in flight, I think I can squeeze this topic in.  Among the three, I
find that this kind of clean-up is the most interesting/urgent thing
we need before we can move forward ;-)



>
> Interdiff:
>
> diff --git a/environment.c b/environment.c
> index 47c6e31559..b2128c1188 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -149,7 +149,8 @@ static char *expand_namespace(const char *raw_namespace)
>   return strbuf_detach(, NULL);
>  }
>  
> -/* Wrapper of getenv() that returns a strdup value. This value is kept
> +/*
> + * Wrapper of getenv() that returns a strdup value. This value is kept
>   * in argv to be freed later.
>   */
>  static const char *getenv_safe(struct argv_array *argv, const char *name)
> @@ -170,7 +171,7 @@ void setup_git_env(const char *git_dir)
>   struct set_gitdir_args args = { NULL };
>   struct argv_array to_free = ARGV_ARRAY_INIT;
>  
> - args.shared_root = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
> + args.commondir = getenv_safe(_free, GIT_COMMON_DIR_ENVIRONMENT);
>   args.object_dir = getenv_safe(_free, DB_ENVIRONMENT);
>   args.graft_file = getenv_safe(_free, GRAFT_ENVIRONMENT);
>   args.index_file = getenv_safe(_free, INDEX_ENVIRONMENT);
> diff --git a/repository.c b/repository.c
> index c555dacad2..4f44384dde 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -27,15 +27,15 @@ static void expand_base_dir(char **out, const char *in,
>  }
>  
>  static void repo_set_commondir(struct repository *repo,
> -const char *shared_root)
> +const char *commondir)
>  {
>   struct strbuf sb = STRBUF_INIT;
>  
>   free(repo->commondir);
>  
> - if (shared_root) {
> + if (commondir) {
>   repo->different_commondir = 1;
> - repo->commondir = xstrdup(shared_root);
> + repo->commondir = xstrdup(commondir);
>   return;
>   }
>  
> @@ -58,7 +58,7 @@ void repo_set_gitdir(struct repository *repo,
>   repo->gitdir = xstrdup(gitfile ? gitfile : root);
>   free(old_gitdir);
>  
> - repo_set_commondir(repo, o->shared_root);
> + repo_set_commondir(repo, o->commondir);
>   expand_base_dir(>objects.objectdir, o->object_dir,
>   repo->commondir, "objects");
>   free(repo->objects.alternate_db);
> diff --git a/repository.h b/repository.h
> index 07e8971428..e05a77a099 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -81,7 +81,7 @@ struct repository {
>  extern struct repository *the_repository;
>  
>  struct set_gitdir_args {
> - const char *shared_root;
> + const char *commondir;
>   const char *object_dir;
>   const char *graft_file;
>   const char *index_file;
>
> Nguyễn Thái Ngọc Duy (4):
>   repository.c: move env-related setup code back to environment.c
>   repository.c: delete dead functions
>   sha1_file.c: move delayed getenv(altdb) back to setup_git_env()
>   repository: delete ignore_env member
>
>  cache.h|  2 +-
>  environment.c  | 31 +--
>  object-store.h |  5 ++-
>  object.c   |  1 +
>  repository.c   | 84 --
>  repository.h   | 21 +++--
>  setup.c|  3 +-
>  sha1_file.c|  6 +---
>  8 files changed, 87 insertions(+), 66 deletions(-)