Re: Intermittent failure of t1700-split-index.sh

[Christian Couder]

On Fri, Jan 27, 2017 at 4:58 AM, Jeff King  wrote:
> On Fri, Jan 27, 2017 at 02:45:15AM +, Ramsay Jones wrote:
>
>> I can't devote any time to looking at this further tonight
>> (it's 2-45am here, I'm off to bed!). Can you reproduce the
>> problem, or is it just me? :)
>
> I can reproduce here with 'pu'. t1700.17 seems to fail reliably with my
> stress script after 5-10 seconds.
>
> It bisects to ff97d10f57c2f99b6d86da8f07c16659979b2780, but take that
> with a moderate grain of salt, as I may have marked a bad commit as
> "good" if it simply got lucky and didn't fail as quickly.

Thanks both for your tests and your report. I will take a look.

Re: [PATCH v3 4/4] urlmatch: allow globbing for the URL host part

[Patrick Steinhardt]

On Thu, Jan 26, 2017 at 12:43:31PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt  writes:
> 
> > The URL matching function computes for two URLs whether they match not.
> > The match is performed by splitting up the URL into different parts and
> > then doing an exact comparison with the to-be-matched URL.
> >
> > The main user of `urlmatch` is the configuration subsystem. It allows to
> > set certain configurations based on the URL which is being connected to
> > via keys like `http..*`. A common use case for this is to set
> > proxies for only some remotes which match the given URL. Unfortunately,
> > having exact matches for all parts of the URL can become quite tedious
> > in some setups. Imagine for example a corporate network where there are
> > dozens or even hundreds of subdomains, which would have to be configured
> > individually.
> >
> > This commit introduces the ability to use globbing in the host-part of
> > the URLs. A user can simply specify a `*` as part of the host name to
> > match all subdomains at this level. For example adding a configuration
> > key `http.https://*.example.com.proxy` will match all subdomains of
> > `https://example.com`.
> 
> This is probably a useful improvement.
> 
> Having said that, when I mentioned "glob", I meant to also support
> something like this:
> 
>   https://www[1-4].ibm.com/

The problem with additional extended syntax like proposed by you
is that we would indeed need an escaping mechanism here. '[]' are
already allowed inside the host part to enable IPv6 hosts of the
form 'https://[2001:0db8:]/', so the syntax is now ambiguous. So
we have to be cautios which characters to enable for globbing
syntax. As of now, I think we can only safely include '*' and '?'
here without escaping mechanisms.

If additional use cases come up we might still extend the syntax
later on to allow for more special syntax.

> And when people read "glob", that is what they expect.
> 
> So calling this "the ability to use globbing" is misleading.
> The last paragraph in the log message above needs a bit of
> tweaking, perhaps like this:
> 
>   Allow users to write an asterisk '*' in place of any 'host'
>   or 'subdomain' label as part of the host name.  For example,
>   "http.https://*.example.com.proxy; sets "http.proxy" for all
>   direct subdomains of "https://example.com;,
>   e.g. "https://foo.example.com;, but not
>   "https://foo.bar.example.com;.
> 
> Fortunately, your update to config.txt, which is facing the end
> users, does not misuse the word and instead is explicit that the
> only thing the matcher does is to match '*' to a single hierarchy.
> It is clear that even http://www*.ibm.com/ is not supported from
> the description, which is good.

I agree that globbing is the wrong word here. I'll swap in
"wildcard" where applicable.

I'll send a version 4 later on. Thanks again for your feedback
and improvements.

Regards
Patrick


signature.asc
Description: PGP signature

Re: vger not relaying some of Junio's messages today?

[Jeff King]

On Fri, Jan 27, 2017 at 03:57:53AM +, Eric Wong wrote:

> I noticed both of these are are missing from my archives
> (which rejects messages unless they come from vger):
> 
> 
> 

I don't have them either, so presumably vger ate them (I usually delete
my cc copies after reading and keep the list ones, and I now have
neither).

> Not sure if there's something up with vger or Junio's setup because
> other messages (even from Junio) are getting through fine.

Sporadic failures, especially on a single topic, imply that something in
the content may triggered the taboo filter (though often that takes out
replies, too, which this doesn't seem to have done).

-Peff

Re: Intermittent failure of t1700-split-index.sh

[Jeff King]

On Fri, Jan 27, 2017 at 02:45:15AM +, Ramsay Jones wrote:

> I can't devote any time to looking at this further tonight
> (it's 2-45am here, I'm off to bed!). Can you reproduce the
> problem, or is it just me? :)

I can reproduce here with 'pu'. t1700.17 seems to fail reliably with my
stress script after 5-10 seconds.

It bisects to ff97d10f57c2f99b6d86da8f07c16659979b2780, but take that
with a moderate grain of salt, as I may have marked a bad commit as
"good" if it simply got lucky and didn't fail as quickly.

-Peff

vger not relaying some of Junio's messages today?

[Eric Wong]

I noticed both of these are are missing from my archives
(which rejects messages unless they come from vger):




https://public-inbox.org/git/20170125234101.n2pzrp77df4zy...@genre.crustytoothpaste.net/#r

I only have them because I was Cc:-ed.

gmane doesn't seem to have them, either:

nntp://news.gmane.org/xmqqefzp1d1x@gitster.mtv.corp.google.com
nntp://news.gmane.org/xmqq1svp7lcs@gitster.mtv.corp.google.com

Not sure if there's something up with vger or Junio's setup because
other messages (even from Junio) are getting through fine.

Intermittent failure of t1700-split-index.sh

[Ramsay Jones]

Hi Christian,

I noticed the intermittent failure of t1700-split-index.sh
(tests #17 and #18) yesterday. It failed in a full test-suite
run, but would not fail when run by hand, until I ran it
like so:

$ cd t
$ for i in `seq 100`; do
> echo "== $i =="
> ./t1700-split-index.sh -i -v || break
> done

... when it failed on the 82nd run!

I only had a brief look in the trash directory, but I could
see that the 'twelve' file did not exist, 'eleven' did exist
and was in the index (well, git ls-files showed it), and that
there were only two '.git/sharedindex.*' files and that their
timestamps had not been changed.

I can't devote any time to looking at this further tonight
(it's 2-45am here, I'm off to bed!). Can you reproduce the
problem, or is it just me? :)

ATB,
Ramsay Jones

Re: [RFC 02/14] upload-pack: allow ref name and glob requests

[Junio C Hamano]

Jonathan Tan  writes:

>> I am not sure if this "at the conclusion of" is sensible.  It is OK
>> to assume that what the client side has is fixed, and it is probably
>> OK to desire that what the server side has can change, but at the
>> same time, it feels quite fragile to move the goalpost in between.
>
> Do you have any specific concerns as to this fragility? Peff mentioned
> some concerns with the client making some decisions based on the
> initial SHA-1 vs the SHA-1 reported by "wanted-ref", to which I
> replied [1].

There were two but I think you are aware of both.  One is what Peff
already mentioned, the client may want to make the decision before
going through the negotiation.  The other is "moving the goalpost",
the history the last server has may violate the view of the history
common between the server and the client that is established during
the negotiation with previous servers.

Private

[info]

Hello, I am Carlos Zulueta Secretary to Dr. Gertjan Vlieghe (Bank Of  
England), we have an inheritance of a deceased client with your  
surname Contact Dr. Gertjan Vlieghe With your: Full Name, Tel Number,  
Age, Occupation and Address through email: gvlie...@yandex.com

--
Correo Corporativo Hospital Universitario del Valle E.S.E
***

"Estamos re-dimensionandonos para crecer!"

**

Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Jonathan Tan]

On 01/26/2017 02:33 PM, Junio C Hamano wrote:

Jonathan Tan  writes:


diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..060ec0300
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+   "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
+   rm one-time-sed
+else
+   "$GIT_EXEC_PATH/git-http-backend"
+fi


CodingGuidelines?


Thanks - done locally and will send out in the next reroll.


+inconsistency() {
+   # Simulate that the server initially reports $2 as the ref
+   # corresponding to $1, and after that, $1 as the ref corresponding to
+   # $1. This corresponds to the real-life situation where the server's
+   # repository appears to change during negotiation, for example, when
+   # different servers in a load-balancing arrangement serve (stateless)
+   # RPCs during a single negotiation.
+   printf "s/%s/%s/" \
+  $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+  $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+  >"$HTTPD_ROOT_PATH/one-time-sed"


I'd prefer for the printf'd result to have a final LF (i.e. not
leaving the resulting one-time-sed with a final incomplete line).
Also, do you really need the pipe to tr-d?  Doesn't the result of
$(command substitution) omit the final LF anyway?

$ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
1 foo 2 bar 3
OK


Done.


diff --git a/upload-pack.c b/upload-pack.c
index b88ed8e26..0678c53d6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -862,9 +862,13 @@ static void receive_needs(struct string_list 
*wanted_ns_refs)
} else if (skip_prefix(line, "want ", ) &&
   !get_sha1_hex(arg, sha1_buf)) {
o = parse_object(sha1_buf);
-   if (!o)
+   if (!o) {
+   packet_write_fmt(1,
+"ERR upload-pack: not our ref 
%s",
+sha1_to_hex(sha1_buf));
die("git upload-pack: not our ref %s",
sha1_to_hex(sha1_buf));
+   }


This somehow looks like a good thing to do even in production.  Am I
mistaken?


Yes, that's true. If this patch set stalls (for whatever reason), I'll 
spin this off into an independent patch.

Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor

[brian m. carlson]

On Thu, Jan 26, 2017 at 07:18:41PM +, Eric Wong wrote:
> > Eric Wong  writes:
> Junio C Hamano  wrote:
> > +  "\n"
> > +"#{target}"
> > +"#{attrs[1]}\n"
> > +  "\n"
> >  end
> 
> You need the '\' at the end of those strings, it's not like C
> since Ruby doesn't require semi-colons to terminate lines.
> In other words, that should be:
> 
>   "\n" \
> "#{target}" \
> "#{attrs[1]}\n" \
>   "\n"
> 

This change is fine with me.

For the record, I don't have a strong opinion one way or the other.
Since this code is related to Asciidoctor and Git has no existing Ruby
style standards, I picked the Asciidoctor house style, which uses
multi-line %().  We could pick [0] as an option, or just argue it out
when someone cares, like here.

[0] https://github.com/bbatsov/ruby-style-guide
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [RFC 02/14] upload-pack: allow ref name and glob requests

[Jonathan Tan]

On 01/26/2017 02:23 PM, Junio C Hamano wrote:

Jonathan Tan  writes:


Currently, while performing packfile negotiation [1], upload-pack allows
clients to specify their desired objects only as SHA-1s. This causes:
(a) vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, upload-pack is
provided by multiple Git servers in a load-balancing arrangement,
and
(b) dependence on the server first publishing a list of refs with
associated objects.

To eliminate (a) and take a step towards eliminating (b), teach
upload-pack to support requests in the form of ref names and globs (in
addition to the existing support for SHA-1s) through a new line of the
form "want-ref " where ref is the full name of a ref, a glob
pattern, or a SHA-1. At the conclusion of negotiation, the server will
write "wanted-ref  " for all requests that have been
specified this way.


I am not sure if this "at the conclusion of" is sensible.  It is OK
to assume that what the client side has is fixed, and it is probably
OK to desire that what the server side has can change, but at the
same time, it feels quite fragile to move the goalpost in between.


Do you have any specific concerns as to this fragility? Peff mentioned 
some concerns with the client making some decisions based on the initial 
SHA-1 vs the SHA-1 reported by "wanted-ref", to which I replied [1].



Stepping back a bit, in an environment that involves multiple server
instances that have inconsistent set of refs, can the negotiation
even be sensibly and safely implemented?  The first server the
client contacts may, in response to a "have", say "I do have that
commit so you do not have to send its ancestors to me.  We found one
cut-off point.  Please do explore other lines of histories."  The
next server that concludes the negotiation exchange may not have
that commit and will be unable to produce a pack that excludes the
objects reachable from that commit---wouldn't that become a problem?


It's true that this patch set wouldn't solve this problem. This problem 
only occurs when there is a commit that the client knows but only a few 
of the servers know (maybe because the client just pushed it to one of 
them). If, for example, the client does not know a commit and only a few 
of the servers know it (for example, because another user just pushed 
it), this patch set does help. The latter scenario seems like it would 
occur relatively commonly.



One way to prevent such a problem from hurting clients may be for
these multiple server instances to coordinate and make sure they
have a shared perception of the common history among them.  Some
pushes may have come to one instance but may not have propagated to
other instances, and such a commit cannot be accepted as usable
"have" if the servers anticipate that the final client request would
go to any of the servers.  Otherwise the multiple server arrangement
would not work safely, methinks.

And if the servers are ensuring the safety using such a mechanism,
they can use the same mechanism to restrain "faster" instances from
sending too fresh state of refs that other instances haven't caught
up to, which would mean they can present a consistent set of refs to
the client in the first place, no?

So I am not sure if the mechanism to request history by refname
instead of the tip commit would help the multi-server environment as
advertised.  It may help solving other problems, though (e.g. like
"somebody pushed to update after the initial advertisement was sent
out" which can happen even in a single server environment).


This patch set would solve the problem you describe (whether in a single 
server environment or the coordination between multiple servers that 
provides "strong consistency"). (Although it may not be an important 
problem to solve, since it is probably OK if the client got a "slow" 
version of the state of the refs.)



To be flexible with respect to client needs, the server does not
indicate an error if a "want-ref" line corresponds to no refs, but
instead relies on the client to ensure that what the user needs has been
fetched. For example, a client could reasonably expand an abbreviated
name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref
refs/tags/foo", among others, and ensure that at least one such ref has
been fetched.


Cute.  This may be one way to implement the DWIM thing within the
constraint of eventually wanting to go to "client speaks first, the
server does not advertise things the client is not interested in"
world.

But at the same time it may end up bloating the set of refs the
client asks instead.  Instead of receiving the advertisement and
then sending one request after picking the matching one from it,
the client needs to send "refs/{heads,tags,whatever}/foo".


That is true, although I think that the client will typically send only 
a few ref names (with or without globs), so the 

Re: [RFC 00/14] Allow fetch-pack to send ref names (globs allowed)

[Jonathan Tan]

Thanks for your comments.

On 01/26/2017 03:00 PM, Jeff King wrote:

On Wed, Jan 25, 2017 at 02:02:53PM -0800, Jonathan Tan wrote:


Negotiation currently happens by upload-pack initially sending a list of
refs with names and SHA-1 hashes, and then several request/response
pairs in which the request from fetch-pack consists of SHA-1 hashes
(selected from the initial list). Allowing the request to consist of
names instead of SHA-1 hashes increases tolerance to refs changing
(due to time, and due to having load-balanced servers without strong
consistency),


Interesting. My big question is: what happens when a ref _does_ change?
How does the client handle this?

The existing uploadpack.allowReachableSHA1InWant is there to work around
the problem that an http client may get a ref advertisement in one step,
and then come back later to do the want/have negotiation, at which point
the server has moved on (or maybe it's even a different server). There
the client says "I want sha1 X", and the server needs to say "well, X
isn't my tip now, but it's still acceptable for you to fetch".

But this seems to go in the opposite direction. After the advertisement,
the client decides "OK, I want to fetch refs/heads/master which is at
SHA1 X". It connects to the server and says "I want refs/heads/master".
Let's say the server has moved its version of the ref to SHA1 Y.

What happens? I think the server will say "wanted-ref master Y". Does
the client just decide to use "Y" then?  How does that interact with any
decisions the client might have made about X? I guess things like
fast-forwards have to be decided after we fetch the objects anyway
(since we cannot compute them until we get the pack), so maybe there
aren't any such decisions. I haven't checked.


Yes, the server will say "wanted-ref master Y". The relevant code 
regarding the decisions the client makes regarding X or Y is in do_fetch 
in builtin/fetch.c.


There, I can see these decisions done using X:
 - check_not_current_branch (forbidding fetching that modifies the
   current branch) (I just noticed that this has to be done for Y too,
   and will do so)
 - prune refs [*]
 - automatic tag following [*]

[*] X and Y may differ in that one relevant ref appears in one set but 
not in the other (because a ref was added or removed in the meantime), 
causing a different result if these decisions were to be done using Y, 
but I think that it is OK either way.


Fetch optimizations (for example, everything_local in fetch-pack.c) that 
check if the client really needs to fetch are also done using X, of 
course (and if the optimization succeeds, there is no Y).


Fast-forwards (and everything else in store_updated_refs) are decided 
using Y.



and is a step towards eliminating the need for the server
to send the list of refs first (possibly improving performance).


I'm not sure it is all that useful towards that end. You still have to
break compatibility so that the client tells the server to suppress the
ref advertisement. After that, it is just a question of asking for the
refs. And you have two options:

  1. Ask the server to tell you the values of some subset of the refs,
 pick what you want, and then do the want/have as normal.

  2. Go straight to the want/have, but tell the server the refs you want
 instead of their sha1s.

I think your approach here would lead to (2).

But (1), besides being closer to how the protocol works now, seems like
it's more flexible. I can ask about the ref state without necessarily
having to retrieve the objects. How would you write git-ls-remote with
such a system?


Assuming a new protocol with the appropriate backwards compatibility 
(which would have to be done for both options), (2) does save a 
request/response pair (because we can send the ref names directly as 
"want-ref" in the initial request instead of sending ref names in the 
initial request and then confirming them using "want " in the 
subsequent request). Also, (2) is more tolerant towards refs changing 
over time. (1) might be closer to the current protocol, but I think that 
the difference is not so significant (only in "want-ref" vs "want" 
request and the "wanted-ref" response).


As for git-ls-remote, I currently think that it would have to use the 
existing protocol.



[1] There has been some discussion about whether the server should
accept partial ref names, e.g. [2]. In this patch set, I have made the
server only accept full names, and it is the responsibility of the
client to send the multiple patterns which it wants to match. Quoting
from the commit message of the second patch:

  For example, a client could reasonably expand an abbreviated
  name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref
  refs/tags/foo", among others, and ensure that at least one such ref has
  been fetched.


That has a cost that scales linearly with the number of refs, because
you have to ask for each name 6 times.  After the discussion you linked,
I think my 

Re: [PATCH v2 3/3] update-ref: add test cases for bare repository

[Junio C Hamano]

cornelius.w...@tngtech.com writes:

> From: Cornelius Weig 
>
> The default behavior of update-ref to create reflogs differs in
> repositories with worktree and bare ones. The existing tests cover only
> the behavior of repositories with worktree.
>
> This commit adds tests that assert the correct behavior in bare
> repositories for update-ref. Two cases are covered:
>
>  - If core.logAllRefUpdates is not set, no reflogs should be created
>  - If core.logAllRefUpdates is true, reflogs should be created
>
> Signed-off-by: Cornelius Weig 
> ---
>  t/t1400-update-ref.sh | 43 ---
>  1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index b9084ca..bad88c8 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -8,23 +8,33 @@ test_description='Test git update-ref and basic ref logging'
>  
>  Z=$_z40
>  
> -test_expect_success setup '
> +m=refs/heads/master
> +n_dir=refs/heads/gu
> +n=$n_dir/fixes
> +outside=refs/foo
> +bare=bare-repo
>  
> +create_test_objects ()
> +{
> + local T, sha1, prfx="$1"

CodingGuidelines.  Do not use bash-ism "local" (besides, I do not
think you want to have comma here).

>   for name in A B C D E F
>   do
>   test_tick &&
>   T=$(git write-tree) &&
>   sha1=$(echo $name | git commit-tree $T) &&
> - eval $name=$sha1
> + eval $prfx$name=$sha1
>   done
> +}

Re: [PATCH v2 2/3] refs: add option core.logAllRefUpdates = always

[Junio C Hamano]

cornelius.w...@tngtech.com writes:

> From: Cornelius Weig 
>
> When core.logallrefupdates is true, we only create a new reflog for refs
> that are under certain well-known hierarchies. The reason is that we
> know that some hierarchies (like refs/tags) do not typically change, and

s/do not typically/are not meant to/;

> that unknown hierarchies might not want reflogs at all (e.g., a
> hypothetical refs/foo might be meant to change often and drop old
> history immediately).
>
> However, sometimes it is useful to override this decision and simply log
> for all refs, because the safety and audit trail is more important than
> the performance implications of keeping the log around.
>
> This patch introduces a new "always" mode for the core.logallrefupdates
> option which will log updates to everything under refs/, regardless
> where in the hierarchy it is (we still will not log things like
> ORIG_HEAD and FETCH_HEAD, which are known to be transient).

OK.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 3cd8030..2117616 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -522,6 +522,8 @@ core.logAllRefUpdates::
>   refs/heads/), remote refs (i.e. under refs/remotes/),
>   `refs/heads/`), remote refs (i.e. under `refs/remotes/`),

Ahh, the answer to my question on 1/3 is "no, the commit that the
patch was taken out of was already wrong, still having the old line
in front of its rewrite".

>   note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
> + If it is set to `always`, then a missing reflog is automatically
> + created for any ref under `refs/`.
>  +

OK.

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 5055a96..2ac25a9 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -150,7 +150,8 @@ This option is only applicable when listing tags without 
> annotation lines.
>   'strip' removes both whitespace and commentary.
>  
>  --create-reflog::
> - Create a reflog for the tag.
> + Create a reflog for the tag. To globally enable reflogs for tags, see
> + `core.logAllRefUpdates` in linkgit:git-config[1].

OK.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfe685c..1db0b44 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -612,7 +612,7 @@ static void update_refs_for_switch(const struct 
> checkout_opts *opts,
>   const char *old_desc, *reflog_msg;
>   if (opts->new_branch) {
>   if (opts->new_orphan_branch) {
> - if (opts->new_branch_log && !log_all_ref_updates) {
> + if (opts->new_branch_log && 
> should_autocreate_reflog("refs/heads/")) {

This is inviting a maintenance nightmare.  The helper function is
defined to take the final refname, not a leading directory name.
That is why you named the parameter "refname" in your patch like
this:

--- a/refs.h
+++ b/refs.h
@@ -64,6 +64,8 @@ int read_ref(const char *refname, unsigned char *sha1);

 int ref_exists(const char *refname);

+int should_autocreate_reflog(const char *refname);
+
 int is_branch(const char *refname);

The callers are not supposed to know that its current implementation
happens to only use the leading prefix.  When the definition of this
helper function is changed (e.g. imagine a future where this
"log.allrefupdate" is further enhanced to take glob patterns to
match the refname against), this may break and nobody would notice
for a few weeks, and we will get a regression report after a release
is made.

Don't we have the refname for the branch already in this codepath?

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index d4fb977..b9084ca 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -93,6 +93,42 @@ test_expect_success 'update-ref creates reflogs with 
> --create-reflog' '
>   git reflog exists $outside
>  '
>  
> +test_expect_success 'core.logAllRefUpdates=true does not create reflog by 
> default' '
> + test_config core.logAllRefUpdates true &&
> + test_when_finished "git update-ref -d $outside" &&
> + git update-ref $outside $A &&
> + git rev-parse $A >expect &&
> + git rev-parse $outside >actual &&
> + test_cmp expect actual &&
> + test_must_fail git reflog exists $outside
> +'
> +
> +test_expect_success 'core.logAllRefUpdates=always creates reflog by default' 
> '
> + test_config core.logAllRefUpdates always &&
> + test_when_finished "git update-ref -d $outside" &&
> + git update-ref $outside $A &&
> + git rev-parse $A >expect &&
> + git rev-parse $outside >actual &&
> + test_cmp expect actual &&
> + git reflog exists $outside
> +'

You might want to add two tests for your original motivation, i.e.

test_config core.logAllRefUpdates always &&
git tag a-tag &&
git reflog exists refs/tags/a-tag

and the other one 

Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Junio C Hamano]

Jonathan Tan  writes:

> diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
> new file mode 100644
> index 0..060ec0300
> --- /dev/null
> +++ b/t/lib-httpd/one-time-sed.sh
> @@ -0,0 +1,8 @@
> +#!/bin/sh
> +
> +if [ -e one-time-sed ]; then
> + "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
> + rm one-time-sed
> +else
> + "$GIT_EXEC_PATH/git-http-backend"
> +fi

CodingGuidelines?

> +inconsistency() {
> + # Simulate that the server initially reports $2 as the ref
> + # corresponding to $1, and after that, $1 as the ref corresponding to
> + # $1. This corresponds to the real-life situation where the server's
> + # repository appears to change during negotiation, for example, when
> + # different servers in a load-balancing arrangement serve (stateless)
> + # RPCs during a single negotiation.
> + printf "s/%s/%s/" \
> +$(git -C "$REPO" rev-parse $1 | tr -d "\n") \
> +$(git -C "$REPO" rev-parse $2 | tr -d "\n") \
> +>"$HTTPD_ROOT_PATH/one-time-sed"

I'd prefer for the printf'd result to have a final LF (i.e. not
leaving the resulting one-time-sed with a final incomplete line).
Also, do you really need the pipe to tr-d?  Doesn't the result of 
$(command substitution) omit the final LF anyway?

$ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
1 foo 2 bar 3
OK

> diff --git a/upload-pack.c b/upload-pack.c
> index b88ed8e26..0678c53d6 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -862,9 +862,13 @@ static void receive_needs(struct string_list 
> *wanted_ns_refs)
>   } else if (skip_prefix(line, "want ", ) &&
>  !get_sha1_hex(arg, sha1_buf)) {
>   o = parse_object(sha1_buf);
> - if (!o)
> + if (!o) {
> + packet_write_fmt(1,
> +  "ERR upload-pack: not our ref 
> %s",
> +  sha1_to_hex(sha1_buf));
>   die("git upload-pack: not our ref %s",
>   sha1_to_hex(sha1_buf));
> + }

This somehow looks like a good thing to do even in production.  Am I
mistaken?

Re: [PATCH] git-bisect: allow running in a working tree subdirectory

[Junio C Hamano]

Johannes Sixt  writes:

> Am 26.01.2017 um 19:30 schrieb marcandre.lur...@redhat.com:
>> From: Marc-André Lureau 
>>
>> It looks like it can do it.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  git-bisect.sh | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index ae3cb013e..b0bd604d4 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -1,5 +1,6 @@
>>  #!/bin/sh
>>
>> +SUBDIRECTORY_OK=Yes
>>  
>> USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
>>  LONG_USAGE='git bisect help
>>  print this long help message.
>>
>
> Does it also work to drive git bisect from a subdirectory and pass a
> file name (or pathspec) that is relative to that subdirectory rather
> than relative to the root of the worktree? Can `git bisect good` or
> `git bisect bad` of later bisection steps be invoked from different
> subdirectories or the root?

I think the answers are no and no.  Entries in BISECT_NAMES and
BISECT_LOG are not getting any prefix.

Re: [PATCH v2 1/3] config: add markup to core.logAllRefUpdates doc

[Junio C Hamano]

cornelius.w...@tngtech.com writes:

> From: Cornelius Weig 
>
> Signed-off-by: Cornelius Weig 
> ---
>
> Notes:
> As suggested, I moved the modification of the markup to its own commit.
>
>  Documentation/config.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index af2ae4c..3cd8030 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -517,10 +517,11 @@ core.logAllRefUpdates::
>   "`$GIT_DIR/logs/`", by appending the new and old
>   SHA-1, the date/time and the reason of the update, but
>   only when the file exists.  If this configuration
> - variable is set to true, missing "`$GIT_DIR/logs/`"
> + variable is set to `true`, missing "`$GIT_DIR/logs/`"
>   file is automatically created for branch heads (i.e. under
>   refs/heads/), remote refs (i.e. under refs/remotes/),
> - note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
> + `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
> + note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.

This is a peculiar patch.  

Did you hand edit and lose a leading '-' from one of the lines by
accident, or something?

What's cooking in git.git (Jan 2017, #05; Thu, 26)

[Junio C Hamano]

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

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

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

--
[New Topics]

* js/mingw-hooks-with-exe-suffix (2017-01-26) 1 commit
 - mingw: allow hooks to be .exe files

 Names of the various hook scripts must be spelled exactly, but on
 Windows, an .exe binary must be named with .exe suffix; notice
 $GIT_DIR/hooks/.exe as a valid  hook.

 Will merge to 'next'.


* js/retire-relink (2017-01-25) 2 commits
 - relink: really remove the command
 - relink: retire the command

 Cruft removal.

 Will merge to 'next'.


* js/status-pre-rebase-i (2017-01-26) 1 commit
 - status: be prepared for not-yet-started interactive rebase

 After starting "git rebase -i", which first opens the user's editor
 to edit the series of patches to apply, but before saving the
 contents of that file, "git status" failed to show the current
 state (i.e. you are in an interactive rebase session, but you have
 applied no steps yet) correctly.

 Will merge to 'next'.


* ps/urlmatch-wildcard (2017-01-26) 5 commits
 - SQUASH???
 - urlmatch: allow globbing for the URL host part
 - urlmatch: split host and port fields in `struct url_info`
 - urlmatch: enable normalization of URLs with globs
 - mailmap: add Patrick Steinhardt's work address

 The  part in "http.." configuration variable
 can now be spelled with '*' that serves as wildcard.
 E.g. "http.https://*.example.com.proxy; can be used to specify the
 proxy used for https://a.example.com, https://b.example.com, etc.,
 i.e. any host in the example.com domain.


* rs/absolute-pathdup (2017-01-26) 2 commits
 - use absolute_pathdup()
 - abspath: add absolute_pathdup()

 Code cleanup.

 Will merge to 'next'.


* sb/submodule-recursive-absorb (2017-01-26) 3 commits
 - submodule absorbing: fix worktree/gitdir pointers recursively for non-moves
 - cache.h: expose the dying procedure for reading gitlinks
 - setup: add gentle version of resolve_git_dir

 When a submodule "sub", which has another submodule "module" nested
 within it, is "absorbed" into the top-level superproject, the inner
 submodule "module" is left in a strange state.

 Will merge to 'next'.


* sb/submodule-update-initial-runs-custom-script (2017-01-26) 1 commit
 - submodule update: run custom update script for initial populating as well

 The user can specify a custom update method that is run when
 "submodule update" updates an already checked out submodule.  This
 was ignored when checking the submodule out for the first time and
 we instead always just checked out the commit that is bound to the
 path in the superproject's index.

 Will merge to 'next'.


* sf/putty-w-args (2017-01-26) 3 commits
 - connect: support GIT_SSH_VARIANT and ssh.variant
 - connect: rename tortoiseplink and putty variables
 - connect: handle putty/plink also in GIT_SSH_COMMAND

 The command line options for ssh invocation needs to be tweaked for
 some implementations of SSH (e.g. PuTTY plink wants "-P "
 while OpenSSH wants "-p " to specify port to connect to), and
 the variant was guessed when GIT_SSH environment variable is used
 to specify it.  Extend the guess to the command specified by the
 newer GIT_SSH_COMMAND and also core.sshcommand configuration
 variable, and give an escape hatch for users to deal with
 misdetected cases.

 Expecting a reroll of the last step.

--
[Stalled]

* jc/diff-b-m (2015-02-23) 5 commits
 . WIPWIP
 . WIP: diff-b-m
 - diffcore-rename: allow easier debugging
 - diffcore-rename.c: add locate_rename_src()
 - diffcore-break: allow debugging

 "git diff -B -M" produced incorrect patch when the postimage of a
 completely rewritten file is similar to the preimage of a removed
 file; such a resulting file must not be expressed as a rename from
 other place.

 The fix in this patch is broken, unfortunately.

 Will discard.

--
[Cooking]

* sh/grep-tree-obj-tweak-output (2017-01-20) 2 commits
 - grep: use '/' delimiter for paths
 - grep: only add delimiter if there isn't one already

 "git grep", when fed a tree-ish as an input, shows each hit
 prefixed with ":::".  As  is
 almost always either a commit or a tag that points at a commit, the
 early part of the output ":" can be used as the
 name of the blob and given to "git show".  When  is a
 tree given in the extended SHA-1 syntax (e.g. ":", or
 ":"), however, this results in a string that does not
 name a blob (e.g. "::" or "::").
 "git grep" has been taught to be a bit more intelligent about these
 cases and omit a colon (in the former case) or use slash (in the

Re: [RFC 00/14] Allow fetch-pack to send ref names (globs allowed)

[Jeff King]

On Wed, Jan 25, 2017 at 02:02:53PM -0800, Jonathan Tan wrote:

> Negotiation currently happens by upload-pack initially sending a list of
> refs with names and SHA-1 hashes, and then several request/response
> pairs in which the request from fetch-pack consists of SHA-1 hashes
> (selected from the initial list). Allowing the request to consist of
> names instead of SHA-1 hashes increases tolerance to refs changing
> (due to time, and due to having load-balanced servers without strong
> consistency),

Interesting. My big question is: what happens when a ref _does_ change?
How does the client handle this?

The existing uploadpack.allowReachableSHA1InWant is there to work around
the problem that an http client may get a ref advertisement in one step,
and then come back later to do the want/have negotiation, at which point
the server has moved on (or maybe it's even a different server). There
the client says "I want sha1 X", and the server needs to say "well, X
isn't my tip now, but it's still acceptable for you to fetch".

But this seems to go in the opposite direction. After the advertisement,
the client decides "OK, I want to fetch refs/heads/master which is at
SHA1 X". It connects to the server and says "I want refs/heads/master".
Let's say the server has moved its version of the ref to SHA1 Y.

What happens? I think the server will say "wanted-ref master Y". Does
the client just decide to use "Y" then?  How does that interact with any
decisions the client might have made about X? I guess things like
fast-forwards have to be decided after we fetch the objects anyway
(since we cannot compute them until we get the pack), so maybe there
aren't any such decisions. I haven't checked.

> and is a step towards eliminating the need for the server
> to send the list of refs first (possibly improving performance).

I'm not sure it is all that useful towards that end. You still have to
break compatibility so that the client tells the server to suppress the
ref advertisement. After that, it is just a question of asking for the
refs. And you have two options:

  1. Ask the server to tell you the values of some subset of the refs,
 pick what you want, and then do the want/have as normal.

  2. Go straight to the want/have, but tell the server the refs you want
 instead of their sha1s.

I think your approach here would lead to (2).

But (1), besides being closer to how the protocol works now, seems like
it's more flexible. I can ask about the ref state without necessarily
having to retrieve the objects. How would you write git-ls-remote with
such a system?

> [1] There has been some discussion about whether the server should
> accept partial ref names, e.g. [2]. In this patch set, I have made the
> server only accept full names, and it is the responsibility of the
> client to send the multiple patterns which it wants to match. Quoting
> from the commit message of the second patch:
> 
>   For example, a client could reasonably expand an abbreviated
>   name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref
>   refs/tags/foo", among others, and ensure that at least one such ref has
>   been fetched.

That has a cost that scales linearly with the number of refs, because
you have to ask for each name 6 times.  After the discussion you linked,
I think my preference is more like:

  1. Teach servers to accept a list of patterns from the client
 which will be resolved in order. Unlike your system, the client
 only needs to specify the list once per session, rather than once
 per ref.

  2. (Optional) Give a shorthand for the stock patterns that git has had
 in place for years. That saves some bytes over specifying the
 patterns completely (though it's really not _that_ many bytes, so
 perhaps the complication isn't a big deal).

-Peff

[PATCH v2 2/3] refs: add option core.logAllRefUpdates = always

[cornelius . weig]

From: Cornelius Weig 

When core.logallrefupdates is true, we only create a new reflog for refs
that are under certain well-known hierarchies. The reason is that we
know that some hierarchies (like refs/tags) do not typically change, and
that unknown hierarchies might not want reflogs at all (e.g., a
hypothetical refs/foo might be meant to change often and drop old
history immediately).

However, sometimes it is useful to override this decision and simply log
for all refs, because the safety and audit trail is more important than
the performance implications of keeping the log around.

This patch introduces a new "always" mode for the core.logallrefupdates
option which will log updates to everything under refs/, regardless
where in the hierarchy it is (we still will not log things like
ORIG_HEAD and FETCH_HEAD, which are known to be transient).

Based-on-patch-by: Jeff King 
Signed-off-by: Cornelius Weig 
Reviewed-by: Jeff King 
---

Notes:
Changes with respect to the previous version:

 - add test that checks that no reflog is created for ORIG_HEAD if
   core.logAllRefUpdates=always
 - remove redundant tests that check reflog creation when update-ref is 
called
   with --stdin
 - make test description shorter
 - make the function should_autocreate_reflog() public and use it in
   update_refs_for_switch().

The last item addresses Peff's concern that the previous version only works 
by
accident and may break in the future (see
20170126033547.7bszipvkpi2jb...@sigill.intra.peff.net). In particular, this
concerns the following change:

- if (opts->new_branch_log && !log_all_ref_updates) {
+ if (opts->new_branch_log && should_autocreate_reflog("refs/heads/")) {

The function call to `should_autocreate_reflog()` answers exactly the 
question
that the original test `!log_all_ref_updates` tried to resolve in the 
original
version.

 Documentation/config.txt  |  2 ++
 Documentation/git-tag.txt |  3 ++-
 branch.c  |  2 +-
 builtin/checkout.c|  2 +-
 builtin/init-db.c |  2 +-
 cache.h   |  9 -
 config.c  |  7 ++-
 environment.c |  2 +-
 refs.c| 15 ++-
 refs.h|  2 ++
 refs/files-backend.c  |  6 +++---
 refs/refs-internal.h  |  2 --
 t/t1400-update-ref.sh | 37 +
 13 files changed, 74 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3cd8030..2117616 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -522,6 +522,8 @@ core.logAllRefUpdates::
refs/heads/), remote refs (i.e. under refs/remotes/),
`refs/heads/`), remote refs (i.e. under `refs/remotes/`),
note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
+   If it is set to `always`, then a missing reflog is automatically
+   created for any ref under `refs/`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5055a96..2ac25a9 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -150,7 +150,8 @@ This option is only applicable when listing tags without 
annotation lines.
'strip' removes both whitespace and commentary.
 
 --create-reflog::
-   Create a reflog for the tag.
+   Create a reflog for the tag. To globally enable reflogs for tags, see
+   `core.logAllRefUpdates` in linkgit:git-config[1].
 
 ::
The name of the tag to create, delete, or describe.
diff --git a/branch.c b/branch.c
index c431cbf..b955d4f 100644
--- a/branch.c
+++ b/branch.c
@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,
 start_name);
 
if (reflog)
-   log_all_ref_updates = 1;
+   log_all_ref_updates = LOG_REFS_NORMAL;
 
if (!dont_change_ref) {
struct ref_transaction *transaction;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfe685c..1db0b44 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -612,7 +612,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
const char *old_desc, *reflog_msg;
if (opts->new_branch) {
if (opts->new_orphan_branch) {
-   if (opts->new_branch_log && !log_all_ref_updates) {
+   if (opts->new_branch_log && 
should_autocreate_reflog("refs/heads/")) {
int ret;
char *refname;
struct strbuf err = STRBUF_INIT;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 76d68fa..1d4d6a0 100644
--- a/builtin/init-db.c
+++ 

[PATCH v2 1/3] config: add markup to core.logAllRefUpdates doc

[cornelius . weig]

From: Cornelius Weig 

Signed-off-by: Cornelius Weig 
---

Notes:
As suggested, I moved the modification of the markup to its own commit.

 Documentation/config.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..3cd8030 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -517,10 +517,11 @@ core.logAllRefUpdates::
"`$GIT_DIR/logs/`", by appending the new and old
SHA-1, the date/time and the reason of the update, but
only when the file exists.  If this configuration
-   variable is set to true, missing "`$GIT_DIR/logs/`"
+   variable is set to `true`, missing "`$GIT_DIR/logs/`"
file is automatically created for branch heads (i.e. under
refs/heads/), remote refs (i.e. under refs/remotes/),
-   note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+   `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+   note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
-- 
2.10.2

[PATCH v2 3/3] update-ref: add test cases for bare repository

[cornelius . weig]

From: Cornelius Weig 

The default behavior of update-ref to create reflogs differs in
repositories with worktree and bare ones. The existing tests cover only
the behavior of repositories with worktree.

This commit adds tests that assert the correct behavior in bare
repositories for update-ref. Two cases are covered:

 - If core.logAllRefUpdates is not set, no reflogs should be created
 - If core.logAllRefUpdates is true, reflogs should be created

Signed-off-by: Cornelius Weig 
---
 t/t1400-update-ref.sh | 43 ---
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b9084ca..bad88c8 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -8,23 +8,33 @@ test_description='Test git update-ref and basic ref logging'
 
 Z=$_z40
 
-test_expect_success setup '
+m=refs/heads/master
+n_dir=refs/heads/gu
+n=$n_dir/fixes
+outside=refs/foo
+bare=bare-repo
 
+create_test_objects ()
+{
+   local T, sha1, prfx="$1"
for name in A B C D E F
do
test_tick &&
T=$(git write-tree) &&
sha1=$(echo $name | git commit-tree $T) &&
-   eval $name=$sha1
+   eval $prfx$name=$sha1
done
+}
 
+test_expect_success setup '
+   create_test_objects "" &&
+   mkdir $bare &&
+   cd $bare &&
+   git init --bare &&
+   create_test_objects "bare" &&
+   cd -
 '
 
-m=refs/heads/master
-n_dir=refs/heads/gu
-n=$n_dir/fixes
-outside=refs/foo
-
 test_expect_success \
"create $m" \
"git update-ref $m $A &&
@@ -93,6 +103,25 @@ test_expect_success 'update-ref creates reflogs with 
--create-reflog' '
git reflog exists $outside
 '
 
+test_expect_success 'creates no reflog in bare repository' '
+   git -C $bare update-ref $m $bareA &&
+   git -C $bare rev-parse $bareA >expect &&
+   git -C $bare rev-parse $m >actual &&
+   test_cmp expect actual &&
+   test_must_fail git -C $bare reflog exists $m
+'
+
+test_expect_success 'core.logAllRefUpdates=true creates reflog in bare 
repository' '
+   test_when_finished "git -C $bare config --unset core.logAllRefUpdates 
&& \
+   rm $bare/logs/$m" &&
+   git -C $bare config core.logAllRefUpdates true &&
+   git -C $bare update-ref $m $bareB &&
+   git -C $bare rev-parse $bareB >expect &&
+   git -C $bare rev-parse $m >actual &&
+   test_cmp expect actual &&
+   git -C $bare reflog exists $m
+'
+
 test_expect_success 'core.logAllRefUpdates=true does not create reflog by 
default' '
test_config core.logAllRefUpdates true &&
test_when_finished "git update-ref -d $outside" &&
-- 
2.10.2

Re: [RFC 02/14] upload-pack: allow ref name and glob requests

[Junio C Hamano]

Jonathan Tan  writes:

> Currently, while performing packfile negotiation [1], upload-pack allows
> clients to specify their desired objects only as SHA-1s. This causes:
> (a) vulnerability to failure when an object turns non-existent during
> negotiation, which may happen if, for example, upload-pack is
> provided by multiple Git servers in a load-balancing arrangement,
> and
> (b) dependence on the server first publishing a list of refs with
> associated objects.
>
> To eliminate (a) and take a step towards eliminating (b), teach
> upload-pack to support requests in the form of ref names and globs (in
> addition to the existing support for SHA-1s) through a new line of the
> form "want-ref " where ref is the full name of a ref, a glob
> pattern, or a SHA-1. At the conclusion of negotiation, the server will
> write "wanted-ref  " for all requests that have been
> specified this way.

I am not sure if this "at the conclusion of" is sensible.  It is OK
to assume that what the client side has is fixed, and it is probably
OK to desire that what the server side has can change, but at the
same time, it feels quite fragile to move the goalpost in between.

Stepping back a bit, in an environment that involves multiple server
instances that have inconsistent set of refs, can the negotiation
even be sensibly and safely implemented?  The first server the
client contacts may, in response to a "have", say "I do have that
commit so you do not have to send its ancestors to me.  We found one
cut-off point.  Please do explore other lines of histories."  The
next server that concludes the negotiation exchange may not have
that commit and will be unable to produce a pack that excludes the
objects reachable from that commit---wouldn't that become a problem?

One way to prevent such a problem from hurting clients may be for
these multiple server instances to coordinate and make sure they
have a shared perception of the common history among them.  Some
pushes may have come to one instance but may not have propagated to
other instances, and such a commit cannot be accepted as usable
"have" if the servers anticipate that the final client request would
go to any of the servers.  Otherwise the multiple server arrangement
would not work safely, methinks.

And if the servers are ensuring the safety using such a mechanism,
they can use the same mechanism to restrain "faster" instances from
sending too fresh state of refs that other instances haven't caught
up to, which would mean they can present a consistent set of refs to
the client in the first place, no?

So I am not sure if the mechanism to request history by refname
instead of the tip commit would help the multi-server environment as
advertised.  It may help solving other problems, though (e.g. like
"somebody pushed to update after the initial advertisement was sent
out" which can happen even in a single server environment).

> The server indicates that it supports this feature by advertising the
> capability "ref-in-want". Advertisement of this capability is by default
> disabled, but can be enabled through a configuration option.

OK.

> To be flexible with respect to client needs, the server does not
> indicate an error if a "want-ref" line corresponds to no refs, but
> instead relies on the client to ensure that what the user needs has been
> fetched. For example, a client could reasonably expand an abbreviated
> name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref
> refs/tags/foo", among others, and ensure that at least one such ref has
> been fetched.

Cute.  This may be one way to implement the DWIM thing within the
constraint of eventually wanting to go to "client speaks first, the
server does not advertise things the client is not interested in"
world.  

But at the same time it may end up bloating the set of refs the
client asks instead.  Instead of receiving the advertisement and
then sending one request after picking the matching one from it,
the client needs to send "refs/{heads,tags,whatever}/foo".

Re: [RFC 00/14] Allow fetch-pack to send ref names (globs allowed)

[Stefan Beller]

On Wed, Jan 25, 2017 at 2:02 PM, Jonathan Tan  wrote:
> Hello everyone - this is a proposal for a protocol change to allow the
> fetch-pack/upload-pack to converse in terms of ref names (globs
> allowed), and also an implementation of the server (upload-pack) and
> fetch-from-HTTP client (fetch-pack invoked through fetch).
>
> Negotiation currently happens by upload-pack initially sending a list of
> refs with names and SHA-1 hashes, and then several request/response
> pairs in which the request from fetch-pack consists of SHA-1 hashes
> (selected from the initial list). Allowing the request to consist of
> names instead of SHA-1 hashes increases tolerance to refs changing
> (due to time, and due to having load-balanced servers without strong
> consistency), and is a step towards eliminating the need for the server
> to send the list of refs first (possibly improving performance).
>
> The protocol is extended by allowing fetch-pack to send
> "want-ref ", where  is a full name (refs/...) [1], possibly
> including glob characters. Upload-pack will inform the client of the
> refs actually matched by sending "wanted-ref  " before
> sending the last ACK or NAK.

I have reviewed the patches and think they are a good idea,
cc'ing Jeff who you linked to and who had some ideas about the protocol as well.

Stefan

Re: HEAD's reflog entry for a renamed branch

[Jeff King]

On Thu, Jan 26, 2017 at 01:30:54PM -0800, Junio C Hamano wrote:

> >   - "git branch -m" does seem to realize when we are renaming HEAD,
> > because it updates HEAD to point to the new branch name. But it
> > should probably insert another reflog entry mentioning the rename
> > (we do for "git checkout foo", even when "foo" has the same sha1 as
> > the current HEAD).
> 
> This one I care less (not in the sense that I prefer it not done,
> but in the sense that I do not mind it is left unfixed than the
> other one you pointed out).

I wondered if it might affect how "git checkout -" works. But that
feature looks for reflogs like "checkout: moving from X to Y" to know to
move back to X.  So we are fine here. Even though the HEAD reflog does
not show us going _to_ new-master, we would see it in a later entry as
"from new-master to Y". What we are missing is "rename from master to
new-master", but that entry does not matter. There is no "master" to
go back to anymore. :)

-Peff

Re: [PATCH] git-bisect: allow running in a working tree subdirectory

[Johannes Sixt]

Am 26.01.2017 um 19:30 schrieb marcandre.lur...@redhat.com:

From: Marc-André Lureau 

It looks like it can do it.

Signed-off-by: Marc-André Lureau 
---
 git-bisect.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-bisect.sh b/git-bisect.sh
index ae3cb013e..b0bd604d4 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,5 +1,6 @@
 #!/bin/sh

+SUBDIRECTORY_OK=Yes
 
USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
 LONG_USAGE='git bisect help
print this long help message.



Does it also work to drive git bisect from a subdirectory and pass a 
file name (or pathspec) that is relative to that subdirectory rather 
than relative to the root of the worktree? Can `git bisect good` or `git 
bisect bad` of later bisection steps be invoked from different 
subdirectories or the root?


-- Hannes

Re: HEAD's reflog entry for a renamed branch

[Junio C Hamano]

Jeff King  writes:

> It's unfortunate that there's no message. This is because the rename
> calls delete_ref() under the hood, but that function doesn't take a
> reflog message argument at all. It usually doesn't matter because
> deleting the ref will also delete the reflog.
>
> But as your example shows, deletions _can_ get logged in the HEAD
> reflog; the low-level ref code detects when HEAD points to the updated
> ref and logs an entry there, too.
> ...
> I'd say there are two potential improvements:
>
>   - delete_ref() should take a reflog message argument, in case it
> updates the HEAD reflog (as a bonus, this will future-proof us for a
> day when we might keep reflogs for deleted refs).

This sounds sensible.

>   - "git branch -m" does seem to realize when we are renaming HEAD,
> because it updates HEAD to point to the new branch name. But it
> should probably insert another reflog entry mentioning the rename
> (we do for "git checkout foo", even when "foo" has the same sha1 as
> the current HEAD).

This one I care less (not in the sense that I prefer it not done,
but in the sense that I do not mind it is left unfixed than the
other one you pointed out).

Re: "git fetch -p" incorrectly deletes branches

[Jeff King]

On Tue, Jan 17, 2017 at 07:04:28AM +0100, Reimar Döffinger wrote:

> Deletes refs/heads/test every second time when run repeatedly:
> 
> $ git fetch -p -v origin master:refs/heads/test
> From https://github.com/git/git
>  * [new branch]  master -> test
>  = [up to date]  master -> origin/master
> $ git fetch -p -v origin master:refs/heads/test
> From https://github.com/git/git
>  - [deleted] (none) -> test
>  = [up to date]  master -> test
>  = [up to date]  master -> origin/master

Hmm. It seems like the problem is that "-p" is saying "the other side
does not have refs/heads/test; we must prune it". But I think it is
probably nonsense to apply pruning to a non-wildcard refspec.


> Also note that this behaviour appears also when fetch.prune=yes
> is set in the config (instead of -p on the command-line),
> which makes it much less obvious and there is no option to turn
> of prune just for that command to work-around this.

There is a separate issue of whether it is sane to apply fetch.prune to
a refspec given on the command line; I can imagine it as surprising, to
say the least.

I think "--no-prune" would disable it, though.

-Peff

Re: [PATCH v3 4/4] urlmatch: allow globbing for the URL host part

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> +while (url_len && pat_len) {
>> +const char *url_next = end_of_token(url, '.', url_len);
>> +const char *pat_next = end_of_token(pat, '.', pat_len);
>> + ...
>>  }
>>  
>> +return 1;
>
> Embarrassing.  The last one must be "have they both run out?" i.e.
>
>   return (!url_len && !pat_len);

OK, here is my second try.  The added test piece is to catch the
silly mistake I made above.

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index ec545e0929..33fd59fbb3 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1177,7 +1177,7 @@ test_expect_success 'urlmatch' '
test_cmp expect actual
 '
 
-test_expect_success 'glob-based urlmatch' '
+test_expect_success 'urlmatch with wildcard' '
cat >.git/config <<-\EOF &&
[http]
sslVerify
@@ -1210,6 +1210,10 @@ test_expect_success 'glob-based urlmatch' '
echo http.sslverify false
} >expect &&
git config --get-urlmatch HTTP https://good.example.com >actual &&
+   test_cmp expect actual &&
+
+   echo http.sslverify >expect &&
+   git config --get-urlmatch HTTP https://more.example.com.au >actual &&
test_cmp expect actual
 '
 
diff --git a/urlmatch.c b/urlmatch.c
index 53ff972a60..0e007a3e07 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -63,36 +63,47 @@ static int append_normalized_escapes(struct strbuf *buf,
return 1;
 }
 
+static const char *end_of_token(const char *s, int c, size_t n)
+{
+   const char *next = memchr(s, c, n);
+   if (!next)
+   next = s + n;
+   return next;
+}
+
 static int match_host(const struct url_info *url_info,
  const struct url_info *pattern_info)
 {
-   char *url = xmemdupz(url_info->url + url_info->host_off, 
url_info->host_len);
-   char *pat = xmemdupz(pattern_info->url + pattern_info->host_off, 
pattern_info->host_len);
-   char *url_tok, *pat_tok, *url_save, *pat_save;
-   int matching;
-
-   url_tok = strtok_r(url, ".", _save);
-   pat_tok = strtok_r(pat, ".", _save);
-
-   for (; url_tok && pat_tok; url_tok = strtok_r(NULL, ".", _save),
-  pat_tok = strtok_r(NULL, ".", _save)) {
-   if (!strcmp(pat_tok, "*"))
-   continue; /* a simple glob matches everything */
-
-   if (strcmp(url_tok, pat_tok)) {
-   /* subdomains do not match */
-   matching = 0;
-   break;
-   }
+   const char *url = url_info->url + url_info->host_off;
+   const char *pat = pattern_info->url + pattern_info->host_off;
+   int url_len = url_info->host_len;
+   int pat_len = pattern_info->host_len;
+
+   while (url_len && pat_len) {
+   const char *url_next = end_of_token(url, '.', url_len);
+   const char *pat_next = end_of_token(pat, '.', pat_len);
+
+   if (pat_next == pat + 1 && pat[0] == '*')
+   /* wildcard matches anything */
+   ;
+   else if ((pat_next - pat) == (url_next - url) &&
+!memcmp(url, pat, url_next - url))
+   /* the components are the same */
+   ;
+   else
+   return 0; /* found an unmatch */
+
+   if (url_next < url + url_len)
+   url_next++;
+   url_len -= url_next - url;
+   url = url_next;
+   if (pat_next < pat + pat_len)
+   pat_next++;
+   pat_len -= pat_next - pat;
+   pat = pat_next;
}
 
-   /* matching if both URL and pattern are at their ends */
-   matching = (url_tok == NULL && pat_tok == NULL);
-
-   free(url);
-   free(pat);
-
-   return matching;
+   return (!url_len && !pat_len);
 }
 
 static char *url_normalize_1(const char *url, struct url_info *out_info, char 
allow_globs)

Re: HEAD's reflog entry for a renamed branch

[Jeff King]

On Mon, Jan 16, 2017 at 06:17:29PM -0500, Kyle Meyer wrote:

> I noticed that, after renaming the current branch, the corresponding
> message in .git/logs/HEAD is empty.
> 
> For example, running
> 
> $ mkdir test-repo
> $ cd test-repo
> $ git init
> $ echo abc >file.txt
> $ git add file.txt
> $ git commit -m"Add file.txt"
> $ git branch -m master new-master

Thanks for providing a simple reproduction recipe.

> resulted in the following reflogs:
> 
>$ cat .git/logs/refs/heads/new-master
>0... 68730... Kyle Meyer  1484607020 -0500commit 
> (initial): Add file.txt
>68730... 68730... Kyle Meyer  1484607020 -0500Branch: 
> renamed refs/heads/master to refs/heads/new-master
> 
>$ cat .git/logs/HEAD
>0... 68730... Kyle Meyer  1484607020 -0500commit 
> (initial): Add file.txt
>68730... 0... Kyle Meyer  1484607020 -0500
> 
> I expected the second line of .git/logs/HEAD to mirror the second line
> of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
> in HEAD's entry intentional?

The null sha1 is correct, I think. The branch we were on went away, and
we use the null sha1 to indicate "no value" in both the creation and
deletion cases.

It's unfortunate that there's no message. This is because the rename
calls delete_ref() under the hood, but that function doesn't take a
reflog message argument at all. It usually doesn't matter because
deleting the ref will also delete the reflog.

But as your example shows, deletions _can_ get logged in the HEAD
reflog; the low-level ref code detects when HEAD points to the updated
ref and logs an entry there, too.

You can see the same thing with a simpler example:

  $ git init
  $ git commit -m foo --allow-empty
  $ git update-ref -d refs/heads/master
  $ cat .git/logs/HEAD
  0...  8ffd1... Jeff King  1485464779 -0500 commit 
(initial): foo
  8ffd1...  0... Jeff King  1485464787 -0500

This doesn't come up much because most porcelain commands will refuse to
delete the current branch (notice I had to use "update-ref" and not
"branch -d").

I'd say there are two potential improvements:

  - delete_ref() should take a reflog message argument, in case it
updates the HEAD reflog (as a bonus, this will future-proof us for a
day when we might keep reflogs for deleted refs).

  - "git branch -m" does seem to realize when we are renaming HEAD,
because it updates HEAD to point to the new branch name. But it
should probably insert another reflog entry mentioning the rename
(we do for "git checkout foo", even when "foo" has the same sha1 as
the current HEAD).

-Peff

Re: SubmittingPatches: drop temporal reference for PGP signing

[Philip Oakley]

From: "Junio C Hamano" 

Cornelius Weig  writes:


How about something along these lines? Does the forward reference
break the main line of thought too severly?


I find it a bit distracting for those who know PGP signing has
nothing to do with signing off your patch, but I think that is OK
because they are not the primary target audience of this part of the
document.


Agreed. I this case the target audience was those weren't aware of that.


I however am more worried that it may be misleading to mention these
two in the same sentence.  Those who skim these paragraphs without
knowing the difference between the two may get a false impression
that these two may somehow be related because they are mentioned in
the same sentence.

The retitling of section (5) you did, without any other change,
might be sufficient.  It may also help to be even more explicit in
the updated title, i.e. s/by signing off/by adding Signed-off-by:/


Maybe even s/by signing off/by adding your Signed-off-by:/ to be sure that 
the reader knows that it is _their certification_ that is being sought. Even 
if it does double up on the 'your'.




Thanks.

diff --git a/Documentation/SubmittingPatches 
b/Documentation/SubmittingPatches

index 08352de..c2b0cbe 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -216,12 +216,12 @@ that it will be postponed.
 Exception:  If your mailer is mangling patches then someone may ask
 you to re-send them using MIME, that is OK.

-Do not PGP sign your patch, at least for now.  Most likely, your
-maintainer or other people on the list would not have your PGP
-key and would not bother obtaining it anyway.  Your patch is not
-judged by who you are; a good patch from an unknown origin has a
-far better chance of being accepted than a patch from a known,
-respected origin that is done poorly or does incorrect things.
+Do not PGP sign your patch, but do sign-off your work as explained in 
(5).
+Most likely, your maintainer or other people on the list would not have 
your
+PGP key and would not bother obtaining it anyway. Your patch is not 
judged by
+who you are; a good patch from an unknown origin has a far better chance 
of
+being accepted than a patch from a known, respected origin that is done 
poorly

+or does incorrect things.

 If you really really really really want to do a PGP signed
 patch, format it as "multipart/signed", not a text/plain message
@@ -246,7 +246,7 @@ patch.
  *2* The mailing list: git@vger.kernel.org


-(5) Sign your work
+(5) Certify your work by signing off

 To improve tracking of who did what, we've borrowed the
 "sign-off" procedure from the Linux kernel project on patches

Re: Force Confirmation for Dropping Changed Lines

[Junio C Hamano]

Hilco Wijbenga  writes:

> On 25 January 2017 at 18:32, Junio C Hamano  wrote:
>> I think you should be able to do something like
>>
>> $ cat >$HOME/bin/fail-3way <<\EOF
>> #!/bin/sh
>> git merge-file "$@"
>> exit 1
>> EOF
>> $ chmod +x $HOME/bin/fail-3way
>> $ cat >>$HOME/.gitconfig <<\EOF
>> [merge "fail"]
>> name = always fail 3-way merge
>> driver = $HOME/bin/fail-3way %A %O %B
>> recursive = text
>> EOF
>> $ echo pom.xml merge=fail >>.gitattributes
>>
>> to define a custom merge driver whose name is "fail", that runs the
>> fail-3way program, which runs the bog standard 3-way merge we use
>> (so that it will do the best-effort textual merge) but always return
>> with a non-zero status to signal that the result is conflicting and
>> needs manual resolution.
>
> Thank you, that's an improvement. :-) Unfortunately, it still
> completes the merge. Is there any way to simply get the
>/ markers?

You can, but you need to write one yourself without relying on "git
merge-file", because the whole point of the 3way merge we implement
(including in that program) is "do not bother the user when both
sides made the same change."

Re: [PATCH v3 4/4] urlmatch: allow globbing for the URL host part

[Junio C Hamano]

Junio C Hamano  writes:

> + while (url_len && pat_len) {
> + const char *url_next = end_of_token(url, '.', url_len);
> + const char *pat_next = end_of_token(pat, '.', pat_len);
> + ...
>   }
>  
> + return 1;

Embarrassing.  The last one must be "have they both run out?" i.e.

return (!url_len && !pat_len);

Re: [PATCH v3 4/4] urlmatch: allow globbing for the URL host part

[Junio C Hamano]

Patrick Steinhardt  writes:

> The URL matching function computes for two URLs whether they match not.
> The match is performed by splitting up the URL into different parts and
> then doing an exact comparison with the to-be-matched URL.
>
> The main user of `urlmatch` is the configuration subsystem. It allows to
> set certain configurations based on the URL which is being connected to
> via keys like `http..*`. A common use case for this is to set
> proxies for only some remotes which match the given URL. Unfortunately,
> having exact matches for all parts of the URL can become quite tedious
> in some setups. Imagine for example a corporate network where there are
> dozens or even hundreds of subdomains, which would have to be configured
> individually.
>
> This commit introduces the ability to use globbing in the host-part of
> the URLs. A user can simply specify a `*` as part of the host name to
> match all subdomains at this level. For example adding a configuration
> key `http.https://*.example.com.proxy` will match all subdomains of
> `https://example.com`.

This is probably a useful improvement.

Having said that, when I mentioned "glob", I meant to also support
something like this:

https://www[1-4].ibm.com/

And when people read "glob", that is what they expect.

So calling this "the ability to use globbing" is misleading.
The last paragraph in the log message above needs a bit of
tweaking, perhaps like this:

Allow users to write an asterisk '*' in place of any 'host'
or 'subdomain' label as part of the host name.  For example,
"http.https://*.example.com.proxy; sets "http.proxy" for all
direct subdomains of "https://example.com;,
e.g. "https://foo.example.com;, but not
"https://foo.bar.example.com;.

Fortunately, your update to config.txt, which is facing the end
users, does not misuse the word and instead is explicit that the
only thing the matcher does is to match '*' to a single hierarchy.
It is clear that even http://www*.ibm.com/ is not supported from
the description, which is good.

>  . Host/domain name (e.g., `example.com` in `https://example.com/`).
> -  This field must match exactly between the config key and the URL.
> +  This field must match between the config key and the URL. It is
> +  possible to specify a `*` as part of the host name to match all subdomains
> +  at this level. `https://*.example.com/` for example would match
> +  `https://foo.example.com/`, but not `https://foo.bar.example.com/`.

This is good as-is.

>  . Port number (e.g., `8080` in `http://example.com:8080/`).
>This field must match exactly between the config key and the URL.
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 923bfc5a2..ec545e092 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1177,6 +1177,42 @@ test_expect_success 'urlmatch' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'glob-based urlmatch' '

This is not "glob".  A more generic term "wildcard" is OK.

> + cat >.git/config <<-\EOF &&
> + [http]
> + sslVerify
> ...
> +static int match_host(const struct url_info *url_info,
> +   const struct url_info *pattern_info)
> +{
> + char *url = xmemdupz(url_info->url + url_info->host_off, 
> url_info->host_len);
> + char *pat = xmemdupz(pattern_info->url + pattern_info->host_off, 
> pattern_info->host_len);
> + char *url_tok, *pat_tok, *url_save, *pat_save;
> + int matching;
> +
> + url_tok = strtok_r(url, ".", _save);
> + pat_tok = strtok_r(pat, ".", _save);

Hmph, this will be the first use of strtok_r() in our codebase.
Does everybody have it?

For a use like this where your delimiter set is a singleton, it may
be simpler to do the usual strchrnul() or memchr() based loop.  The
attached is my attempt to do so on top of this patch.

> +
> + for (; url_tok && pat_tok; url_tok = strtok_r(NULL, ".", _save),
> +pat_tok = strtok_r(NULL, ".", _save)) {
> + if (!strcmp(pat_tok, "*"))
> + continue; /* a simple glob matches everything */

s/glob/asterisk/

Other than that, the patch looks OK.



diff --git a/urlmatch.c b/urlmatch.c
index 53ff972a60..8dfc7fd28a 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -63,36 +63,47 @@ static int append_normalized_escapes(struct strbuf *buf,
return 1;
 }
 
+static const char *end_of_token(const char *s, int c, size_t n)
+{
+   const char *next = memchr(s, c, n);
+   if (!next)
+   next = s + n;
+   return next;
+}
+
 static int match_host(const struct url_info *url_info,
  const struct url_info *pattern_info)
 {
-   char *url = xmemdupz(url_info->url + url_info->host_off, 
url_info->host_len);
-   char *pat = xmemdupz(pattern_info->url + pattern_info->host_off, 
pattern_info->host_len);
-   char *url_tok, *pat_tok, *url_save, 

Re: [PATCH] git-bisect: allow running in a working tree subdirectory

[Junio C Hamano]

Stefan Beller  writes:

> + Duy, main author of the worktree feature.
>
> On Thu, Jan 26, 2017 at 10:30 AM,   wrote:
>> From: Marc-André Lureau 
>>
>> It looks like it can do it.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---

I do not think the OP meant by "a working tree subdirectory" using
the command in a secondary worktree.  SUBDIRECTORY_OK is about "can
the command be started in a subdirectory (as opposed to requiring to
be run only at the toplevel)?"

I am slightly negative on this change, though.  The subdirectory you
are sitting in when you start your bisection may disappear and reappear
as you dig the history, and I do not think the code makes anything
special to prevent the disappearing current directory from getting
in the way of bisection process.

>>  git-bisect.sh | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index ae3cb013e..b0bd604d4 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -1,5 +1,6 @@
>>  #!/bin/sh
>>
>> +SUBDIRECTORY_OK=Yes
>>  
>> USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
>>  LONG_USAGE='git bisect help
>> print this long help message.
>> --
>> 2.11.0.295.gd7dffce1c.dirty
>>

Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index af2ae4cc02..f2c210f0a0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1949,6 +1949,13 @@ Environment variable settings always override any 
> matches.  The URLs that are
>  matched against are those given directly to Git commands.  This means any 
> URLs
>  visited as a result of a redirection do not participate in matching.
>  
> +ssh.variant::
> + Override the autodetection of plink/tortoiseplink in the SSH
> + command that 'git fetch' and 'git push' use. It can be set to
> + either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
> + value will be treated as normal ssh. This is useful in case
> + that Git gets this wrong.
> +

I do like the fact that this now sits under ssh.* hierarchy (not core.*).

It should mention core.sshCommand, GIT_SSH_COMMAND, etc., because
reading this alone would mislead users to set only this one and expect
that their plink will be used without setting core.sshCommand etc.

It also should say that GIT_SSH_VARIANT environment variable will
override this variable.

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 4f208fab92..c322558aa7 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options 
> through your
>  personal `.ssh/config` file.  Please consult your ssh documentation
>  for further details.
>  
> +`GIT_SSH_VARIANT`::
> + If this environment variable is set, it overrides the autodetection
> + of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
> + push' use. It can be set to either `ssh`, `plink`, `putty` or
> + `tortoiseplink`. Any other value will be treated as normal ssh. This
> + is useful in case that Git gets this wrong.

Similarly this should mention GIT_SSH_COMMAND at least.  It is crazy
to set something that will cause misdetection to core.sshCommand and
use this environment variable to fix it (instead of using ssh.variant),
so I think it is OK not to mention core.sshCommand (but it would not
hurt to do so).

> diff --git a/connect.c b/connect.c
> index 9f750eacb6..7b4437578b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
>   return NULL;
>  }
>  
> +static int handle_ssh_variant(int *port_option, int *needs_batch)
> +{
> + const char *variant;
> +
> + if (!(variant = getenv("GIT_SSH_VARIANT")) &&
> + git_config_get_string_const("ssh.variant", ))
> + return 0;
> +
> + if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
> + *port_option = 'P';
> + else if (!strcmp(variant, "tortoiseplink")) {
> + *port_option = 'P';
> + *needs_batch = 1;
> + }
> +
> + return 1;
> +}

Between handle and get I do not think there is strong reason to
favor one over the other.  Unlike the one I sent yesterday, this is
not overriding but is getting, so we should keep the original name
Segev gave it, which is shorter, I would think, rather than renaming
it to "handle".

The string that came from the configuration must be freed, no?  That
is what I recall in Peff's comment yesterday.

> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char 
> *url,
>   ssh_argv0 = xstrdup(ssh);
>   }
>  
> - if (ssh_argv0) {
> + if (!handle_ssh_variant(_option, _batch) &&
> + ssh_argv0) {

I like the placement of this "if the user explicitly told us" much
better.

My patch yesterday was deliberately being lazy, because the next
thought that will come to us after comparing the two is "this way,
we avoid having to do the auto-detection altogether, which is way
better than wasting the effort to auto-detect, only to override".

And that reasoning will lead to a realization "there is no reason to
even do the split_cmdline() if the user explicitly told us".  While
that reasoning is correct and we _should_ further refactor, I didn't
want to spend braincycles on that myself.

Re: Force Confirmation for Dropping Changed Lines

[Hilco Wijbenga]

On 25 January 2017 at 18:32, Junio C Hamano  wrote:
> I think you should be able to do something like
>
> $ cat >$HOME/bin/fail-3way <<\EOF
> #!/bin/sh
> git merge-file "$@"
> exit 1
> EOF
> $ chmod +x $HOME/bin/fail-3way
> $ cat >>$HOME/.gitconfig <<\EOF
> [merge "fail"]
> name = always fail 3-way merge
> driver = $HOME/bin/fail-3way %A %O %B
> recursive = text
> EOF
> $ echo pom.xml merge=fail >>.gitattributes
>
> to define a custom merge driver whose name is "fail", that runs the
> fail-3way program, which runs the bog standard 3-way merge we use
> (so that it will do the best-effort textual merge) but always return
> with a non-zero status to signal that the result is conflicting and
> needs manual resolution.

Thank you, that's an improvement. :-) Unfortunately, it still
completes the merge. Is there any way to simply get the
/ markers?

Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor

[Eric Wong]

> Eric Wong  writes:
> > You can use '\' to continue long lines with any Ruby version:
> >
> > "" \
> >   "#{target}" \
> >   "#{attrs[1]}" \
> > ""

Junio C Hamano  wrote:
> +  "\n"
> +"#{target}"
> +"#{attrs[1]}\n"
> +  "\n"
>  end

You need the '\' at the end of those strings, it's not like C
since Ruby doesn't require semi-colons to terminate lines.
In other words, that should be:

  "\n" \
"#{target}" \
"#{attrs[1]}\n" \
  "\n"

Re: [PATCH 2/2] fsck: lazily load types under --connectivity-only

[Jeff King]

On Thu, Jan 26, 2017 at 10:51:00AM -0800, Junio C Hamano wrote:

> >   2. It serves as a cross-check that the coercion in (1a) is
> >  correct (i.e., we'll complain about a parent link that
> >  points to a blob). But we get most of this for free
> >  already, because right after coercing, we'll parse any
> >  non-blob objects. So we'd notice then if we expected a
> >  commit and got a blob.
> >
> >  The one exception is when we expect a blob, in which
> >  case we never actually read the object contents.
> >
> >  So this is a slight weakening, but given that the whole
> >  point of --connectivity-only is to sacrifice some data
> >  integrity checks for speed, this seems like an
> >  acceptable tradeoff.
> 
> The only weakening is that a non-blob (or a corrupt blob) object
> that sits where we expect a blob (because the containing tree or the
> tag says so) would not be diagnosed as an error, then?  I think that
> is in line with the spirit of --connectivity-only and is a good
> trade-off.

Correct. The corrupt-blob case we always knew was a tradeoff (that's the
whole point of --connectivity-only). We could add back in the "we expect
a blob, is it really one?" at the moment we traverse to it, but IMHO
it's not interesting enough to even be worth the sha1_object_info()
lookup time.

-Peff

Re: [PATCH 2/2] ref-filter: add function to parse atoms from a nul-terminated string

[Junio C Hamano]

SZEDER Gábor  writes:

> On Wed, Dec 7, 2016 at 5:09 PM, SZEDER Gábor  wrote:
>> ref-filter's parse_ref_filter_atom() function parses an atom between
>> the start and end pointers it gets as arguments.  This is fine for two
>> of its callers, which process '%(atom)' format specifiers and the end
>> pointer comes directly from strchr() looking for the closing ')'.
>> However, it's not quite so straightforward for its other two callers,
>> which process sort specifiers given as plain nul-terminated strings.
>> Especially not for ref_default_sorting(), which has the default
>> hard-coded as a string literal, but can't use it directly, because a
>> pointer to the end of that string literal is needed as well.
>> The next patch will add yet another caller using a string literal.
>>
>> Add a helper function around parse_ref_filter_atom() to parse an atom
>> up to the terminating nul, and call this helper in those two callers.
>>
>> Signed-off-by: SZEDER Gábor 
>> ---
>>  ref-filter.c | 8 ++--
>>  ref-filter.h | 4 
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> Ping?
>
> It looks like that this little two piece cleanup series fell on the floor.

I am still waiting for somebody else to comment on the changes, and
the final reroll after such a review discussion to address your own
comment in 

Re: [PATCH] rebase: pass --signoff option to git am

[Giuseppe Bilotta]

On Mon, Jan 23, 2017 at 9:03 PM, Giuseppe Bilotta
 wrote:
> On Mon, Jan 23, 2017 at 7:13 PM, Junio C Hamano  wrote:
>>
>> Should we plan to extend this to the interactive backend that is
>> shared between rebase -i and rebase -m, too?  Or is this patch
>> already sufficient to cover them?
>
> AFAIK this is sufficient for both, in the sense that I've used it with
> git rebase -i and it works.

Hm, something very strange is going on, I've just tested the patch on
top of current next and for some reason the signoff line does not get
added. The command-line option gets passed to git am, but I get no
signoff for some reason, so something is failing down the line, I'll
have to investigate.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] mingw: allow hooks to be .exe files

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Peff,
>
> On Wed, 25 Jan 2017, Jeff King wrote:
>
>> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>> 
>> > -  if (access(path.buf, X_OK) < 0)
>> > +  if (access(path.buf, X_OK) < 0) {
>> > +#ifdef STRIP_EXTENSION
>> > +  strbuf_addstr(, ".exe");
>> 
>> I think STRIP_EXTENSION is a string.  Should this line be:
>> 
>>   strbuf_addstr(, STRIP_EXTENSION);
>
> Yep.
>
> v2 coming,
> Johannes

I think I've already tweaked it out when I queued the original one.

Re: [PATCH 2/2] fsck: lazily load types under --connectivity-only

[Junio C Hamano]

Jeff King  writes:

> The recent fixes to "fsck --connectivity-only" load all of
> the objects with their correct types. This keeps the
> connectivity-only code path close to the regular one, but it
> also introduces some unnecessary inefficiency. While getting
> the type of an object is cheap compared to actually opening
> and parsing the object (as the non-connectivity-only case
> would do), it's still not free.
>
> For reachable non-blob objects, we end up having to parse
> them later anyway (to see what they point to), making our
> type lookup here redundant.
>
> For unreachable objects, we might never hit them at all in
> the reachability traversal, making the lookup completely
> wasted. And in some cases, we might have quite a few
> unreachable objects (e.g., when alternates are used for
> shared object storage between repositories, it's normal for
> there to be objects reachable from other repositories but
> not the one running fsck).
>
> The comment in mark_object_for_connectivity() claims two
> benefits to getting the type up front:
>
>   1. We need to know the types during fsck_walk(). (And not
>  explicitly mentioned, but we also need them when
>  printing the types of broken or dangling commits).
>
>  We can address this by lazy-loading the types as
>  necessary. Most objects never need this lazy-load at
>  all, because they fall into one of these categories:
>
>a. Reachable from our tips, and are coerced into the
> correct type as we traverse (e.g., a parent link
> will call lookup_commit(), which converts OBJ_NONE
> to OBJ_COMMIT).
>
>b. Unreachable, but not at the tip of a chunk of
>   unreachable history. We only mention the tips as
> "dangling", so an unreachable commit which links
> to hundreds of other objects needs only report the
> type of the tip commit.
>
>   2. It serves as a cross-check that the coercion in (1a) is
>  correct (i.e., we'll complain about a parent link that
>  points to a blob). But we get most of this for free
>  already, because right after coercing, we'll parse any
>  non-blob objects. So we'd notice then if we expected a
>  commit and got a blob.
>
>  The one exception is when we expect a blob, in which
>  case we never actually read the object contents.
>
>  So this is a slight weakening, but given that the whole
>  point of --connectivity-only is to sacrifice some data
>  integrity checks for speed, this seems like an
>  acceptable tradeoff.

The only weakening is that a non-blob (or a corrupt blob) object
that sits where we expect a blob (because the containing tree or the
tag says so) would not be diagnosed as an error, then?  I think that
is in line with the spirit of --connectivity-only and is a good
trade-off.

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King  wrote:
>> I don't think it means either. It means to include remotes in the
>> selected revisions, but excluding the entries mentioned by --exclude.
>>
>> IOW:
>>
>>   --exclude=foo --remotes
>> include all remotes except refs/remotes/foo
>>
>>   --exclude=foo --unrelated --remotes
>> same
>>
>>   --exclude=foo --decorate-reflog --remotes
>> decorate reflogs of all remotes except "foo". Do _not_ use them
>> as traversal tips.
>>
>>   --decorate-reflog --exclude=foo --remotes
>> same
>>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog?

I would think that "--exclude=foo --something --remotes" 

 * should be diagnosed as an error if "--something" is not compatible
   with "--exclude";

 * should take effect at the concluding "--remotes" if "--something"
   is similar to "--decorate-reflog" whose effect ends at a
   concluding --remotes/--branches/etc.; and

 * should work independently if "--something" is neither.

Re: [PATCH] git-bisect: allow running in a working tree subdirectory

[Stefan Beller]

+ Duy, main author of the worktree feature.

On Thu, Jan 26, 2017 at 10:30 AM,   wrote:
> From: Marc-André Lureau 
>
> It looks like it can do it.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  git-bisect.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index ae3cb013e..b0bd604d4 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -1,5 +1,6 @@
>  #!/bin/sh
>
> +SUBDIRECTORY_OK=Yes
>  
> USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
>  LONG_USAGE='git bisect help
> print this long help message.
> --
> 2.11.0.295.gd7dffce1c.dirty
>

Re: PATCH 1/2] abspath: add absolute_pathdup()

[Brandon Williams]

On 01/26, René Scharfe wrote:
> Add a function that returns a buffer containing the absolute path of its
> argument and a semantic patch for its intended use.  It avoids an extra
> string copy to a static buffer.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  abspath.c| 7 +++
>  cache.h  | 1 +
>  contrib/coccinelle/xstrdup_or_null.cocci | 6 ++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/abspath.c b/abspath.c
> index fce40fddcc..2f0c26e0e2 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -239,6 +239,13 @@ const char *absolute_path(const char *path)
>   return sb.buf;
>  }
>  
> +char *absolute_pathdup(const char *path)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_add_absolute_path(, path);
> + return strbuf_detach(, NULL);
> +}
> +
>  /*
>   * Unlike prefix_path, this should be used if the named file does
>   * not have to interact with index entry; i.e. name of a random file
> diff --git a/cache.h b/cache.h
> index 00a029af36..d7b7a8cd7a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1069,6 +1069,7 @@ const char *real_path(const char *path);
>  const char *real_path_if_valid(const char *path);
>  char *real_pathdup(const char *path);
>  const char *absolute_path(const char *path);
> +char *absolute_pathdup(const char *path);
>  const char *remove_leading_path(const char *in, const char *prefix);
>  const char *relative_path(const char *in, const char *prefix, struct strbuf 
> *sb);
>  int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
> diff --git a/contrib/coccinelle/xstrdup_or_null.cocci 
> b/contrib/coccinelle/xstrdup_or_null.cocci
> index 3fceef132b..8e05d1ca4b 100644
> --- a/contrib/coccinelle/xstrdup_or_null.cocci
> +++ b/contrib/coccinelle/xstrdup_or_null.cocci
> @@ -5,3 +5,9 @@ expression V;
>  - if (E)
>  -V = xstrdup(E);
>  + V = xstrdup_or_null(E);
> +
> +@@
> +expression E;
> +@@
> +- xstrdup(absolute_path(E))
> ++ absolute_pathdup(E)
> -- 
> 2.11.0
> 

These two patches look good to me.

-- 
Brandon Williams

[PATCH] git-bisect: allow running in a working tree subdirectory

[marcandre . lureau]

From: Marc-André Lureau 

It looks like it can do it.

Signed-off-by: Marc-André Lureau 
---
 git-bisect.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-bisect.sh b/git-bisect.sh
index ae3cb013e..b0bd604d4 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,5 +1,6 @@
 #!/bin/sh
 
+SUBDIRECTORY_OK=Yes
 
USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
 LONG_USAGE='git bisect help
print this long help message.
-- 
2.11.0.295.gd7dffce1c.dirty

Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

[Junio C Hamano]

Mike Hommey  writes:

>> With that information recorded in the log (or in-code comment, or
>> both), if it turns out that some lines with the prefix are useful
>> (or some other lines without the prefix are not very useful), they
>> can tweak the filtering criteria as appropriate, with confidence
>> that they _know_ for what purpose the initial "filter lines with the
>> prefix" was trying to serve, and their update is still in the same
>> spirit as the original, only executed better.
>
> Come to think of it, and considering that mutt happily signs emails in
> the same conditions, maybe it would make sense to just ignore gpg return
> code as long as there is a SIG_CREATED message...

I do not think we want to go there.  If GPG reports failure, there
is something funny going on.

Re: [RFC 12/14] fetch-pack: do not printf after closing stdout

[Jonathan Tan]

On 01/25/2017 04:50 PM, Stefan Beller wrote:

On Wed, Jan 25, 2017 at 2:03 PM, Jonathan Tan  wrote:

In fetch-pack, during a stateless RPC, printf is invoked after stdout is
closed. Update the code to not do this, preserving the existing
behavior.


This seems to me as if it could go as an independent
bugfix(?) or refactoring as this seems to be unclear from the code?


The subsequent patches in this patch set are dependent on this patch, 
but it's true that this could be sent out on its own first.


I'm not sure if bugfix is the right word, since the existing behavior is 
correct (except perhaps that we rely on the fact that printf after 
closing stdout does effectively nothing).

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Junio C Hamano]

Jeff King  writes:

> On Thu, Jan 26, 2017 at 07:39:55AM +0100, Johannes Sixt wrote:
>
>> Am 25.01.2017 um 23:01 schrieb Jeff King:
>> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
>> 
>> Last time I used #pragma GCC in a cross-platform project, it triggered an
>> "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't know if
>> the C compiler would also warn.) It would have to be spelled like this:
>> 
>> #pragma warning(disable: 4068)   /* MSVC: unknown pragma */
>> #pragma GCC diagnostic ignored "-Wformat-zero-length"
>> 
>> Dscho mentioned that he's compiling with MSVC. It would do him a favor.
>
> Bleh. The point of #pragma is to ignore ones you don't know about.

Yes.  Let's not go there; somebody else's compiler will complain
about "#pragma warning(disable: 4068)" that it does not understand.

> Anyway. I do not want to make life harder for anyone. I think there are
> several options floating around now, so I will let Junio decide which
> one he wants to pick up.

Well, I'll keep the "do nothing other than squelching this instance"
to solve one of the two problems for now.  

The other "can we make it harder to make the same issue and reduce
the need to discuss this again on the list?" can be an independent
follow-up patch, and I do have a preference (the "less horrible
version, that is static inline warning_blank_line(void)" you gave us
in <20170124230500.h3fasbvutjkkk...@sigill.intra.peff.net>), but I
do not think we are in a hurry.

Thanks.

Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)

[Junio C Hamano]

Lars Schneider  writes:

> The difference between Travis and my machine is that I changed the 
> default shell to ZSH with a few plugins [1]. If I run the test with 
> plain BASH on my Mac then I can reproduce the test failure. Therefore,
> we might want to adjust the commit message if anyone else can reproduce
> the problem on a Mac. 

With "Reportedly macOS does this, at least in the Travis builds.",
even with the result from you in your follow-up message to the
message I am responding to, I think what Peff wrote in the log
message is good enough.

Thanks for digging, and thanks Peff for coming up the workaround.

Re: SubmittingPatches: drop temporal reference for PGP signing

[Junio C Hamano]

Cornelius Weig  writes:

> How about something along these lines? Does the forward reference
> break the main line of thought too severly?

I find it a bit distracting for those who know PGP signing has
nothing to do with signing off your patch, but I think that is OK
because they are not the primary target audience of this part of the
document.

I however am more worried that it may be misleading to mention these
two in the same sentence.  Those who skim these paragraphs without
knowing the difference between the two may get a false impression
that these two may somehow be related because they are mentioned in
the same sentence.

The retitling of section (5) you did, without any other change,
might be sufficient.  It may also help to be even more explicit in
the updated title, i.e. s/by signing off/by adding Signed-off-by:/

Thanks.

> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 08352de..c2b0cbe 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -216,12 +216,12 @@ that it will be postponed.
>  Exception:  If your mailer is mangling patches then someone may ask
>  you to re-send them using MIME, that is OK.
>  
> -Do not PGP sign your patch, at least for now.  Most likely, your
> -maintainer or other people on the list would not have your PGP
> -key and would not bother obtaining it anyway.  Your patch is not
> -judged by who you are; a good patch from an unknown origin has a
> -far better chance of being accepted than a patch from a known,
> -respected origin that is done poorly or does incorrect things.
> +Do not PGP sign your patch, but do sign-off your work as explained in (5).
> +Most likely, your maintainer or other people on the list would not have your
> +PGP key and would not bother obtaining it anyway. Your patch is not judged by
> +who you are; a good patch from an unknown origin has a far better chance of
> +being accepted than a patch from a known, respected origin that is done 
> poorly
> +or does incorrect things.
>  
>  If you really really really really want to do a PGP signed
>  patch, format it as "multipart/signed", not a text/plain message
> @@ -246,7 +246,7 @@ patch.
>   *2* The mailing list: git@vger.kernel.org
>  
>  
> -(5) Sign your work
> +(5) Certify your work by signing off
>  
>  To improve tracking of who did what, we've borrowed the
>  "sign-off" procedure from the Linux kernel project on patches

Re: [PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Stefan Beller]

On Thu, Jan 26, 2017 at 8:08 AM, Johannes Schindelin
 wrote:
> Some developers might want to call `git status` in a working
> directory where they just started an interactive rebase, but the
> edit script is still opened in the editor.
>
> Let's show a meaningful message in such cases.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  t/t7512-status-help.sh | 19 +++
>  wt-status.c| 14 ++
>  2 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index 5c3db656df..458608cc1e 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -944,4 +944,23 @@ EOF
> test_i18ncmp expected actual
>  '
>
> +test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
> +   ONTO=$(git rev-parse --short HEAD^) &&
> +   COMMIT=$(git rev-parse --short HEAD) &&
> +   EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ 
> &&
> +   cat >expected < +On branch several_commits
> +No commands done.
> +Next command to do (1 remaining command):
> +   pick $COMMIT four_commit
> +  (use "git rebase --edit-todo" to view and edit)
> +You are currently editing a commit while rebasing branch 
> '\''several_commits'\'' on '\''$ONTO'\''.
> +  (use "git commit --amend" to amend the current commit)
> +  (use "git rebase --continue" once you are satisfied with your changes)
> +
> +nothing to commit (use -u to show untracked files)
> +EOF
> +   test_i18ncmp expected actual
> +'
> +
>  test_done
> diff --git a/wt-status.c b/wt-status.c
> index a715e71906..4dff0b3e21 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1135,14 +1135,17 @@ static void abbrev_sha1_in_line(struct strbuf *line)
> strbuf_list_free(split);
>  }
>
> -static void read_rebase_todolist(const char *fname, struct string_list 
> *lines)
> +static int read_rebase_todolist(const char *fname, struct string_list *lines)
>  {
> struct strbuf line = STRBUF_INIT;
> FILE *f = fopen(git_path("%s", fname), "r");
>
> -   if (!f)
> +   if (!f) {
> +   if (errno == ENOENT)
> +   return -1;
> die_errno("Could not open file %s for reading",
>   git_path("%s", fname));

While at it, fix the translation with die_errno(_(..),..) ?
(The errno message is translated already by the system,
which make untranslated die_errno things awkward for the users.)

Otherwise the patch looks good to me

Re: [PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Matthieu Moy]

Johannes Schindelin  writes:

> Some developers might want to call `git status` in a working
> directory where they just started an interactive rebase, but the
> edit script is still opened in the editor.
>
> Let's show a meaningful message in such cases.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  t/t7512-status-help.sh | 19 +++
>  wt-status.c| 14 ++
>  2 files changed, 29 insertions(+), 4 deletions(-)

The patch looks good to me.

> @@ -1166,8 +1170,10 @@ static void show_rebase_information(struct wt_status 
> *s,
>   struct string_list yet_to_do = STRING_LIST_INIT_DUP;
>  
>   read_rebase_todolist("rebase-merge/done", _done);
> - read_rebase_todolist("rebase-merge/git-rebase-todo", 
> _to_do);
> -
> + if (read_rebase_todolist("rebase-merge/git-rebase-todo",
> +  _to_do))
> + status_printf_ln(s, color,
> + _("git-rebase-todo is missing."));

I first was surprised not to see this "git-rebase-todo" in the output of
status, but the testcase tests a missing 'done', not a missing todo, so
it's normal.

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: SubmittingPatches: drop temporal reference for PGP signing

[Stefan Beller]

On Thu, Jan 26, 2017 at 5:30 AM, Cornelius Weig
 wrote:
>
>>
>> Yeah I agree. My patch was not the best shot by far.
>>
>
> How about something along these lines? Does the forward reference
> break the main line of thought too severly?
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 08352de..c2b0cbe 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -216,12 +216,12 @@ that it will be postponed.
>  Exception:  If your mailer is mangling patches then someone may ask
>  you to re-send them using MIME, that is OK.
>
> -Do not PGP sign your patch, at least for now.  Most likely, your
> -maintainer or other people on the list would not have your PGP
> -key and would not bother obtaining it anyway.  Your patch is not
> -judged by who you are; a good patch from an unknown origin has a
> -far better chance of being accepted than a patch from a known,
> -respected origin that is done poorly or does incorrect things.
> +Do not PGP sign your patch, but do sign-off your work as explained in (5).
> +Most likely, your maintainer or other people on the list would not have your
> +PGP key and would not bother obtaining it anyway. Your patch is not judged by
> +who you are; a good patch from an unknown origin has a far better chance of
> +being accepted than a patch from a known, respected origin that is done 
> poorly
> +or does incorrect things.
>
>  If you really really really really want to do a PGP signed
>  patch, format it as "multipart/signed", not a text/plain message
> @@ -246,7 +246,7 @@ patch.
>   *2* The mailing list: git@vger.kernel.org
>
>
> -(5) Sign your work
> +(5) Certify your work by signing off
>
>  To improve tracking of who did what, we've borrowed the
>  "sign-off" procedure from the Linux kernel project on patches

I like it.

Thanks,
Stefan

[PATCH 2/2] use absolute_pathdup()

[René Scharfe]

Apply the symantic patch for converting callers that duplicate the
result of absolute_path() to call absolute_pathdup() instead, which
avoids an extra string copy to a static buffer.

Signed-off-by: Rene Scharfe 
---
 builtin/clone.c | 4 ++--
 builtin/submodule--helper.c | 2 +-
 worktree.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 5ef81927a6..3f63edbbf9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -170,7 +170,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
strbuf_addstr(, repo);
raw = get_repo_path_1(, is_bundle);
-   canon = raw ? xstrdup(absolute_path(raw)) : NULL;
+   canon = raw ? absolute_pathdup(raw) : NULL;
strbuf_release();
return canon;
 }
@@ -894,7 +894,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
 
path = get_repo_path(repo_name, _bundle);
if (path)
-   repo = xstrdup(absolute_path(repo_name));
+   repo = absolute_pathdup(repo_name);
else if (!strchr(repo_name, ':'))
die(_("repository '%s' does not exist"), repo_name);
else
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 74614a951e..899dc334e3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -626,7 +626,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
   module_clone_options);
 
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
-   sm_gitdir = xstrdup(absolute_path(sb.buf));
+   sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset();
 
if (!is_absolute_path(path)) {
diff --git a/worktree.c b/worktree.c
index 53b4771c04..d633761575 100644
--- a/worktree.c
+++ b/worktree.c
@@ -145,7 +145,7 @@ static struct worktree *get_linked_worktree(const char *id)
 
 static void mark_current_worktree(struct worktree **worktrees)
 {
-   char *git_dir = xstrdup(absolute_path(get_git_dir()));
+   char *git_dir = absolute_pathdup(get_git_dir());
int i;
 
for (i = 0; worktrees[i]; i++) {
-- 
2.11.0

PATCH 1/2] abspath: add absolute_pathdup()

[René Scharfe]

Add a function that returns a buffer containing the absolute path of its
argument and a semantic patch for its intended use.  It avoids an extra
string copy to a static buffer.

Signed-off-by: Rene Scharfe 
---
 abspath.c| 7 +++
 cache.h  | 1 +
 contrib/coccinelle/xstrdup_or_null.cocci | 6 ++
 3 files changed, 14 insertions(+)

diff --git a/abspath.c b/abspath.c
index fce40fddcc..2f0c26e0e2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -239,6 +239,13 @@ const char *absolute_path(const char *path)
return sb.buf;
 }
 
+char *absolute_pathdup(const char *path)
+{
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_add_absolute_path(, path);
+   return strbuf_detach(, NULL);
+}
+
 /*
  * Unlike prefix_path, this should be used if the named file does
  * not have to interact with index entry; i.e. name of a random file
diff --git a/cache.h b/cache.h
index 00a029af36..d7b7a8cd7a 100644
--- a/cache.h
+++ b/cache.h
@@ -1069,6 +1069,7 @@ const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 char *real_pathdup(const char *path);
 const char *absolute_path(const char *path);
+char *absolute_pathdup(const char *path);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf 
*sb);
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
diff --git a/contrib/coccinelle/xstrdup_or_null.cocci 
b/contrib/coccinelle/xstrdup_or_null.cocci
index 3fceef132b..8e05d1ca4b 100644
--- a/contrib/coccinelle/xstrdup_or_null.cocci
+++ b/contrib/coccinelle/xstrdup_or_null.cocci
@@ -5,3 +5,9 @@ expression V;
 - if (E)
 -V = xstrdup(E);
 + V = xstrdup_or_null(E);
+
+@@
+expression E;
+@@
+- xstrdup(absolute_path(E))
++ absolute_pathdup(E)
-- 
2.11.0

[PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Johannes Schindelin]

Some developers might want to call `git status` in a working
directory where they just started an interactive rebase, but the
edit script is still opened in the editor.

Let's show a meaningful message in such cases.

Signed-off-by: Johannes Schindelin 
---
 t/t7512-status-help.sh | 19 +++
 wt-status.c| 14 ++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 5c3db656df..458608cc1e 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -944,4 +944,23 @@ EOF
test_i18ncmp expected actual
 '
 
+test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
+   ONTO=$(git rev-parse --short HEAD^) &&
+   COMMIT=$(git rev-parse --short HEAD) &&
+   EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
+   cat >expected <

[PATCH v2 0/1] Let `git status` handle a not-yet-started `rebase -i` gracefully

[Johannes Schindelin]

When the `done` file is missing, we die()d. This is not necessary, we
can do much better than that.

Changes since v1:

- When `done` is missing, we still read `git-rebase-todo` and report the
  next steps.

- We now report a missing git-rebase-todo.

- Added a test (thanks, Matthieu, for prodding me into working harder
  ;-)).

- As I changed so much, I took authorship of the patch.


Johannes Schindelin (1):
  status: be prepared for not-yet-started interactive rebase

 t/t7512-status-help.sh | 19 +++
 wt-status.c| 14 ++
 2 files changed, 29 insertions(+), 4 deletions(-)


base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/wt-status-v2
Fetch-It-Via: git fetch https://github.com/dscho/git wt-status-v2

Interdiff vs v1:

 diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
 index 5c3db656df..458608cc1e 100755
 --- a/t/t7512-status-help.sh
 +++ b/t/t7512-status-help.sh
 @@ -944,4 +944,23 @@ EOF
test_i18ncmp expected actual
  '
  
 +test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
 +  ONTO=$(git rev-parse --short HEAD^) &&
 +  COMMIT=$(git rev-parse --short HEAD) &&
 +  EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
 +  cat >expected <

SoC Microprojects 2017

[Pranit Bauva]

Hey everyone,

I helped in just re-organizing the micro project list for 2017. I have
removed the ones which I remember have been done and I have added one
new.

Please add any microproject if it comes to your mind or reply here so
I will add it.

Unfortunately, I can't send the patch here (SMTP blocked by institute
proxy) but I have included it as a link[1]. And here is the PR[2].

[1]: 
https://patch-diff.githubusercontent.com/raw/git/git.github.io/pull/219.patch
[2]: https://github.com/git/git.github.io/pull/219

Regards,
Pranit Bauva

[PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

From: Segev Finer 

This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.

[jes: wrapped overly-long lines, changed get_ssh_variant() to
handle_ssh_variant() to accomodate the change from the
putty/tortoiseplink variables to port_option/needs_batch.]

Signed-off-by: Segev Finer 
Signed-off-by: Johannes Schindelin 
---
 Documentation/config.txt |  7 +++
 Documentation/git.txt|  7 +++
 connect.c| 24 ++--
 t/t5601-clone.sh | 26 ++
 4 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..f2c210f0a0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,13 @@ Environment variable settings always override any 
matches.  The URLs that are
 matched against are those given directly to Git commands.  This means any URLs
 visited as a result of a redirection do not participate in matching.
 
+ssh.variant::
+   Override the autodetection of plink/tortoiseplink in the SSH
+   command that 'git fetch' and 'git push' use. It can be set to
+   either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
+   value will be treated as normal ssh. This is useful in case
+   that Git gets this wrong.
+
 i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..c322558aa7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options 
through your
 personal `.ssh/config` file.  Please consult your ssh documentation
 for further details.
 
+`GIT_SSH_VARIANT`::
+   If this environment variable is set, it overrides the autodetection
+   of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
+   push' use. It can be set to either `ssh`, `plink`, `putty` or
+   `tortoiseplink`. Any other value will be treated as normal ssh. This
+   is useful in case that Git gets this wrong.
+
 `GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 9f750eacb6..7b4437578b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
return NULL;
 }
 
+static int handle_ssh_variant(int *port_option, int *needs_batch)
+{
+   const char *variant;
+
+   if (!(variant = getenv("GIT_SSH_VARIANT")) &&
+   git_config_get_string_const("ssh.variant", ))
+   return 0;
+
+   if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+   *port_option = 'P';
+   else if (!strcmp(variant, "tortoiseplink")) {
+   *port_option = 'P';
+   *needs_batch = 1;
+   }
+
+   return 1;
+}
+
 /*
  * This returns a dummy child_process if the transport protocol does not
  * need fork(2), or a struct child_process object if it does.  Once done,
@@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
ssh_argv0 = xstrdup(ssh);
}
 
-   if (ssh_argv0) {
+   if (!handle_ssh_variant(_option, _batch) &&
+   ssh_argv0) {
const char *base = basename(ssh_argv0);
 
if (!strcasecmp(base, "tortoiseplink") ||
@@ -828,9 +847,10 @@ struct child_process *git_connect(int fd[2], const char 
*url,
   !strcasecmp(base, "plink.exe")) {
port_option = 'P';
}
-   free(ssh_argv0);
}
 
+   free(ssh_argv0);
+
argv_array_push(>args, ssh);
if (flags & CONNECT_IPV4)
argv_array_push(>args, "-4");
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in 
GIT_SSH_COMMAND' '
expect_ssh "-v -P 123" myhost src
 '
 
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+   GIT_SSH_VARIANT=ssh \
+   git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+   expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+   copy_ssh_wrapper_as 

[PATCH v2 2/3] connect: rename tortoiseplink and putty variables

[Johannes Schindelin]

From: Junio C Hamano 

One of these two may have originally been named after "what exact
SSH implementation do we have" so that we can tweak the command line
options, but these days "putty=1" no longer means "We are using the
plink SSH implementation that comes with PuTTY".  It is set when we
guess that either PuTTY plink or Tortoiseplink is in use.

Rename them after what effect is desired.  The current "putty"
option is about using "-P " when OpenSSH would use "-p ",
so rename it to port_option whose value is either 'p' or 'P".  The
other one is about passing an extra command line option "-batch",
so rename it needs_batch.

[jes: wrapped overly-long line]

Signed-off-by: Junio C Hamano 
Signed-off-by: Johannes Schindelin 
---
 connect.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 100644
--- a/connect.c
+++ b/connect.c
@@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
-   int putty = 0, tortoiseplink = 0;
+   int needs_batch = 0;
+   int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (ssh_argv0) {
const char *base = basename(ssh_argv0);
 
-   tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
-   !strcasecmp(base, "tortoiseplink.exe");
-   putty = tortoiseplink ||
-   !strcasecmp(base, "plink") ||
-   !strcasecmp(base, "plink.exe");
-
+   if (!strcasecmp(base, "tortoiseplink") ||
+   !strcasecmp(base, "tortoiseplink.exe")) {
+   port_option = 'P';
+   needs_batch = 1;
+   } else if (!strcasecmp(base, "plink") ||
+  !strcasecmp(base, "plink.exe")) {
+   port_option = 'P';
+   }
free(ssh_argv0);
}
 
@@ -833,11 +836,11 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(>args, "-6");
-   if (tortoiseplink)
+   if (needs_batch)
argv_array_push(>args, "-batch");
if (port) {
-   /* P is for PuTTY, p is for OpenSSH */
-   argv_array_push(>args, putty ? "-P" : 
"-p");
+   argv_array_pushf(>args,
+"-%c", port_option);
argv_array_push(>args, port);
}
argv_array_push(>args, ssh_host);
-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v2 1/3] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

From: Segev Finer 

Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer 
Signed-off-by: Johannes Schindelin 
---
 connect.c| 23 ---
 t/t5601-clone.sh | 15 +++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
int putty = 0, tortoiseplink = 0;
char *ssh_host = hostandport;
const char *port = NULL;
+   char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char 
*url,
}
 
ssh = get_ssh_command();
-   if (!ssh) {
-   const char *base;
-   char *ssh_dup;
-
+   if (ssh) {
+   char *split_ssh = xstrdup(ssh);
+   const char **ssh_argv;
+
+   if (split_cmdline(split_ssh, _argv))
+   ssh_argv0 = xstrdup(ssh_argv[0]);
+   free(split_ssh);
+   free((void *)ssh_argv);
+   } else {
/*
 * GIT_SSH is the no-shell version of
 * GIT_SSH_COMMAND (and must remain so for
@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (!ssh)
ssh = "ssh";
 
-   ssh_dup = xstrdup(ssh);
-   base = basename(ssh_dup);
+   ssh_argv0 = xstrdup(ssh);
+   }
+
+   if (ssh_argv0) {
+   const char *base = basename(ssh_argv0);
 
tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
!strcasecmp(base, "tortoiseplink.exe");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
!strcasecmp(base, "plink") ||
!strcasecmp(base, "plink.exe");
 
-   free(ssh_dup);
+   free(ssh_argv0);
}
 
argv_array_push(>args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with 
extra arguments' '
expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+   GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+   git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+   expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+   GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+   git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+   expect_ssh "-v -P 

[PATCH v2 0/3] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

[Johannes Schindelin]

We already handle PuTTY's plink and TortoiseGit's tortoiseplink in
GIT_SSH by automatically using the -P option to specify ports, and in
tortoiseplink's case by passing the --batch option.

For users who need to pass additional command-line options to plink,
this poses a problem: the only way to do that is to use GIT_SSH_COMMAND,
but Git does not handle that specifically, so those users have to
manually parse the command-line options passed via GIT_SSH_COMMAND and
replace -p (if present) by -P, and add --batch in the case of
tortoiseplink.

This is error-prone and a bad user experience.

To fix this, the changes proposed in this patch series introduce
handling this by splitting the GIT_SSH_COMMAND value and treating the
first parameter with the same grace as GIT_SSH. To counter any possible
misdetection, the user can also specify explicitly via GIT_SSH_VARIANT
or ssh.variant which SSH variant they are using.

This is v2 of the patch, now turned patch series. Relative to v1, it
integrates Junio's cleanup patch and Segev's follow-up Pull Request that
introduces the GIT_SSH_VARIANT and ssh.variant settings to override
Git's autodetection manually.


Junio C Hamano (1):
  connect: rename tortoiseplink and putty variables

Segev Finer (2):
  connect: handle putty/plink also in GIT_SSH_COMMAND
  connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

 Documentation/config.txt |  7 +
 Documentation/git.txt|  7 +
 connect.c| 66 +++-
 t/t5601-clone.sh | 41 ++
 4 files changed, 104 insertions(+), 17 deletions(-)


base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v2
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v2

Interdiff vs v1:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index af2ae4cc02..f2c210f0a0 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1949,6 +1949,13 @@ Environment variable settings always override any 
matches.  The URLs that are
  matched against are those given directly to Git commands.  This means any URLs
  visited as a result of a redirection do not participate in matching.
  
 +ssh.variant::
 +  Override the autodetection of plink/tortoiseplink in the SSH
 +  command that 'git fetch' and 'git push' use. It can be set to
 +  either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
 +  value will be treated as normal ssh. This is useful in case
 +  that Git gets this wrong.
 +
  i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index 4f208fab92..c322558aa7 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options 
through your
  personal `.ssh/config` file.  Please consult your ssh documentation
  for further details.
  
 +`GIT_SSH_VARIANT`::
 +  If this environment variable is set, it overrides the autodetection
 +  of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
 +  push' use. It can be set to either `ssh`, `plink`, `putty` or
 +  `tortoiseplink`. Any other value will be treated as normal ssh. This
 +  is useful in case that Git gets this wrong.
 +
  `GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
 diff --git a/connect.c b/connect.c
 index c81f77001b..7b4437578b 100644
 --- a/connect.c
 +++ b/connect.c
 @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
return NULL;
  }
  
 +static int handle_ssh_variant(int *port_option, int *needs_batch)
 +{
 +  const char *variant;
 +
 +  if (!(variant = getenv("GIT_SSH_VARIANT")) &&
 +  git_config_get_string_const("ssh.variant", ))
 +  return 0;
 +
 +  if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
 +  *port_option = 'P';
 +  else if (!strcmp(variant, "tortoiseplink")) {
 +  *port_option = 'P';
 +  *needs_batch = 1;
 +  }
 +
 +  return 1;
 +}
 +
  /*
   * This returns a dummy child_process if the transport protocol does not
   * need fork(2), or a struct child_process object if it does.  Once done,
 @@ -769,7 +787,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
 -  int putty = 0, tortoiseplink = 0;
 +  int needs_batch = 0;
 +  int port_option = 'p';
char *ssh_host = hostandport;
const 

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Jan 2017, Jeff King wrote:

> On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:
> 
> > We could switch the DEVELOPER option on by default, when gcc or clang
> > is used at least. Otherwise the DEVELOPER option (which I like very
> > much) would not be able to live up to its full potential.
> 
> I'm not sure that is a good idea. The options include -Werror, which is
> a good thing for developers to respect. But people using older versions
> of compilers, or on systems with slightly different header files, may
> see extraneous warnings. It's good to fix those warnings, but it is a
> big inconvenience to regular users who just want to build and use git.

Yeah, you cannot have the cake and eat it, too: on the one side, we want
Git contributors to see problems early (preferably before even submitting
the patch) and at the same time, we want users who compile their Git
themselves to have no trouble doing so.

> You could split the DEVELOPER options into two groups, though, and only
> enable when (after verifying that it is indeed gcc/clang in use). But
> now who is coming up with complicated fixes for the warning("") issue?
> :)

That is yet another instance of the complicator's glove; I would rather
avoid that.

To me, the obvious solution is to pay more attention to Continuous
Integration, in particular on fixing problems right after they are
reported.

Ciao,
Johannes

Re: [PATCH] refs: add option core.logAllRefUpdates = always

[Jeff King]

On Thu, Jan 26, 2017 at 03:06:40PM +0100, Cornelius Weig wrote:

> > But it works quite by accident. I wonder if we should this
> > "is_bare_repository" check into a function that can be called instead of
> > accessing log_all_ref_updates() directly.
> 
> Are you saying that we should move the `!log_all_ref_updates` check into
> its own function where we should also check `is_bare_repository`? I
> don't see that this would win much, because as you said: checkouts in a
> bare repo are forbidden anyway.

Yes, I'm suggesting making something like the should_autocreate_reflog()
function public.

I agree it is working correctly now. It's just that it's rather subtle
that it treats LOG_REFS_UNSET implicitly as LOG_REFS_NONE.

It would also possibly break if more values are added to the enum
(depending on what those values are).

> However, I realized that I have not written tests about ref updates in a
> bare repository. Do you think it's worthwile?

There should already be a test for logAllRefUpdates=true in a bare
repository (if there isn't, we should probably add one). Testing the
"always" case individually does not add much over testing it in a
non-bare repository. IMHO.

-Peff

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

> Subject: [PATCH] connect: rename tortoiseplink and putty variables
> 
> One of these two may have originally been named after "what exact
> SSH implementation do we have" so that we can tweak the command line
> options, but these days "putty=1" no longer means "We are using the
> plink SSH implementation that comes with PuTTY".  It is set when we
> guess that either PuTTY plink or Tortoiseplink is in use.
> 
> Rename them after what effect is desired.  The current "putty"
> option is about using "-P " when OpenSSH would use "-p ",
> so rename it to port_option whose value is either 'p' or 'P".  The
> other one is about passing an extra command line option "-batch",
> so rename it needs_batch.
> 
> Signed-off-by: Junio C Hamano 

Apart from an overly-long line, this patch looks good. It made my life a
little harder, as I had to rebase Segev's ssh.variant (why this should be
a core.* variable is not clear to me) patch and it caused conflicts. I
resolved those conflicts and made both patches part of this patch series.

Will contribute v2 as soon as the test suite passes,
Johannes

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Jeff King]

On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:

> We could switch the DEVELOPER option on by default, when gcc or clang is
> used at least. Otherwise the DEVELOPER option (which I like very much)
> would not be able to live up to its full potential.

I'm not sure that is a good idea. The options include -Werror, which is
a good thing for developers to respect. But people using older versions
of compilers, or on systems with slightly different header files, may
see extraneous warnings. It's good to fix those warnings, but it is a
big inconvenience to regular users who just want to build and use git.

You could split the DEVELOPER options into two groups, though, and only
enable when (after verifying that it is indeed gcc/clang in use). But
now who is coming up with complicated fixes for the warning("") issue?
:)

-Peff

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Jeff King]

On Thu, Jan 26, 2017 at 12:37:46PM +0100, Johannes Schindelin wrote:

> > Am 25.01.2017 um 23:01 schrieb Jeff King:
> > > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
> > 
> > Last time I used #pragma GCC in a cross-platform project, it triggered
> > an "unknown pragma" warning for MSVC.
> 
> It is starting to become a little funny how many ways we can discuss the
> resolution of the GCC compiler warning.
> 
> And it starts to show: we try to solve the thing in so many ways, just to
> avoid the obviously most-trivial patch to change warning(""); to
> warning("%s", "") (the change to warning(" "); would change behavior, but
> I would be fine with that, too).

The point is that the trivial patch fixes _this_ case, but does not
prevent the discussion from happening again later. They are two separate
problems. I am OK not solving the latter one and relying on review (as
I've already said), but the solutions do not do the same thing.

-Peff

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Jeff King]

On Thu, Jan 26, 2017 at 07:39:55AM +0100, Johannes Sixt wrote:

> Am 25.01.2017 um 23:01 schrieb Jeff King:
> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Last time I used #pragma GCC in a cross-platform project, it triggered an
> "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't know if
> the C compiler would also warn.) It would have to be spelled like this:
> 
> #pragma warning(disable: 4068)   /* MSVC: unknown pragma */
> #pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Dscho mentioned that he's compiling with MSVC. It would do him a favor.

Bleh. The point of #pragma is to ignore ones you don't know about.

It would be easy to wrap it in an #ifdef for __GNUC__ (there is already
a similar pragma with similar wrapping in the code base).

Anyway. I do not want to make life harder for anyone. I think there are
several options floating around now, so I will let Junio decide which
one he wants to pick up.

-Peff

Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)

[Jeff King]

On Thu, Jan 26, 2017 at 10:48:30AM +0100, Lars Schneider wrote:

> Oh. I must have made a mistake on my very first test run. I can reproduce
> the failure with ZSH and my plugins... looks like it's a Mac OS problem
> and no TravisCI only problem after all.

Thanks for digging into it. If it's really /bin/mv that causes the
problem, then I doubly think the "mv -f" patch is the right fix.

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Thu, Jan 26, 2017 at 04:28:17PM +0700, Duy Nguyen wrote:

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King  wrote:
> > I don't think it means either. It means to include remotes in the
> > selected revisions, but excluding the entries mentioned by --exclude.
> >
> > IOW:
> >
> >   --exclude=foo --remotes
> > include all remotes except refs/remotes/foo
> >
> >   --exclude=foo --unrelated --remotes
> > same
> >
> >   --exclude=foo --decorate-reflog --remotes
> > decorate reflogs of all remotes except "foo". Do _not_ use them
> > as traversal tips.
> >
> >   --decorate-reflog --exclude=foo --remotes
> > same
> >
> > IOW, the ref-selector options build up until a group option is given,
> > which acts on the built-up options (over that group) and then resets the
> > built-up options. Doing "--unrelated" as above is orthogonal (though I
> > think in practice nobody would do that, because it's hard to read).
> 
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog? Should we simply catch
> such combinations and error out (which may be a bit more complicated
> than this patch, or maybe not)?

I'd cross that bridge when we see what the option is. But my gut is that
rules would be:

  - apply all non-conflicting relevant options. So:

  --exclude=foo/* --decorate-refs --decorate-reflog --remotes

would presumably decorate both ref tips _and_ reflogs for all
remotes (except ones in refs/remotes/foo/*)

  - for ones that are directly related and override each other,
use the usual last-one-wins rule. So:

  --decorate-reflog --no-decorate-reflog --remotes

would countermand the original --decorate-reflog.

  - for ones that really have complex interactions, notice and complain
in handle_refs().

That just seems to me like it follows our usual option parsing
procedure. The only difference here is that process and reset some
subset of the flags when we hit a special marker option ("--remotes" in
these examples) instead of doing it at the end.

-Peff

Re: [PATCH 2/5] revision.c: group ref selection options together

[Jeff King]

On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:

> > Hmm. I see what you're trying to do here, and abstract the repeated
> > bits. But I'm not sure the line-count reflects a real simplification.
> > Everything ends up converted to an enum, and then that enum just expands
> > to similar C code.
> 
> It's not simplification, but hopefully for better maintainability. This
> 
> if (strcmp(arg, "--remotes")) {
>if (preceded_by_exclide())
>   does_something();
>else if (preceded_by_decorate())
>   does_another()
> } else if (strcmp(arg, "--branches")) {
>if (preceded_by_exclide())
>   does_something();
>else if (preceded_by_decorate())
>   does_another()
> }
> 
> starts to look ugly especially when the third "preceded_by_" comes
> into picture. Putting all "does_something" in one group and
> "does_another" in another, I think, gives us a better view how ref
> selection is handled for a specific operation like --exclude or
> --decorate-ref.

I agree that would be ugly. But the current structure, which is:

  if (strcmp(arg, "--remotes")) {
  handle_refs(...);
  cleanup();
  } else if(...) {
  handle_refs(...);
  cleanup();
  }

does not seem so bad, and pushes those conditionals into the
handle_refs() function, where they only need to be expressed once (I
didn't look, but I wonder if you could push the cleanup steps in there,
too, or if there is a caller who wants to handle() multiple times before
cleaning up).

-Peff

Re: [PATCH] refs: add option core.logAllRefUpdates = always

[Cornelius Weig]

Hi Peff,

 thanks for your thoughts.

> I tried to read this patch with fresh eyes. But given the history, you
> may take my review with a grain of salt. :)

Does it mean another reviewer is needed?

> I don't think my original had tests for this, but it might be worth
> adding a test for this last bit (i.e., that an update of ORIG_HEAD does
> not write a reflog when logallrefupdates is set to "always").

Good point. I blindly copied your commit message without thinking too
much about it.

> I guess the backtick fixups came from my original. It might be easier to
> see the change if they were pulled into their own patch, but it's
> probably not that big a deal.

If it's best practice to break out such changes, I'll revise it.

>> @@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const 
>> unsigned char *old_sha1,
>>  {
>>  int logfd, result, oflags = O_APPEND | O_WRONLY;
>>  
>> -if (log_all_ref_updates < 0)
>> -log_all_ref_updates = !is_bare_repository();
>> +if (log_all_ref_updates == LOG_REFS_UNSET)
>> +log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : 
>> LOG_REFS_NORMAL;
> 
> This hunk is new, I think. The enum values are set in such a way that
> the original code would have continued to work, but I think using the
> symbolic names is an improvement.

Yes it's new.

> I assume you grepped for log_all_ref_updates to find this. I see only
> one spot that now doesn't use the symbolic names. In builtin/checkout.c,
> update_refs_for_switch() checks:
> 
>   if (opts->new_branch_log && !log_all_ref_updates)
> 
> That looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET
> the same, and I do not see us resolving the UNSET case to a true/false
> value. But I don't think the bug is new in your patch; the default value
> was "-1" already.
>
> I doubt it can be triggered in practice, because either:
> 
>   - the config value is set in the config file, and we pick up that
> value, whether it's "true" or "false"
> 
>   - it's unset, in which case our default would be to enable reflogs in
> a non-bare repo. And since git-checkout would refuse to run in a
> bare repo, we must be non-bare, and thus enabling reflogs does the
> right thing.

That far I can follow.

> But it works quite by accident. I wonder if we should this
> "is_bare_repository" check into a function that can be called instead of
> accessing log_all_ref_updates() directly.

Are you saying that we should move the `!log_all_ref_updates` check into
its own function where we should also check `is_bare_repository`? I
don't see that this would win much, because as you said: checkouts in a
bare repo are forbidden anyway.

Other than that, I guess it should better read `log_all_ref_update !=
LOG_REFS_NONE` instead of `!log_all_ref_updates`.


>> +test_expect_success 'update-ref does not create reflog with 
>> --no-create-reflog if core.logAllRefUpdates=always' '
> 
> This test title is _really_ long, and will wrap in the output on
> reasonable-sized terminals. Maybe '--no-create-reflog overrides
> core.logAllRefUpdates=always' would be shorter?

Yes, I agree.

>> +test_expect_success 'stdin does not create reflog when 
>> core.logAllRefUpdates=true' '
> 
> I don't mind these extra stdin tests, but IMHO they are just redundant.
> The "--stdin --create-reflog" one makes sure the option is propagated
> down via the --stdin machinery. But we know the config option is handled
> at a low level anyway.
> 
> I guess it depends on how black-box we want the testing to be. It just
> seems unlikely for a regression to be found here and not in the tests
> above.

Since these other stdin tests were around, I added this variant. But
you're right: this test breaks along with the other and doesn't add add
more safety. I'll remove it.

However, I realized that I have not written tests about ref updates in a
bare repository. Do you think it's worthwile?

Cheers,
  Cornelius

Re: [DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)

[Johannes Schindelin]

Hi,

On Wed, 25 Jan 2017, Jeff King wrote:

> On Wed, Jan 25, 2017 at 07:43:01PM +0100, René Scharfe wrote:
> 
> > If we find such cases then we'd better fix them for all platforms, e.g. by
> > importing timsort, no?
> 
> Yes, as long as they are strict improvements.

I think in many cases, we may be better off by replacing the use of a
string-list for lookups by hashmaps for the same purpose.

Ciao,
Dscho

Re: [PATCH] mingw: allow hooks to be .exe files

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This change is necessary to allow the files in .git/hooks/ to optionally
> > have the file extension `.exe` on Windows, as the file names are
> > hardcoded otherwise.
> 
> Hmph.  There is no longer ".com"?

No, no longer .com. You have to jump through hoops in this century to
build .com files.

> I briefly wondered if hooks/post-receive.{py,rb,...} would be good
> things to support, but I do not think we want to go there, primarily
> because we do not want to deal with "what happens when there are many?"

The answer is correct, the reasoning not. The reason why .exe is special:
it simply won't execute unless it has the .exe file extension. That is not
true for .py, .rb, etc

Ciao,
Johannes

Re: SubmittingPatches: drop temporal reference for PGP signing

[Cornelius Weig]

> 
> Yeah I agree. My patch was not the best shot by far.
> 

How about something along these lines? Does the forward reference
break the main line of thought too severly?

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 08352de..c2b0cbe 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -216,12 +216,12 @@ that it will be postponed.
 Exception:  If your mailer is mangling patches then someone may ask
 you to re-send them using MIME, that is OK.
 
-Do not PGP sign your patch, at least for now.  Most likely, your
-maintainer or other people on the list would not have your PGP
-key and would not bother obtaining it anyway.  Your patch is not
-judged by who you are; a good patch from an unknown origin has a
-far better chance of being accepted than a patch from a known,
-respected origin that is done poorly or does incorrect things.
+Do not PGP sign your patch, but do sign-off your work as explained in (5).
+Most likely, your maintainer or other people on the list would not have your
+PGP key and would not bother obtaining it anyway. Your patch is not judged by
+who you are; a good patch from an unknown origin has a far better chance of
+being accepted than a patch from a known, respected origin that is done poorly
+or does incorrect things.
 
 If you really really really really want to do a PGP signed
 patch, format it as "multipart/signed", not a text/plain message
@@ -246,7 +246,7 @@ patch.
  *2* The mailing list: git@vger.kernel.org
 
 
-(5) Sign your work
+(5) Certify your work by signing off
 
 To improve tracking of who did what, we've borrowed the
 "sign-off" procedure from the Linux kernel project on patches

Re: [PATCH 2/2] ref-filter: add function to parse atoms from a nul-terminated string

[SZEDER Gábor]

On Wed, Dec 7, 2016 at 5:09 PM, SZEDER Gábor  wrote:
> ref-filter's parse_ref_filter_atom() function parses an atom between
> the start and end pointers it gets as arguments.  This is fine for two
> of its callers, which process '%(atom)' format specifiers and the end
> pointer comes directly from strchr() looking for the closing ')'.
> However, it's not quite so straightforward for its other two callers,
> which process sort specifiers given as plain nul-terminated strings.
> Especially not for ref_default_sorting(), which has the default
> hard-coded as a string literal, but can't use it directly, because a
> pointer to the end of that string literal is needed as well.
> The next patch will add yet another caller using a string literal.
>
> Add a helper function around parse_ref_filter_atom() to parse an atom
> up to the terminating nul, and call this helper in those two callers.
>
> Signed-off-by: SZEDER Gábor 
> ---
>  ref-filter.c | 8 ++--
>  ref-filter.h | 4 
>  2 files changed, 6 insertions(+), 6 deletions(-)

Ping?

It looks like that this little two piece cleanup series fell on the floor.



> diff --git a/ref-filter.c b/ref-filter.c
> index dfadf577c..3c6fd4ba7 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1658,19 +1658,16 @@ void show_ref_array_item(struct ref_array_item *info, 
> const char *format, int qu
>  /*  If no sorting option is given, use refname to sort as default */
>  struct ref_sorting *ref_default_sorting(void)
>  {
> -   static const char cstr_name[] = "refname";
> -
> struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
>
> sorting->next = NULL;
> -   sorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + 
> strlen(cstr_name));
> +   sorting->atom = parse_ref_filter_atom_from_string("refname");
> return sorting;
>  }
>
>  void parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
>  {
> struct ref_sorting *s;
> -   int len;
>
> s = xcalloc(1, sizeof(*s));
> s->next = *sorting_tail;
> @@ -1683,8 +1680,7 @@ void parse_sorting_string(const char *arg, struct 
> ref_sorting **sorting_tail)
> if (skip_prefix(arg, "version:", ) ||
> skip_prefix(arg, "v:", ))
> s->version = 1;
> -   len = strlen(arg);
> -   s->atom = parse_ref_filter_atom(arg, arg+len);
> +   s->atom = parse_ref_filter_atom_from_string(arg);
>  }
>
>  int parse_opt_ref_sorting(const struct option *opt, const char *arg, int 
> unset)
> diff --git a/ref-filter.h b/ref-filter.h
> index 49466a17d..1250537cf 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -94,6 +94,10 @@ int filter_refs(struct ref_array *array, struct ref_filter 
> *filter, unsigned int
>  void ref_array_clear(struct ref_array *array);
>  /*  Parse format string and sort specifiers */
>  int parse_ref_filter_atom(const char *atom, const char *ep);
> +static inline int parse_ref_filter_atom_from_string(const char *atom)
> +{
> +   return parse_ref_filter_atom(atom, atom+strlen(atom));
> +}
>  /*  Used to verify if the given format is correct and to parse out the used 
> atoms */
>  int verify_ref_format(const char *format);
>  /*  Sort the given ref_array as per the ref_sorting provided */
> --
> 2.11.0.78.g5a2d011
>

[PATCH v2] mingw: allow hooks to be .exe files

[Johannes Schindelin]

This change is necessary to allow the files in .git/hooks/ to optionally
have the file extension `.exe` on Windows, as the file names are
hardcoded otherwise.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v2
Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v2
Interdiff vs v1:

 diff --git a/run-command.c b/run-command.c
 index 45229ef052..5227f78aea 100644
 --- a/run-command.c
 +++ b/run-command.c
 @@ -873,7 +873,7 @@ const char *find_hook(const char *name)
strbuf_git_path(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
  #ifdef STRIP_EXTENSION
 -  strbuf_addstr(, ".exe");
 +  strbuf_addstr(, STRIP_EXTENSION);
if (access(path.buf, X_OK) >= 0)
return path.buf;
  #endif


 run-command.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 73bfba7ef9..5227f78aea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -871,8 +871,14 @@ const char *find_hook(const char *name)
 
strbuf_reset();
strbuf_git_path(, "hooks/%s", name);
-   if (access(path.buf, X_OK) < 0)
+   if (access(path.buf, X_OK) < 0) {
+#ifdef STRIP_EXTENSION
+   strbuf_addstr(, STRIP_EXTENSION);
+   if (access(path.buf, X_OK) >= 0)
+   return path.buf;
+#endif
return NULL;
+   }
return path.buf;
 }
 

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
-- 
2.11.1.windows.prerelease.2.9.g3014b57

Re: [PATCH] mingw: allow hooks to be .exe files

[Johannes Schindelin]

Hi Peff,

On Wed, 25 Jan 2017, Jeff King wrote:

> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
> 
> > -   if (access(path.buf, X_OK) < 0)
> > +   if (access(path.buf, X_OK) < 0) {
> > +#ifdef STRIP_EXTENSION
> > +   strbuf_addstr(, ".exe");
> 
> I think STRIP_EXTENSION is a string.  Should this line be:
> 
>   strbuf_addstr(, STRIP_EXTENSION);

Yep.

v2 coming,
Johannes

Re: [PATCH] Retire the `relink` command

[Johannes Schindelin]

Hi Eric,

On Wed, 25 Jan 2017, Eric Wong wrote:

> Johannes Schindelin  wrote:
> > Back in the olden days, when all objects were loose and rubber boots
> > were made out of wood, it made sense to try to share (immutable)
> > objects between repositories.
> > 
> > Ever since the arrival of pack files, it is but an anachronism.
> > 
> > Let's move the script to the contrib/examples/ directory and no longer
> > offer it.
> 
> On the other hand, we have no idea if there are still people
> using it for whatever reason...
> 
> I suggest we have a deprecation period where:

I would be fine with a deprecation phase, but that decision is solely on
Junio's shoulders.

Ciao,
Johannes

Re: [PATCH 2/2] fsck: lazily load types under --connectivity-only

[Johannes Schindelin]

Hi Peff,

On Wed, 25 Jan 2017, Jeff King wrote:

>  builtin/fsck.c | 58 
> +++---
>  fsck.c |  4 
>  2 files changed, 11 insertions(+), 51 deletions(-)

Patch looks good to my eyes.

Ciao,
Johannes

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Now, with the patch in question (without the follow-up, which I would
> > like to ask you to ignore, just like you did so far), Git would not
> > figure out that your script calls PuTTY eventually. The work-around?
> > Easy:
> >
> > DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh
> 
> Think about how you would explain that to an end-user in our document?
> You'll need to explain how exactly the auto-detection works, so that the
> user can "exploit" the loophole to do that.  And what maintenance burden
> does it add when auto-detection is updated?

Fine, you do not like it. Saying so (instead of asking me questions) would
have been helpful.

> I think I know you well enough that you know well enough that it is too
> ugly to live, and I suspect that the above is a tongue-in-cheek "arguing
> for the sake of argument" and would not need a serious response, but
> just in case...

It was not tongue-in-cheek, I was being serious.

> Yes.  Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).

I'd much rather prefer
https://github.com/git-for-windows/git/pull/1030 than your patch.

Ciao,
Johannes

Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor

[Johannes Schindelin]

Hi Brian,

On Thu, 26 Jan 2017, brian m. carlson wrote:

> AsciiDoc uses a configuration file to implement macros like linkgit,
> while Asciidoctor uses Ruby extensions.  Implement a Ruby extension that
> implements the linkgit macro for Asciidoctor in the same way that
> asciidoc.conf does for AsciiDoc.  Adjust the Makefile to use it by
> default.

I like it.

Thank you,
Johannes

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Johannes Schindelin]

Hi Hannes,

On Thu, 26 Jan 2017, Johannes Sixt wrote:

> Am 25.01.2017 um 23:01 schrieb Jeff King:
> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Last time I used #pragma GCC in a cross-platform project, it triggered
> an "unknown pragma" warning for MSVC.

It is starting to become a little funny how many ways we can discuss the
resolution of the GCC compiler warning.

And it starts to show: we try to solve the thing in so many ways, just to
avoid the obviously most-trivial patch to change warning(""); to
warning("%s", "") (the change to warning(" "); would change behavior, but
I would be fine with that, too).

I am not really interested in any of these complicated workarounds. If you
gentle people decide they are better in Git's source code, go ahead. I do
not have to like what you are doing, I just have to work with it.

> (It was the C++ compiler, I don't know if the C compiler would also
> warn.) It would have to be spelled like this:
> 
> #pragma warning(disable: 4068)   /* MSVC: unknown pragma */
> #pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Dscho mentioned that he's compiling with MSVC. It would do him a favor.

I am compiling with MSVC, and the idea is to tap into that large number of
Windows developers who Git traditionally has had a really bad time
attracting. From that perspective, I would say it would not only do me a
favor, but anybody who builds Git for Windows using Visual Studio.

But we also have to consider whether it would do anybody a "dis-favor".
#pragma statements are by definition highly dependent on the compiler. I
have no idea whether there are developers out there building Git with
C compilers other than GCC, clang or MSVC (as I did back in the days on
IRIX and HP/UX), but there is quite the potential for problems here [*1*].

To keep Git's source code truly portable, the #pragma would have to be
guarded by a GCC-specific #ifdef ... #endif.

Ciao,
Dscho

Footnote *1*: This is just another instance where a discussion on the Git
mailing list reminds me of
http://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves, as it tries
to avoid an obvious solution by trying to come up with a different
solution that in turn requires additional solutions to additional problems
caused by the alternative solution.

Re: sparse checkout - weird behavior

[Paul Hammant]

Well I feel a bit silly. Thanks for responding.

On Wed, Jan 25, 2017 at 11:59 PM, Jeff King  wrote:
> On Wed, Jan 25, 2017 at 10:21:19PM -0500, Paul Hammant wrote:
>
>> Related bug (maybe the same). Reproduction:
>>
>>   $ git clone g...@github.com:jekyll/jekyll.git --no-checkout
>>   Cloning into 'jekyll'...
>>   remote: Counting objects: 41331, done.
>>   remote: Compressing objects: 100% (5/5), done.
>>   remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
>>   Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
>>   Resolving deltas: 100% (26530/26530), done.
>>   $ cd jekyll
>>   $ git config core.sparsecheckout true
>>   $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
>>   $ echo 'Gemfile' >> .git/info/sparse-checkout
>>   $ echo 'Rakefile' >> .git/info/sparse-checkout
>>   $ echo 'appveyor.yml' >> .git/info/sparse-checkout
>>   $ git checkout --
>>   Your branch is up-to-date with 'origin/master'.
>>   $ ls
>>   CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
>>
>> I was not expecting to see 'lib' in the resulting file list
>
> Yep, I think this is the same problem. Inside lib, you get only
> "lib/theme_template/Gemfile", because it matches your unanchored
> pattern. Using "/Gemfile" in the sparse-checkout file fixes it.
>
> -Peff

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Johannes Schindelin]

Hi Peff,

On Wed, 25 Jan 2017, Jeff King wrote:

> On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:
> 
> > > Gross, but at least it's self documenting. :)
> > > 
> > > I guess a less horrible version of that is:
> > > 
> > >   static inline warning_blank_line(void)
> > >   {
> > >   warning("%s", "");
> > >   }
> > > 
> > > We'd potentially need a matching one for error(), but at last it avoids
> > > macro trickery.
> > 
> > I fail to see how this function, or this definition, makes the code better
> > than simply calling `warning("%s", "");` and be done with it.
> 
> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to ("").

We could switch the DEVELOPER option on by default, when gcc or clang is
used at least. Otherwise the DEVELOPER option (which I like very much)
would not be able to live up to its full potential.

Another thing we should consider: paying more attention to Continuous
Integration. At the moment, it happens quite frequently that `pu` builds
and passes the test suite fine on Linux, but neither on Windows nor on
MacOSX and it takes days to get the regressions fixed.

I vote for this patch:

> -- >8 --
> Subject: [PATCH] difftool: hack around -Wzero-length-format warning
> 
> Building with "gcc -Wall" will complain that the format in:
> 
>   warning("")
> 
> is empty. Which is true, but the warning is over-eager. We
> are calling the function for its side effect of printing
> "warning:", even with an empty string.
> 
> Our DEVELOPER Makefile knob disables the warning, but not
> everybody uses it. Let's silence the warning in the code so
> that nobody reports it or tries to "fix" it.
> 
> Signed-off-by: Jeff King 
> ---
>  builtin/difftool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 42ad9e804..b5e85ab07 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
> const char *prefix,
>   warning(_("both files modified: '%s' and 
> '%s'."),
>   wtdir.buf, rdir.buf);
>   warning(_("working tree file has been left."));
> - warning("");
> + warning("%s", "");
>   err = 1;
>   } else if (unlink(wtdir.buf) ||
>  copy_file(wtdir.buf, rdir.buf, st.st_mode))
> -- 
> 2.11.0.840.gd37c5973a

Ciao,
Dscho

Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)

[Lars Schneider]

> On 26 Jan 2017, at 10:14, Lars Schneider  wrote:
> 
> 
>> On 25 Jan 2017, at 23:51, Junio C Hamano  wrote:
>> 
>> Jeff King  writes:
>> 
>>> I guess the way to dig would be to add a test that looks at the output
>>> of "type mv" or something, push it to a Travis-hooked branch, and then
>>> wait for the output
>> 
>> Sounds tempting ;-)
> 
> Well, I tried that:
> 
> mv is /bin/mv
> 
> ... and "/bin/mv" is exactly the version that I have on my machine.
> 
> The difference between Travis and my machine is that I changed the 
> default shell to ZSH with a few plugins [1]. If I run the test with 
> plain BASH on my Mac then I can reproduce the test failure. Therefore,
> we might want to adjust the commit message if anyone else can reproduce
> the problem on a Mac. 
> 
> I can even reproduce the failure if I run the test with plain ZSH. 
> However, I can't find a plugin that defines an alias for "mv". Puzzled...
> 
> - Lars
> 
> [1] https://github.com/robbyrussell/oh-my-zsh

Oh. I must have made a mistake on my very first test run. I can reproduce
the failure with ZSH and my plugins... looks like it's a Mac OS problem
and no TravisCI only problem after all. 

Sorry for the noise/confusion,
Lars

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Duy Nguyen]

On Thu, Jan 26, 2017 at 3:57 AM, Jeff King  wrote:
> I don't think it means either. It means to include remotes in the
> selected revisions, but excluding the entries mentioned by --exclude.
>
> IOW:
>
>   --exclude=foo --remotes
> include all remotes except refs/remotes/foo
>
>   --exclude=foo --unrelated --remotes
> same
>
>   --exclude=foo --decorate-reflog --remotes
> decorate reflogs of all remotes except "foo". Do _not_ use them
> as traversal tips.
>
>   --decorate-reflog --exclude=foo --remotes
> same
>
> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).

This is because it makes sense to combine --exclude and
--decorate-reflog. But what about a new --something that conflicts
with either --exclude or --decorate-reflog? Should we simply catch
such combinations and error out (which may be a bit more complicated
than this patch, or maybe not)?
-- 
Duy

Re: [PATCH 2/5] revision.c: group ref selection options together

[Duy Nguyen]

On Thu, Jan 26, 2017 at 3:50 AM, Jeff King  wrote:
> On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> These options have on thing in common: when specified right after
>> --exclude, they will de-select refs instead of selecting them by
>> default.
>>
>> This change makes it possible to introduce new options that use these
>> options in the same way as --exclude. Such an option would just
>> implement something like handle_refs_pseudo_opt().
>>
>> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
>> that similar functions like handle_refs_pseudo_opt() are forced to
>> handle all ref selector options, not skipping some by mistake, which may
>> revert the option back to default behavior (rev selection).
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  revision.c | 134 
>> +
>>  1 file changed, 100 insertions(+), 34 deletions(-)
>
> Hmm. I see what you're trying to do here, and abstract the repeated
> bits. But I'm not sure the line-count reflects a real simplification.
> Everything ends up converted to an enum, and then that enum just expands
> to similar C code.

It's not simplification, but hopefully for better maintainability. This

if (strcmp(arg, "--remotes")) {
   if (preceded_by_exclide())
  does_something();
   else if (preceded_by_decorate())
  does_another()
} else if (strcmp(arg, "--branches")) {
   if (preceded_by_exclide())
  does_something();
   else if (preceded_by_decorate())
  does_another()
}

starts to look ugly especially when the third "preceded_by_" comes
into picture. Putting all "does_something" in one group and
"does_another" in another, I think, gives us a better view how ref
selection is handled for a specific operation like --exclude or
--decorate-ref.

> I kind of expected that clear_ref_exclusion() would just become a more
> abstract clear_ref_selection(). For now it would clear exclusions, and
> then later learn to clear the decoration flags.

It may go that way, depending on how we handle these options for
decorate-reflog. The current load_ref_decorations() is not really
suited for fine-grained ref selection yet.
--
Duy

Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)

[Lars Schneider]

> On 25 Jan 2017, at 23:51, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> I guess the way to dig would be to add a test that looks at the output
>> of "type mv" or something, push it to a Travis-hooked branch, and then
>> wait for the output
> 
> Sounds tempting ;-)

Well, I tried that:

mv is /bin/mv

... and "/bin/mv" is exactly the version that I have on my machine.

The difference between Travis and my machine is that I changed the 
default shell to ZSH with a few plugins [1]. If I run the test with 
plain BASH on my Mac then I can reproduce the test failure. Therefore,
we might want to adjust the commit message if anyone else can reproduce
the problem on a Mac. 

I can even reproduce the failure if I run the test with plain ZSH. 
However, I can't find a plugin that defines an alias for "mv". Puzzled...

- Lars

[1] https://github.com/robbyrussell/oh-my-zsh

Re: [PATCH 2/5] revision.c: group ref selection options together

[Duy Nguyen]

On Thu, Jan 26, 2017 at 4:11 AM, Junio C Hamano  wrote:
>  * I am expecting that the new one yet to be introduced will not
>share the huge "switch (selector)" part, but does its own things
>in a separate function with a similar structure.  The only thing
>common between these two functions would be the structure
>(i.e. it has a big "switch(selector)" that does different things
>depending on REF_SELECT_*) and a call to clear_* function.

Yep. The "new one" is demonstrated in 5/5.

>If we were to add a new kind of REF_SELECT_* (say
>REF_SELECT_NOTES just for the sake of being concrete), what
>changes will be needed to the code if the addition of "use reflog
>from this class of refs for decoration" feature was done with or
>without this step?  I have a suspicion that the change will be
>simpler without this step.

The switch/case is to deal with new REF_SELECT_* (at least it's how I
imagine it). What I was worried about was, when a user adds
--select-notes, they may not be aware that it's in the same
all/branches/tags/remotes group that's supposed to work with
--decorate-reflog as well, and as a result "--decorate-reflog
--select-notes" is the same as "--select-notes".

With the switch/case, when you add a new enum item, at the least the
compiler should warn about unhandled cases. And we can have a new
"case REF_SELECT_NOTES:" for both --exclude and --decorate-reflog.
Without the switch/case, I guess it's still possible to do something
like

if (!strcmp(arg, "--select-notes")) {
if (preceded_by_exclude())
does_one_thing();
else if (preceded_by_decorate_reflog())
   does_another_thing();
}

It's probably easier to maintain though, if all
decorate-reflog-related things are grouped together, rather than
spread out per option like the above.
-- 
Duy