Re: [PATCH v3 0/9] Fix the early config

[Jeff King]

On Fri, Mar 03, 2017 at 06:31:55PM +0100, Johannes Schindelin wrote:

> Interdiff vs v2:
> [...]
>  + * When we are not about to create a repository ourselves (init or
>  + * clone) and when no .git/ directory was set up yet (in which case
>  + * git_config_with_options() would already have picked up the
>  + * repository config), we ask discover_git_directory() to figure out
>  + * whether there is any repository config we should use (but unlike
>  + * setup_git_directory_gently(), no global state is changed, most
>  + * notably, the current working directory is still the same after
>  + * the call).
>*/
>  -if (!startup_info->creating_repository && !have_git_dir() &&
>  -discover_git_directory(&buf)) {
>  +if (!have_git_dir() && discover_git_directory(&buf)) {

I think this "when we are not about to..." part of the comment is no
longer true, given the second part of the hunk.

>  @@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char 
> *gitdir,
>   if (offset == cwd->len)
>   return NULL;
>   
>  -/* Make "offset" point to past the '/', and add a '/' at the end */
>  -offset++;
>  +/* Make "offset" point past the '/' (already the case for root dirs) */
>  +if (offset != offset_1st_component(cwd->buf))
>  +offset++;

Nice. I was worried we would have to have a hacky "well, sometimes we
don't add one here..." code, but using offset_1st_component says
exactly what we mean.

> +/* Find GIT_DIR without changing the working directory or other global state 
> */
>  extern const char *discover_git_directory(struct strbuf *gitdir);

The parts that actually confused me were the parameters (mostly whether
gitdir was a directory to start looking in, or an output parameter). So
maybe:

  /*
   * Find GIT_DIR of the repository that contains the current working
   * directory, without changing the working directory or other global
   * state. The result is appended to gitdir. The return value is NULL
   * if no repository was found, or gitdir->buf otherwise.
   */

This looks good to me aside from those few comment nits. I'm still not
sure I understand how ceil_offset works in setup_git_directory_gently_1(),
but I don't think your patch actually changed it. I can live with my
confusion.

-Peff

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King]

On Sat, Mar 04, 2017 at 06:55:48AM +, Eric Wong wrote:

> Jeff King  wrote:
> > The warning itself:
> > 
> > > + warning("alternate disabled by http.followRedirects!=true: %s",
> > 
> > feels like it could use some whitespace around the "!=", but maybe
> > that's just me.
> 
> Yeah, I kinda wanted to emulate the command-line syntax.
> 
> Maybe rewording it a bit and showing how to enable it will
> make more sense:
> 
>   warning("alternate: %s", url);
>   warning(" may be enabled by -c http.followRedirects=true");

I kind of hoped people would look at the documentation for
followRedirects before blindly enabling it. Though I guess the
documentation doesn't really explain the possible security implications,
so maybe it doesn't matter (and they're pretty subtle anyway).

-Peff

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Eric Wong]

Jeff King  wrote:
> The warning itself:
> 
> > +   warning("alternate disabled by http.followRedirects!=true: %s",
> 
> feels like it could use some whitespace around the "!=", but maybe
> that's just me.

Yeah, I kinda wanted to emulate the command-line syntax.

Maybe rewording it a bit and showing how to enable it will
make more sense:

warning("alternate: %s", url);
warning(" may be enabled by -c http.followRedirects=true");

As well as keeping individual lines shorter and hopefully
easier-to-read.

Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

[Torsten Bögershausen]

On 2017-03-03 18:47, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
>> Understood, thanks for the explanation.
>>
>> quiet is not quite any more..
>>
>> Does the following fix help ?
>>
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,
>> unsigned int flags)
>> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
>> ? SAFE_CRLF_WARN
>> : safe_crlf);
>> +   if (size_only)
>> +   crlf_warn = SAFE_CRLF_FALSE;
> 
> If you were to go this route, it may be sufficient to change its
> initialization from WARN to FALSE _unconditionally_, because this
> function uses the convert_to_git() only to _show_ the differences by
> computing canonical form out of working tree contents, and the
> conversion is not done to _write_ into object database to create a
> new object.
Hm, since when (is it not used) ?

I thought that it is needed to support the safecrlf handling introduced in
21e5ad50fc5e7277c74cfbb3cf6502468e840f86
Author: Steffen Prohaska 
Date:   Wed Feb 6 12:25:58 2008 +0100

safecrlf: Add mechanism to warn about irreversible crlf conversions

-
The SAFE_CRLF_FAIL was converted into WARN here:
commit 5430bb283b478991a979437a79e10dcbb6f20e28
Author: Junio C Hamano 
Date:   Mon Jun 24 14:35:04 2013 -0700

diff: demote core.safecrlf=true to core.safecrlf=warn

Otherwise the user will not be able to start to guess where in the
contents in the working tree the offending unsafe CR lies.


My understanding is that we don't want to break the safecrlf feature,
but after applying

diff --git a/diff.c b/diff.c
index a628ac3a95..a05d88dd9f 100644
--- a/diff.c
+++ b/diff.c
@@ -2820,12 +2820,10 @@ int diff_populate_filespec(struct diff_filespec *s,
unsigned int flags)
int size_only = flags & CHECK_SIZE_ONLY;
int err = 0;
/*
-* demote FAIL to WARN to allow inspecting the situation
-* instead of refusing.
+* Don't use FAIL or WARN as this code is not called when _writing_
+* into object database to create a new object.
 */
-   enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
-   ? SAFE_CRLF_WARN
-   : safe_crlf);
+   enum safe_crlf crlf_warn = SAFE_CRLF_FALSE;


None of the test cases in t0020--t0027 fails or complain about missing warnings.
Does this all means that, looking back,  5430bb283b478991 could have been more
aggressive and could have used SAFE_CRLF_FALSE ?
And we can do this change now?

(If the answer is yes, we don't need to deal with the problem below)
> Having size_only here is not a sign of getting --quiet passed from
> the command line, by the way.
> 

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano  wrote:
>
> I only recently started looking at Travis logs, so I cannot tell if
> it is just a usual flake (e.g. some builds a few days ago seems to
> have failed due to not being able to check out the tree being
> tested, which I do not think is our fault) that we shouldn't worry
> about, or if it is a sign of a real problem.

Tonight's pushout also seems to stall the same way. Dscho's
unversioned one didn't exhibit the problem?
https://travis-ci.org/git/git/jobs/206811396

> Unrelated to linux-32, the same build has hard failure with Apple
> clang in t0021 with the rot13-filter.pl thing, by the way.

This one may be a Heisenbug which may indicate some raciness in the
clean/smudge filter protocol.
The latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to
have passed OK.

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King]

On Fri, Mar 03, 2017 at 10:13:14PM -0500, Jeff King wrote:

> > -   if (http_follow_config != HTTP_FOLLOW_ALWAYS)
> > -   return;
> > -
> 
> I was surprised from the description to see not just the addition of a
> warning, but a movement of the enforcement code.
> 
> I think it's necessary because the original did not bother even fetching
> http-alternates if we were not going to respect it. Whereas the new code
> will fetch and parse it, and warn only if we actually found something in
> it. Which seems reasonable.

One side effect of this is that it exposes[1] the http-alternates
parsing code to the server's input, even if we aren't planning on using
the result. That code is not very well audited. Just looking at the
context from your patches, I noticed one obvious memory access problem.
The fix is below.

-Peff

[1] Obviously this same code was exposed prior to the redirect-limiting
patches, so it's not like there aren't tons of older clients that
exhibit the same behavior. But IMHO one of the beneficial side
effects of the redirect-limiting was that it avoided this largely
unused and untested code entirely.

-- >8 --
Subject: http-walker: fix buffer underflow processing remote alternates

If we parse a remote alternates (or http-alternates) file,
we expect relative lines like:

  ../../foo.git/objects

which we convert into "$URL/../foo.git/" (and then use that
as a base for fetching more objects).

But if the remote feeds us nonsense like just:

  ../

we will try to blindly strip the last 7 characters, assuming
they contain the string "objects". Since we don't _have_ 7
characters at all, this results in feeding a small negative
value to strbuf_add(), which converts it to a size_t,
resulting in a big positive value. This should consistently
fail (since we can't generally allocate the max size_t minus
7 bytes), so there shouldn't be any security implications
(and even if we did allocate, we'd just copy in gigabytes of
garbage, not overflow a buffer).

Let's fix this by using strbuf_strip_suffix() to drop the
characters we want. As a bonus this lets us handle names
that do not end in "objects" (all git repos do, but there is
nothing to say that an alternate object store needs to be a
git repo).

And while we're here, we can add a few other parsing
niceties, like dropping trailing whitespace, and handling
names that do not end in "/".

Signed-off-by: Jeff King 
---
 http-walker.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7..d62ca8953 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -296,11 +296,13 @@ static void process_alternates_response(void 
*callback_data)
okay = 1;
}
}
-   /* skip "objects\n" at end */
if (okay) {
struct strbuf target = STRBUF_INIT;
strbuf_add(&target, base, serverlen);
-   strbuf_add(&target, data + i, posn - i - 7);
+   strbuf_add(&target, data + i, posn - i);
+   strbuf_rtrim(&target);
+   strbuf_strip_suffix(&target, "objects");
+   strbuf_complete(&target, '/');
 
if (is_alternate_allowed(target.buf)) {
warning("adding alternate object store: 
%s",
-- 
2.12.0.404.g442e75cca

Re: [PATCH] http: release strbuf on disabled alternates

[Jeff King]

On Sat, Mar 04, 2017 at 01:50:16AM +, Eric Wong wrote:

> This likely has no real-world impact on memory usage,
> but it is cleaner for future readers.

Yep, this looks good.

-Peff

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King]

On Sat, Mar 04, 2017 at 01:35:04AM +, Eric Wong wrote:

> It is disconcerting for users to not notice the behavior
> change in handling alternates from commit cb4d2d35c4622ec2
> ("http: treat http-alternates like redirects")
> 
> Give the user a hint about the config option so they can
> see the URL and decide whether or not they want to enable
> http.followRedirects in their config.

Yeah, I agree it makes sense to notify the user.

> diff --git a/http-walker.c b/http-walker.c
> index b34b6ace7..626badfe6 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)
>   };
>   int i;
>  
> + if (http_follow_config != HTTP_FOLLOW_ALWAYS) {
> + warning("alternate disabled by http.followRedirects!=true: %s",
> + url);
> + return 0;
> + }
> +
>   for (i = 0; i < ARRAY_SIZE(protocols); i++) {
>   const char *end;
>   if (skip_prefix(url, protocols[i], &end) &&
> @@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const 
> char *base)
>   struct alternates_request alt_req;
>   struct walker_data *cdata = walker->data;
>  
> - if (http_follow_config != HTTP_FOLLOW_ALWAYS)
> - return;
> -

I was surprised from the description to see not just the addition of a
warning, but a movement of the enforcement code.

I think it's necessary because the original did not bother even fetching
http-alternates if we were not going to respect it. Whereas the new code
will fetch and parse it, and warn only if we actually found something in
it. Which seems reasonable.

The warning itself:

> + warning("alternate disabled by http.followRedirects!=true: %s",

feels like it could use some whitespace around the "!=", but maybe
that's just me.

-Peff

[PATCH] http: release strbuf on disabled alternates

[Eric Wong]

This likely has no real-world impact on memory usage,
but it is cleaner for future readers.

Fixes: abcbdc03895f ("http: respect protocol.*.allow=user for http-alternates")
Signed-off-by: Eric Wong 
---
 http-walker.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7c..2c85316112 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -314,6 +314,8 @@ static void process_alternates_response(void *callback_data)
while (tail->next != NULL)
tail = tail->next;
tail->next = newalt;
+   } else {
+   strbuf_release(&target);
}
}
}
-- 
EW

RFC: Another proposed hash function transition plan

[Jonathan Nieder]

Hi,

This past week we came up with this idea for what a transition to a new
hash function for Git would look like.  I'd be interested in your
thoughts (especially if you can make them as comments on the document,
which makes it easier to address them and update the document).

This document is still in flux but I thought it best to send it out
early to start getting feedback.

We tried to incorporate some thoughts from the thread
http://public-inbox.org/git/20170223164306.spg2avxzukkgg...@kitenet.net
but it is a little long so it is easy to imagine we've missed
some things already discussed there.

You can use the doc URL

 https://goo.gl/gh2Mzc

to view the latest version and comment.

Thoughts welcome, as always.

Git hash function transition

Status: Draft
Last Updated: 2017-03-03

Objective
-
Migrate Git from SHA-1 to a stronger hash function.

Background
--
The Git version control system can be thought of as a content
addressable filesystem. It uses the SHA-1 hash function to name
content. For example, files, trees, commits are referred to by hash
values unlike in other traditional version control systems where files
or versions are referred to via sequential numbers. The use of a hash
function to address its content delivers a few advantages:

* Integrity checking is easy. Bit flips, for example, are easily
  detected, as the hash of corrupted content does not match its name.
  Lookup of objects is fast.

Using a cryptographically secure hash function brings additional advantages:

* Object names can be signed and third parties can trust the hash to
  address the signed object and all objects it references.
* Communication using Git protocol and out of band communication
  methods have a short reliable string that can be used to reliably
  address stored content.

Over time some flaws in SHA-1 have been discovered by security
researchers. https://shattered.io demonstrated a practical SHA-1 hash
collision. As a result, SHA-1 cannot be considered cryptographically
secure any more. This impacts the communication of hash values because
we cannot trust that a given hash value represents the known good
version of content that the speaker intended.

SHA-1 still possesses the other properties such as fast object lookup
and safe error checking, but other hash functions are equally suitable
that are believed to be cryptographically secure.

Goals
-
1. The transition to SHA256 can be done one local repository at a time.
   a. Requiring no action by any other party.
   b. A SHA256 repository can communicate with SHA-1 Git servers and
  clients (push/fetch).
   c. Users can use SHA-1 and SHA256 identifiers for objects
  interchangeably.
   d. New signed objects make use of a stronger hash function than
  SHA-1 for their security guarantees.
2. Allow a complete transition away from SHA-1.
   a. Local metadata for SHA-1 compatibility can be dropped in a
  repository if compatibility with SHA-1 is no longer needed.
3. Maintainability throughout the process.
   a. The object format is kept simple and consistent.
   b. Creation of a generalized repository conversion tool.

Non-Goals
-
1. Add SHA256 support to Git protocol. This is valuable and the
   logical next step but it is out of scope for this initial design.
2. Transparently improving the security of existing SHA-1 signed
   objects.
3. Intermixing objects using multiple hash functions in a single
   repository.
4. Taking the opportunity to fix other bugs in git's formats and
   protocols.
5. Shallow clones and fetches into a SHA256 repository. (This will
   change when we add SHA256 support to Git protocol.)
6. Skip fetching some submodules of a project into a SHA256
   repository. (This also depends on SHA256 support in Git protocol.)

Overview

We introduce a new repository format extension `sha256`. Repositories
with this extension enabled use SHA256 instead of SHA-1 to name their
objects. This affects both object names and object content --- both
the names of objects and all references to other objects within an
object are switched to the new hash function.

sha256 repositories cannot be read by older versions of Git.

Alongside the packfile, a sha256 stores a bidirectional mapping
between sha256 and sha1 object names. The mapping is generated locally
and can be verified using "git fsck". Object lookups use this mapping
to allow naming objects using either their sha1 and sha256 names
interchangeably.

"git cat-file" and "git hash-object" gain options to display a sha256
object in its sha1 form and write a sha256 object given its sha1 form.
This requires all objects referenced by that object to be present in
the object database so that they can be named using the appropriate
name (using the bidirectional hash mapping).

Fetches from a SHA-1 based server convert the fetched objects into
sha256 form and record the mapping in the bidirectional mapping table
(see below for details)

[PATCH] http: inform about alternates-as-redirects behavior

[Eric Wong]

It is disconcerting for users to not notice the behavior
change in handling alternates from commit cb4d2d35c4622ec2
("http: treat http-alternates like redirects")

Give the user a hint about the config option so they can
see the URL and decide whether or not they want to enable
http.followRedirects in their config.

Signed-off-by: Eric Wong 
---
 http-walker.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7..626badfe6 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)
};
int i;
 
+   if (http_follow_config != HTTP_FOLLOW_ALWAYS) {
+   warning("alternate disabled by http.followRedirects!=true: %s",
+   url);
+   return 0;
+   }
+
for (i = 0; i < ARRAY_SIZE(protocols); i++) {
const char *end;
if (skip_prefix(url, protocols[i], &end) &&
@@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const 
char *base)
struct alternates_request alt_req;
struct walker_data *cdata = walker->data;
 
-   if (http_follow_config != HTTP_FOLLOW_ALWAYS)
-   return;
-
/*
 * If another request has already started fetching alternates,
 * wait for them to arrive and return to processing this request's
-- 
EW

RE: [PATCH] Put sha1dc on a diet

[Dan Shumow]

From: Junio C Hamano [mailto:gits...@pobox.com] 

> As you and Marc seemed to be still working on speeding up, such a 
> customization work to fully adjust your code to our codebase was premature, 
> so I tentatively queued what we saw on the list as-is on our 'pu' branch so 
> that people can have a reference point.  Which unfortunately solicited a 
> premature reaction by Johannes.  Please do not worry too much about the 
> comment.

> But if you are willing to help us by getting involved in the "customization" 
> part, too, that would be a very welcome news to us.
>In that case, "welcome to the Git development community" ;-)

> So,... from my point of view, we are OK either way.  It is OK if you are a 
> third-party upstream that is not particularly interested in Git project's 
> specific requirement.  We surely would be happier if you and Marc, the 
> upstream authors of the code in question, also act as participants in the Git 
> development community.

> Either way, thanks for your great help.

You are very welcome.  Thank you for the warm welcome.  As it turns out, Marc 
and I are working on the simplifications / removal of c99 and performance 
upstream in our GitHub repo.  I am happy to help for any GitHub specific 
customizations that are needed as well.  But for now, lets see if we can get 
you everything you want upstream -- I think that's the most simple.

Thanks again,
Dan

Re: bisect-helper: we do not bisect --objects

[Junio C Hamano]

"Philip Oakley"  writes:

> Bikeshedding: If the given boundary is a tag, it could be tagging a
> blob or tree rather than a commit. Would that be a scenario that
> reaches this part of the code?

I do not think it is relevant.

Bisection is an operation over a "bisectable" commit DAG, where
commits can be partitioned into "new" (aka "bad") and "old" (aka
"good") camp, all descendants of a "new" commit are all "new", all
ancestors of an "old" commit are all "old".  Think of the "new"-ness
as a 100% inheritable disease that happens spontaneously and without
cure.  Once you are infected with "new"-ness, all your descendants
are forever "new".  If you know you are free of the "new"-ness, you
know that all your ancestors are not with the "new"-ness, either.

The goal of the operation is to find a "new" commit whose parents
are all "old".

The bisectability of the commit DAG is what allows you to say "this
commit is new" to a commit somewhere in the middle of the history
and avoid having to test any descendants, as they all inherit the
"new"-ness from it (similarly when you have one commit that is
"old", you do not have to test any ancestor), thereby reducing the
number of test from N (all commits in good..bad range) to log(N).

There is no room for a tree or a blob to participate in this graph
partitioning problem.  A "bad" tree that is "new" cannot infect its
children with the "new"-ness and a "good" tree cannot guarantee the
lack of "new"-ness of its parents, because a tree or a blob does not
have parent or child commits.

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

Junio C Hamano  writes:

> I see he did v2 which you Acked in a different thread.  Will replace
> what's been on 'pu' and running with Travis the past few days with
> it.  Let's wait for one or more Travis cycles and then merge it to
> 'next'.

https://travis-ci.org/git/git/jobs/207517043 is an output from 'pu'
with the v2 patch; it seems to have fell into a funny state.  The
output ends like so:

No output has been received in the last 10m0s, this potentially
indicates a stalled build or something wrong with the build itself.

I only recently started looking at Travis logs, so I cannot tell if
it is just a usual flake (e.g. some builds a few days ago seems to
have failed due to not being able to check out the tree being
tested, which I do not think is our fault) that we shouldn't worry
about, or if it is a sign of a real problem.

Unrelated to linux-32, the same build has hard failure with Apple
clang in t0021 with the rot13-filter.pl thing, by the way.

Server-side hooks on non-bare repository

[Mike Lewis]

Hello,

I’m having some issues with using server-side hooks when pushing to a non-bare 
repository. In my git config, I have `receive.denyCurrentBranch` set to 
`updateInstead`, which behaves as expected, and updates the current working 
tree when the current branch is pushed to. However, attempting to process those 
changes with pre-receive and post-receive hooks results in some unexpected 
behavior regarding the current working directory of the scripts and using git 
commands. I’ve tested these issues using both git 2.11 and 2.12 on various 
systems (macOS and CentOS), and get the same behavior each time.

Essentially, my problem boils down to two things:

1. When using a non-bare repository, I would expect the the working directory 
of the hook to be the root directory of the working tree, as this mirrors the 
behavior of “client”-side hooks like pre-commit. Instead, the working directory 
is set to the .git directory. That in and of itself is not a huge deal, but it 
leads into #2:

2. While running the hooks, git treats the repository as being bare, regardless 
of whether it actually is. For instance, changing the working directory of the 
scripts to the actual root of the working tree and attempting to run any git 
commands (for instance, `git rev-parse --abbrev-ref HEAD` to get the current 
branch name) results in "fatal: Not a git repository: ‘.’” being returned to 
the client-side `git push` command. I’ve found a workaround to this, which is 
to explicitly set the “GIT_DIR” to the .git directory, and make sure that is 
passed to any external scripts as well. However, this is very unintuitive 
behavior, as the working tree is still there and either unmodified or done 
being updated, depending on which hook is being called. At the very least, this 
behavior should be in the documentation somewhere so that users can write their 
hooks accordingly.

TL;DR: it seems that server-side hooks don’t account for the repository being 
non-bare, which is no longer a valid assumption with the 
`receive.denyCurrentBranch=updateInstead` configuration value introduced in 
2.3.0.

Thanks, and I’d be happy to provide any other information that anyone needs to 
take a look at this.

Mike Lewis

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Jeff King  writes:

> Heh. I had no idea we had FREAD_READS_DIRECTORIES. I think Duy and I
> reinvented it in another thread. ;)

You two were not alone.  I was planning to reinvent it before I went
to bed last night and then found it already was there this morning ;-)

> I agree that may be worth setting on Linux (though note that we _do_
> ignore other stdio read errors in the rest of the code path, which may
> be something we want to address).

Yes, we need an error check on fopen() in git_config_from_file()
regardless.

Re: bisect-helper: we do not bisect --objects

[Philip Oakley]

From: "Junio C Hamano" 

Ever since "bisect--helper" was introduced in 1bf072e366
("bisect--helper: implement "git bisect--helper"", 2009-03-26),
after setting up the "rev-list $bad --not $good_ones" machinery, the
code somehow prepared to mark the trees and blobs at the good boundary
as uninteresting, only when --objects option was given.  This was kept
across a bit of refactoring done by 2ace9727be ("bisect: move common
bisect functionality to "bisect_common"", 2009-04-19) and survives
to this day.

However, "git bisect" does not care about tree/blob object
reachability at all---it purely works at the commit DAG level and
nobody passes (and nobody should pass) "--objects" option to the
underlying rev-list machinery.  Remove the dead code.

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

* Christian, do you recall what we were thinking when we added this
  mark_edges_uninteresting() call in this program?  If you don't,
  don't worry--this was done more than 8 years ago.  I am just
  being curious and also a bit being cautious in case I am missing
  something.



Bikeshedding: If the given boundary is a tag, it could be tagging a blob or 
tree rather than a commit. Would that be a scenario that reaches this part 
of the code? I thought I read previous comments that there is a case in the 
Linux tree.

--
Philip


  Thanks.

bisect.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/bisect.c b/bisect.c
index 30808cadf7..86c5929a23 100644
--- a/bisect.c
+++ b/bisect.c
@@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)
{
 if (prepare_revision_walk(revs))
 die("revision walk setup failed");
- if (revs->tree_objects)
- mark_edges_uninteresting(revs, NULL);
}

static void exit_if_skipped_commits(struct commit_list *tried,

What's cooking in git.git (Mar 2017, #02; Fri, 3)

[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]

* ew/markdown-url-in-readme (2017-03-01) 1 commit
  (merged to 'next' on 2017-03-03 at 3d35e3a991)
 + README: create HTTP/HTTPS links from URLs in Markdown

 Doc update.

 Will merge to 'master'.


* jk/add-i-patch-do-prompt (2017-03-02) 1 commit
 - add--interactive: fix missing file prompt for patch mode with "-i"

 The patch subcommand of "git add -i" was meant to have paths
 selection prompt just like other subcommand, unlike "git add -p"
 directly jumps to hunk selection.  Recently, this was broken and
 "add -i" lost the paths selection dialog, but it now has been
 fixed.

 Will merge to 'next'.


* ax/line-log-range-merge-fix (2017-03-03) 1 commit
 - line-log.c: prevent crash during union of too many ranges

 The code to parse "git log -L..." command line was buggy when there
 are many ranges specified with -L; overrun of the allocated buffer
 has been fixed.

 Will merge to 'next'.


* js/early-config (2017-03-03) 11 commits
 - t1309: test read_early_config()
 - read_early_config(): really discover .git/
 - read_early_config(): avoid .git/config hack when unneeded
 - setup: make read_early_config() reusable
 - setup: export the discover_git_directory() function
 - SQUASH??? ERROR: trailing statements should be on next line
 - setup_git_directory_1(): avoid changing global state
 - setup: prepare setup_discovered_git_directory() the root directory
 - SQUASH??? ERROR: trailing statements should be on next line
 - setup_git_directory(): use is_dir_sep() helper
 - t7006: replace dubious test

 The start-up sequence of "git" needs to figure out some configured
 settings before it finds and set itself up in the location of the
 repository and was quite messy due to its "chicken-and-egg" nature.
 The code has been restructured.

 Will merge to 'next' after squashing niggle-fixes in.


* jt/perf-updates (2017-03-03) 3 commits
 - t/perf: add fallback for pre-bin-wrappers versions of git
 - t/perf: use $MODERN_GIT for all repo-copying steps
 - t/perf: export variable used in other blocks

 The t/perf performance test suite was not prepared to test not so
 old versions of Git, but now it covers versions of Git that are not
 so ancient.

 Will merge to 'next'.


* ss/remote-bzr-hg-placeholder-wo-python (2017-03-03) 1 commit
 - contrib: git-remote-{bzr,hg} placeholders don't need Python

 There is no need for Python only to give a few messages to the
 standard error stream, but we somehow did.

 Will merge to 'next'.

--
[Stalled]

* nd/worktree-move (2017-01-27) 7 commits
 . fixup! worktree move: new command
 . worktree remove: new command
 . worktree move: refuse to move worktrees with submodules
 . worktree move: accept destination as directory
 . worktree move: new command
 . worktree.c: add update_worktree_location()
 . worktree.c: add validate_worktree()

 "git worktree" learned move and remove subcommands.

 Tentatively ejected as it seems to break 'pu' when merged.


* pb/bisect (2017-02-18) 28 commits
 - fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
 - bisect--helper: remove the dequote in bisect_start()
 - bisect--helper: retire `--bisect-auto-next` subcommand
 - bisect--helper: retire `--bisect-autostart` subcommand
 - bisect--helper: retire `--bisect-write` subcommand
 - bisect--helper: `bisect_replay` shell function in C
 - bisect--helper: `bisect_log` shell function in C
 - bisect--helper: retire `--write-terms` subcommand
 - bisect--helper: retire `--check-expected-revs` subcommand
 - bisect--helper: `bisect_state` & `bisect_head` shell function in C
 - bisect--helper: `bisect_autostart` shell function in C
 - bisect--helper: retire `--next-all` subcommand
 - bisect--helper: retire `--bisect-clean-state` subcommand
 - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
 - t6030: no cleanup with bad merge base
 - bisect--helper: `bisect_start` shell function partially in C
 - bisect--helper: `get_terms` & `bisect_terms` shell function in C
 - bisect--helper: `bisect_next_check` & bisect_voc shell function in C
 - bisect--helper: `check_and_set_terms` shell function in C
 - bisect--helper: `bisect_write` shell function in C
 - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
 - bisect--helper: `bisect_reset` shell function in C
 - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
 - t6030: explicitly test for bisection cleanup
 - bisect--helper: `bisect_clean_state`

Re: bisect-helper: we do not bisect --objects

[Christian Couder]

On Fri, Mar 3, 2017 at 5:16 PM, Junio C Hamano  wrote:
> Ever since "bisect--helper" was introduced in 1bf072e366
> ("bisect--helper: implement "git bisect--helper"", 2009-03-26),
> after setting up the "rev-list $bad --not $good_ones" machinery, the
> code somehow prepared to mark the trees and blobs at the good boundary
> as uninteresting, only when --objects option was given.  This was kept
> across a bit of refactoring done by 2ace9727be ("bisect: move common
> bisect functionality to "bisect_common"", 2009-04-19) and survives
> to this day.
>
> However, "git bisect" does not care about tree/blob object
> reachability at all---it purely works at the commit DAG level and
> nobody passes (and nobody should pass) "--objects" option to the
> underlying rev-list machinery.  Remove the dead code.
>
> Signed-off-by: Junio C Hamano 
> ---
>
>  * Christian, do you recall what we were thinking when we added this
>mark_edges_uninteresting() call in this program?  If you don't,
>don't worry--this was done more than 8 years ago.  I am just
>being curious and also a bit being cautious in case I am missing
>something.

I think I just copy pasted the code from cmd_rev_list() in
builtin-rev-list.c and probably didn't realize that revs->tree_objects
would always be false.

Thanks for spotting this and removing the dead code.

Re: [PATCH] t/perf: export variable used in other blocks

[Jeff King]

On Fri, Mar 03, 2017 at 10:51:57AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Thanks, this is obviously the right thing to do, and the mistake is mine
> > from ea97002fc (t/perf: time rev-list with UNINTERESTING commits,
> > 2014-01-20). This is not the first time I've been confused by missing
> > variables in t/perf scripts, since it behaves differently than the
> > normal test suite. I wonder if we should turn on "set -a" during t/perf
> > setup snippets. That's a bit of a blunt tool, but I suspect it would
> > just be easier to work with.
> 
> I wonder if we can make t/perf to behave more similar to the normal
> test suite to eliminate the need for this exporting, by the way.
> t/perf/README does not say anything more than "for lack of better
> options" throughout its history, which does not help very much.

The problem is that it has to run each block multiple times under an
external "time" program. If you assume that the shell running the perf
suite is bash, you can do:

  i=0
  foo() { echo $i; i=$(($i+1)); }
  for run in 1 2 3; do time foo; done

which shows that you can get repeated timings from a shell function.
That shell function would run the actual test snippet, and would have to
do some redirect trickery to split the "time" results from the stderr of
the underlying test. And parse bash's time output.

But those are all do-able things. The main thing is throwing out a
dependency on an external "time" for "bash". That may not be a big loss,
though.

-Peff

Re: [PATCH v7 0/3] Conditional config include

[Jeff King]

On Fri, Mar 03, 2017 at 10:24:05AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > For those following on the mailing list, there is some discussion at:
> >
> >   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
> >
> > I think that is mostly focused around another failing in the
> > error-handling of the config code, and that does not need to be
> > addressed by this series (though of course I'd welcome any fixes).
> 
> Thanks.  Without a message like this, the list may have never known
> about the discussion taken elsewhere.  I'd appreciate such a report
> to appear on list the next time much earlier ;-)
> 
> When built with FREAD_READS_DIRECTORIES=Yes on Linux, the error in
> the test can easily reproduce.

Heh. I had no idea we had FREAD_READS_DIRECTORIES. I think Duy and I
reinvented it in another thread. ;)

I agree that may be worth setting on Linux (though note that we _do_
ignore other stdio read errors in the rest of the code path, which may
be something we want to address).

-Peff

Re: [PATCH v7 3/3] config: add conditional include

[Jeff King]

On Fri, Mar 03, 2017 at 11:05:20AM -0800, Junio C Hamano wrote:

> I am not sure about "obviousness", but I agree that we do not know
> that "conditional include" would be the only thing we want for the
> second level for "include.path" directive.  "include-if..path"
> is better for that reason.
> 
> I presume that you could still do
> 
>   [include "if:gitdir=$path"]
>   path = ...
> 
> i.e. design the second level to begin with a token that tells
> readers what it means (and assign "if:" token for "conditional
> include"), but I do not think it is worth it.

Yep, all true.

> I also imagine that
> 
>   [include]
>   condition = ...
>   path = ...
> 
> is easier to read and write by end-users, but it probably is not
> feasible because it requires too invasive change to the current code
> to teach it to grok such construct.

I am against that as it introduces a dependency in the presence and
ordering between two config variables, which can yield some surprises.

> Between "include-if" and "includeIf", if people find the latter not
> too ugly, I'd prefer to keep it the way Duy posted.  Because of the
> way "include.path" and "include-if..path" work, we can declare
> that they are not like ordinary configuration variable definition
> at all but are higher-level directives and that may be a sufficient
> justification to allow "-" in its name, though, if people find
> "includeIf" too ugly a name to live.

OK. I can live with includeIf.

-Peff

Re: SHA1 collisions found

[Jeff King]

On Fri, Mar 03, 2017 at 02:54:56PM +, Ian Jackson wrote:

> > What _does_ get rewritten is what's in your ref files, your pack .idx,
> > etc. Those are all sha256 (or whatever), and work as sha1's do now.
> 
> This all sounds very similar to my proposal.

Yeah, sorry I haven't reviewed that more carefully yet.

> > Looking up a sha1 reference from an old object just goes through the
> > extra level of indirection.
> 
> I don't understand why this is a level of indirection, rather than
> simply a retention of the existing SHA-1 object database (in parallel,
> but deprecated).

I just meant that we will need to know both the sha1 and the sha-256 of
every object (because the same blob, for example, may be referred to by
either name depending on whether it is a new or a historical tree).  So
one way to do that is to have a table mapping sha1 to sha-256, and then
a lookup of sha-1 goes through that before looking up the object content
on disk via sha-256.

But it may also be fine to just keep an index mapping sha1 directly to
object contents. That makes a sha1 lookup slightly faster, but it's more
expensive to do a sha-256 verification (you have to reverse-index the
object location in the sha-256 list).

(And my usual disclaimer: I am using sha-256 as a placeholder; I don't
have a strong opinion on the actual hash choice).

> Perhaps I have misunderstood what you mean by "graft".  I assume you
> don't mean info/grafts, because that is not conveyed by transfer
> protocols.

No, I just mean there will be a spot in the commit graph (or many spots,
potentially) where a "v2" commit using sha-256 references (by sha-256) a
"v1" commit that is full of sha-1s. I say "graft" only to differentiate
it from the idea of rewriting the content of those commit objects.

> Specifically, the parent reference in the first post-transition commit
> has to refer to something.  What does it refer to ?  The possibilities
> seem to be:
> 
>  1a. It names the SHA1 hash of an old commit object
>  1b. It names the BLAKE[1] hash of an old commit object, which
> object of course refers to its parents by SHA1.
> 
>  2. It names the BLAKE hash of a translated version of the
> old commit object.
> 
>  3. It doesn't name the parent, and the old history is not
> automatically transferred by clone and not readily accessible.
> 
> (1a) and (1b) are different variants of something like my mixed hash
> proposal.  Old and new hashes live side by side.

Thanks for laying out those options. My proposals have definitely been
in the (1b) camp.

I think (1a) is not so bad; it just bumps the transition point one
commit higher. But I think the "rules" for which hash to expect are
easier if they depend on which object the _pointer_ is using, rather
than the _pointee_.

> (2) involves rewriting all of the old history, to recursively generate
> new objects (with BLAKE names, and which in turn refer to other
> rewritten old objects by BLAKE names).  The first time a particular
> tree needs to look up an object by a BLAKE name, it needs to run a
> conversion its own entire existing history.
> 
> For (2) there would have to be some kind of mapping table in every
> tree, which allowed object names to be maped in both directions.  The
> object content translation would have to be reversible, so that the
> actual pre-translation objects would not need to be stored; rather
> they would be reconstructed from the post-translation objects, when
> someone asks for a pre-translation object.  In principle it would be
> possible to convert future BLAKE commits to SHA-1 ones, again by
> recursive rewriting.

Hmm. I had initially rejected this as being pretty nasty for accessing
the old format on the fly. But as long as you keep the bidirectional
mapping from the initial expensive conversion, in most cases you only
need to convert at most a single object. E.g., the two cases that are
really interesting are:

  - I have an old commit sha1 and I want to run "git show" on it. We
convert the sha1 to the BLAKE name, and then just show the BLAKE
contents as usual.

  - I want to verify a commit or tag signature. We need the original
bytes for this. So we convert the sha1 to the BLAKE name to get the
BLAKE'd contents. Then we rewrite only the _single_ object,
converting any of its internal BLAKE references back to sha1s via
the mapping.

That's more appealing than I had originally given it credit for, because
most of the other code just happily uses the BLAKE name internally. Even
a diff across the conversion boundary works at full speed, because it's
using the same names consistently.

The big downside is that the mapping is more expensive to generate than
a 1b-style mapping. In 1b, you just compute both hashes over all
incoming objects and store the sha1 in a side lookup table. But here you
actually need to _rewrite_ each object to come up with its
alternate-universe sha1. And you need to do it in reverse-graph order,
not the arbitrary order 

Re: SHA1 collisions found

[Stefan Beller]

On Thu, Mar 2, 2017 at 11:55 AM, Linus Torvalds
 wrote:
>
> So: What do you think about the concept?
>
>Linus

One of the things I like about working on Git is its pretty
high standard of testing. So we would need to come up with
good methods of testing this, e.g. when
GIT_TEST_WITH_DEGENERATE_HASH is set, we'd use
an intentionally weak hashing function and have tests for
the collisions. These tests would need cover most of the
workflows that are currently performed with Git
(local creation, fetching, pushing). Writing all these
additional tests (which consists of creating colliding
objects/commits and then performing all these tests),
sounds about as much work as actually converting to
a new hash function. (First locally and then at a later
point in time all the networking related things).

I would not want to go that way.

Stefan

Re: [PATCH v3 0/9] Fix the early config

[Junio C Hamano]

Johannes Schindelin  writes:

> Notable notes:
>
> - In contrast to earlier versions, I no longer special-case init and
>   clone. Peff pointed out that this adds technical debt, and that we can
>   actually argue (for consistency's sake) that early config reads the
>   current repository config (if any) even for init and clone.
>
> - The read_early_config() function does not cache Git directory
>   discovery nor read values. If needed, this can be implemented later,
>   in a separate patch series.
>
> - The alias handling in git.c could possibly benefit from this work, but
>   again, this is a separate topic from the current patch series.

As Peff said in his review, I too find the result of this series a
more pleasant read than than original.

2/9 and corresponding 4/9 triggers "ERROR: trailing statements
should be on next line" from ../linux/scripts/checkpatch.pl because
of a line inherited from the original; I'll queue them with an
obvious style fix to work it around.

Thanks.

Re: git status --> Out of memory, realloc failed

[Carsten Fuchs]

Hi René,

Am 02.03.2017 um 21:04 schrieb René Scharfe:

When I use ulimit -v with lower and lower numbers I can provoke mmap failures on
bigger pack files, but not the realloc failures that you're seeing.  And your
packs should be only up to 20MB anyway (you can check that with "ls -l
.git/objects/pack/*.pack").


Yes, there are 9 of them, all about 20 MB in size.


So a shot in the dark: Do you have a lot of untracked files?  You could check by
cloning your repository locally (which copies only tracked contents).  Does "git
status" work on the clone?


I had about 40 modified or untracked files with a combined file size of 
about 5 MB.


Before I got your latest mail, I tried something else: Instead of 
connecting to the system with the repository via SSH as usual, I 
accessed the repository by mounting its file system via gvfs-mount onto 
my local desktop machine. Then I used my local, normally working Git to 
commit all modified and some untracked files (and deleted those that 
were left). Running `git status` and other commands took a very long 
time over a network connection of only 6 MBit/s, but eventually I got 
everything committed and pushed.


Quite in the spirit of your above words, getting rid of untracked files 
helped: Logging normally via SSH into the remote machine again, `git 
status` ran normally.


Only then did I see your mail and got another clone, where `git status` 
worked equally well – but then `git diff` failed in a similar manner, 
continuing the problems of `git status`:


(uiserver):p7715773:~/__TEST__$ git clone 
https://carst...@bitbucket.org/CarstenF/website-cafu.git

Cloning into 'website-cafu'...
Password for 'https://carst...@bitbucket.org':
remote: Counting objects: 44359, done.
remote: Compressing objects: 100% (28777/28777), done.
remote: Total 44359 (delta 19201), reused 38661 (delta 14143)
Receiving objects: 100% (44359/44359), 168.28 MiB | 20.95 MiB/s, done.
Resolving deltas: 100% (19201/19201), done.
Checking connectivity... done.
Checking out files: 100% (18524/18524), done.

(uiserver):p7715773:~/__TEST__$ cd website-cafu/

(uiserver):p7715773:~/__TEST__/website-cafu$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean

(uiserver):p7715773:~/__TEST__/website-cafu$ git diff
fatal: unable to create threaded lstat

(uiserver):p7715773:~/__TEST__/website-cafu$ git diff
fatal: Out of memory, realloc failed
fatal: Out of memory, realloc failed
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler


Another one, darker yet: Does "git config core.preloadIndex 0" help?


That's it!  :-)

Once that was set, both `git status` and `git diff` worked normally again!

I cannot test `git status` right now in the same situation as before, as 
I no longer have a working directory that is "as dirty" as before, but 
the above setting seems to fix the problem promptly and thoroughly!


Many thanks for your help!   :-)

Best regards,
Carsten

Re: [PATCH 3/3] Remove outdated info in difftool manpage

[Denton Liu]

On Fri, Mar 03, 2017 at 04:46:36PM +0100, Johannes Schindelin wrote:
> Hi Denton (or should I address you as Liu?),
Denton is fine, thanks.
> 
> On Fri, 3 Mar 2017, Denton Liu wrote:
> 
> > When difftool was rewritten in C, it removed the capability to read
> > fallback configs from mergetool. This changes the documentation to
> > reflect this.
> 
> Thanks for pointing that out. But that is probably an oversight on my
> part, not an intentional change...
Do you expect to be submitting a patch for this soon? Or, if not, would
it be fine if I went ahead and did it?
> 
> Ciao,
> Johannes

Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool

[Denton Liu]

On Fri, Mar 03, 2017 at 11:39:30AM -0800, Junio C Hamano wrote:
> Denton Liu  writes:
> 
> > In these tests, there are many situations where
> > 'echo "" | git mergetool' is used. This replaces all of those
> > occurrences with 'git mergetool -y' for simplicity and readability.
> 
> "-y where _possible_" is not necessarily a good thing to do in
> tests.  We do want to make sure that both "yes" from the input and
> "-y" from the command line work.  Changes to "-y" done only for the
> tests that are not about accepting the interactive input from the
> end-user is a very good idea, but "replaces all of those" makes me
> worried.
The 'custom mergetool' test case seems like a good place to introduce an
interactive test. Would the following patch to my patch work?

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index b6a419258..71624583c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -135,8 +135,8 @@ test_expect_success 'custom mergetool' '
test_expect_code 1 git merge master >/dev/null 2>&1 &&
git mergetool -y both >/dev/null 2>&1 &&
git mergetool -y file1 file1 &&
-   git mergetool -y file2 "spaced name" >/dev/null 2>&1 &&
-   git mergetool -y subdir/file3 >/dev/null 2>&1 &&
+   ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
+   ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&

> > -   ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
> > -   ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
> > -   ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
> > -   ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> > -   ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> > +   git mergetool file1 >/dev/null 2>&1 &&
> > +   git mergetool file2 >/dev/null 2>&1 &&
> > +   git mergetool "spaced name" >/dev/null 2>&1 &&
> > +   git mergetool both >/dev/null 2>&1 &&
> > +   git mergetool subdir/file3 >/dev/null 2>&1 &&
> 
> The reason for the lack of "-y" in the rewrite, in contrast to what
> was done in the previous hunk we saw, is not quite obvious.
> 
Sorry, it was my mistake. I had forgotten to replace the '-y'. I have
corrected this for a future revision.

Re: git status --> Out of memory, realloc failed

[Carsten Fuchs]

Hi Duy,

Am 2017-03-02 um 10:31 schrieb Duy Nguyen:

You could also try "git fast-export --anonymize" (read the doc first).


Eventually this was not needed, but thanks for letting me know about it! 
I've not been aware of this feature beforehand, learned something.


Best regards,
Carsten

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Duy Nguyen  writes:

> On Fri, Mar 03, 2017 at 01:33:29AM -0500, Jeff King wrote:
>> For those following on the mailing list, there is some discussion at:
>> 
>>   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>> 
>> I think that is mostly focused around another failing in the
>> error-handling of the config code, and that does not need to be
>> addressed by this series (though of course I'd welcome any fixes).
>> 
>> But there's a test failure that probably does need to be dealt with
>> before this graduates to 'next'.
>
> The patch to fix it is this
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f0cd2056ba..e833939320 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect 
> includes' '
>  
>  test_expect_success 'missing include files are ignored' '
>   cat >.gitconfig <<-\EOF &&
> - [include]path = foo
> + [include]path = non-existent
>   [test]value = yes
>   EOF
>   echo yes >expect &&
>
> Junio could you squash this in?
>

I said yes, but I won't, as the series already has SQUASH??? for
other things, so I'll just queue this at the tip.  I _might_ get
inclined to do the squashing and cleaning up myself later, but
please don't hold your breath.

Re: [PATCH 1/2] config: check if config path is a file before parsing it

[Junio C Hamano]

Jeff King  writes:

>a. actually check ferror() after getting EOF and report the read
>   error. That catches EISDIR, along with any other unexpected
> errors.

That is the most sensible, I would think (assuming that we really
get EISDIR instead of silent EOF).

>b. use an fopen wrapper that checks fstat(fileno(fh)) after the
>   open, and turns fopen(some_dir) into an error.

That's already an option with FREAD_READS_DIRECTORIES, I think.

>   2. It doesn't address the root problem for git_config_from_file(),
>  which is that it is quiet when fopen fails, even if the reason is
>  something interesting besides ENOENT. The caller can't check errno
>  because it doesn't know if fopen() failed, or if the config
>  callback returned an error.

Perhaps like this one as a starting point, with FREAD_READS_DIRECTORIES?

 config.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.c b/config.c
index 0dac0f4cb2..af8c01c8a3 100644
--- a/config.c
+++ b/config.c
@@ -1305,6 +1305,9 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
FILE *f;
 
f = fopen(filename, "r");
+   if (!f && errno != ENOENT)
+   die_errno("fopen('%s') failed", filename);
+
if (f) {
flockfile(f);
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, 
filename, f, data);

Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits

[Junio C Hamano]

Jeff King  writes:

> The change to filter-branch itself looks obviously correct. The only
> objectionable thing I noticed in the test additions is that the early
> ones should be marked test_expect_failure until the fix from 3/4 flips
> them to "success". Otherwise it breaks bisectability.

I'll squash in the necessary changes to flip between expect_success
and expect_failure in the appropriate places and re-queue on 'pu'.

Thanks.


commit 07cac4a5fdfeeb3c1c8385e222100d575a4460b0
Author: Junio C Hamano 
Date:   Fri Mar 3 11:41:36 2017 -0800

fixup! t7003: ensure --prune-empty can prune root commit

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 2dfe462501..45372a1cba 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -362,7 +362,7 @@ test_expect_success 'prune empty works even without 
index/tree filters' '
test_cmp expect actual
 '
 
-test_expect_success '--prune-empty is able to prune root commit' '
+test_expect_failure '--prune-empty is able to prune root commit' '
git rev-list branch-no-a >expect &&
git branch testing H &&
git filter-branch -f --prune-empty --index-filter "git update-index 
--remove A.t" testing &&


commit 562ed048c681686426ca95e0e550378b48aa4852
Author: Junio C Hamano 
Date:   Fri Mar 3 12:11:25 2017 -0800

fixup! t7003: ensure --prune-empty removes entire branch when applicable

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index a774a8e4b3..40526d1716 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -371,7 +371,7 @@ test_expect_failure '--prune-empty is able to prune root 
commit' '
test_cmp expect actual
 '
 
-test_expect_success '--prune-empty is able to prune entire branch' '
+test_expect_failure '--prune-empty is able to prune entire branch' '
git branch prune-entire B &&
git filter-branch -f --prune-empty --index-filter "git update-index 
--remove A.t B.t" prune-entire &&
test_path_is_missing .git/refs/heads/prune-entire &&


commit 520534c4035a13c54229dab0320e745d18635ef3
Author: Junio C Hamano 
Date:   Fri Mar 3 12:39:58 2017 -0800

fixup! filter-branch: fix --prune-empty on parentless commits

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 40526d1716..7cb60799be 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -362,7 +362,7 @@ test_expect_success 'prune empty works even without 
index/tree filters' '
test_cmp expect actual
 '
 
-test_expect_failure '--prune-empty is able to prune root commit' '
+test_expect_success '--prune-empty is able to prune root commit' '
git rev-list branch-no-a >expect &&
git branch testing H &&
git filter-branch -f --prune-empty --index-filter "git update-index 
--remove A.t" testing &&
@@ -371,7 +371,7 @@ test_expect_failure '--prune-empty is able to prune root 
commit' '
test_cmp expect actual
 '
 
-test_expect_failure '--prune-empty is able to prune entire branch' '
+test_expect_success '--prune-empty is able to prune entire branch' '
git branch prune-entire B &&
git filter-branch -f --prune-empty --index-filter "git update-index 
--remove A.t B.t" prune-entire &&
test_path_is_missing .git/refs/heads/prune-entire &&

Re: [PATCH 4/4] p7000: add test for filter-branch with --prune-empty

[Devin J. Pohly]

On Fri, Mar 03, 2017 at 02:56:05AM -0500, Jeff King wrote:
> On Thu, Feb 23, 2017 at 02:27:36AM -0600, Devin J. Pohly wrote:
> 
> > +test_perf 'noop prune-empty' '
> > +   git checkout --detach tip &&
> > +   git filter-branch -f --prune-empty base..HEAD
> > +'
> 
> I don't mind adding this, but of curiosity, does it show anything
> interesting?
> 
> -Peff

Nothing surprising; the overhead for the change was minimal.  I wasn't
sure what the practice is for adding unit/perf tests, so I erred on the
side of covering everything.

I will leave this as the last commit in the series so that it can be
dropped or merged as you see fit.

-- 
<><

Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits

[Devin J. Pohly]

On Fri, Mar 03, 2017 at 02:55:35AM -0500, Jeff King wrote:
> The only objectionable thing I noticed in the test additions is that
> the early ones should be marked test_expect_failure until the fix from
> 3/4 flips them to "success". Otherwise it breaks bisectability.
> 
> -Peff

Good point.  Will fix in a v2 set then.

-- 
<><

Re: [PATCH 1/2] config: check if config path is a file before parsing it

[Ramsay Jones]

On 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote:
> If a directory is given as a config file by accident, we keep open it
> as a file. The behavior of fopen() in this case seems to be
> undefined.
> 
> On Linux, we open a directory as a file ok, then get error (which we
> consider eof) on the first read. So the config parser sees this "file"
> as empty (i.e. valid config). All is well and we don't complain
> anything (but we should).
> 
> The situation is slighly different on Windows. I think fopen() returns
> NULL. And we get a very unhelpful message:
> 
> $ cat >abc < [include]
> path = /tmp/foo
> EOF
> $ mkdir /tmp/foo
> $ git config --includes --file=abc core.bare
> fatal: bad config line 3 in file abc
> 
> Opening a directory is wrong in the first place. Avoid it. If caught,
> print something better. With this patch, we have
> 
> $ git config --includes --file=abc core.bare
> error: '/tmp/foo' is not a file
> fatal: bad config line 3 in file abc
> 
> It's not perfect (line should be 2 instead of 3). But it's definitely
> improving.
> 
> The new test is only relevant on linux where we blindly open the
> directory and consider it an empty file. On Windows, the test should
> pass even without this patch.
> ---
>  abspath.c  | 7 +++
>  cache.h| 1 +
>  config.c   | 9 +
>  t/t1300-repo-config.sh | 5 +
>  4 files changed, 22 insertions(+)
> 
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2..373cc3abb2 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -11,6 +11,13 @@ int is_directory(const char *path)
>   return (!stat(path, &st) && S_ISDIR(st.st_mode));
>  }
>  
> +int is_not_file(const char *path)
> +{
> + struct stat st;
> +
> + return !stat(path, &st) && !S_ISREG(st.st_mode);
> +}
> +
>  /* removes the last path component from 'path' except if 'path' is root */
>  static void strip_last_component(struct strbuf *path)
>  {
> diff --git a/cache.h b/cache.h
> index 80b6372cf7..bdd1402ab9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path)
>   return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
>  }
>  int is_directory(const char *);
> +int is_not_file(const char *);
>  char *strbuf_realpath(struct strbuf *resolved, const char *path,
> int die_on_error);
>  const char *real_path(const char *path);
> diff --git a/config.c b/config.c
> index c6b874a7bf..c21c0ce433 100644
> --- a/config.c
> +++ b/config.c
> @@ -13,6 +13,7 @@
>  #include "hashmap.h"
>  #include "string-list.h"
>  #include "utf8.h"
> +#include "dir.h"

Something is a bit odd here - nothing in this commit (that I can
see, anyway) would require the addition of this include. Also, this
include is already there in the 'pu' branch, brought in by your
conditional include changes. So, ...

ATB,
Ramsay Jones

Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts

[Sebastian Schuberth]

On Fri, Mar 3, 2017 at 8:10 PM, Junio C Hamano  wrote:

>> Just a niggle:  This change moves the warning message from stderr to stdout.
>
> Right.  Here is what I'll queue.

Indeed, thanks for the note, and also Junio for fixing while queuing.

-- 
Sebastian Schuberth

Re: SHA1 collisions found

[Jeff King]

On Thu, Mar 02, 2017 at 11:55:45AM -0800, Linus Torvalds wrote:

> Anyway, I do have a suggestion for what the "object version" would be,
> but I'm not even going to mention it, because I want people to first
> think about the _concept_ and not the implementation.
> 
> So: What do you think about the concept?

I think it very much depends on what's in the "object version". :)

IMHO, we are best to consider sha1 "broken" and not count on any of its
bytes for cryptographic integrity. I know that's not really the case,
but it just makes reasoning about the whole thing simpler. So at that
point, it's pretty obvious that the "object version" is really just "an
integrity hash".

And that takes us full circle to earlier proposals over the years to do
something like this in the commit header:

  parent ...some sha1...
  parent-sha256 ...some sha256...

and ditto in tag headers, and trees obviously need to be hackily
extended as you described to carry the extra hash. And then internally
we continue to happily use sha1s, except you can check the
sha256-validity of any reference if you feel like it.

This is functionally equivalent to "just start using sha-256, but keep a
mapping of old sha1s to sha-256s to handle old references". The
advantage is that it makes the code part of the transition simpler. The
disadvantage is that you're effectively carrying a piece of that
sha1->sha256 mapping around in _every_ object.

And that means the same bits of mapping data are repeated over and over.
Git's pretty good at de-duplicating on the surface. So yeah, every tree
entry is now 256 bits larger, but deltas mean that we usually only end
up storing each entry a handful of times. But we still pay the price to
walk over the bytes every time we apply a delta, zlib inflate, parse the
tree, etc. The runtime cost of the transition is carried forward
forever, even for repositories that are willing to rewrite history, or
are created after the flag day.

So I dunno. Maybe I am missing something really clever about your
proposal. Reading the rest of the thread, it sounds like you had a
thought that we could get by with a very tiny object version, but the
hash-adding thing nixed that. If I'm still missing the point, please try
to sketch it out a bit more concretely, and I'll come back with my
thinking cap on.

-Peff

Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool

[Junio C Hamano]

Denton Liu  writes:

> In these tests, there are many situations where
> 'echo "" | git mergetool' is used. This replaces all of those
> occurrences with 'git mergetool -y' for simplicity and readability.

"-y where _possible_" is not necessarily a good thing to do in
tests.  We do want to make sure that both "yes" from the input and
"-y" from the command line work.  Changes to "-y" done only for the
tests that are not about accepting the interactive input from the
end-user is a very good idea, but "replaces all of those" makes me
worried.

> - ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool file1 file1 ) &&
> - ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> + git mergetool -y both >/dev/null 2>&1 &&
> + git mergetool -y file1 file1 &&
> + git mergetool -y file2 "spaced name" >/dev/null 2>&1 &&
> + git mergetool -y subdir/file3 >/dev/null 2>&1 &&

So these replace "the user interactively keeps typing " with
"-y", which sounds good.  These are obviously more about what the
code actually does.

> - ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> + git mergetool file1 >/dev/null 2>&1 &&
> + git mergetool file2 >/dev/null 2>&1 &&
> + git mergetool "spaced name" >/dev/null 2>&1 &&
> + git mergetool both >/dev/null 2>&1 &&
> + git mergetool subdir/file3 >/dev/null 2>&1 &&

The reason for the lack of "-y" in the rewrite, in contrast to what
was done in the previous hunk we saw, is not quite obvious.

> @@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' '
>   (
>   cd subdir &&
>   test_expect_code 1 git merge master >/dev/null 2>&1 &&
> - ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
> + git mergetool file3 >/dev/null 2>&1 &&

Likewise, and likewise for the remainder of the patch where the
updated command line does not say "-y".

Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts

[Junio C Hamano]

Marc Branchaud  writes:

> On 2017-03-03 05:57 AM, Sebastian Schuberth wrote:
>> It does not make sense for these placeholder scripts to depend on Python
>> just because the real scripts do. At the example of Git for Windows, we
>> would not even be able to see those warnings as it does not ship with
>> Python. So just use plain shell scripts instead.
>
> Just a niggle:  This change moves the warning message from stderr to stdout.

Right.  Here is what I'll queue.

-- >8 --
From: Sebastian Schuberth 
Date: Fri, 3 Mar 2017 10:57:46 +
Subject: [PATCH] contrib: git-remote-{bzr,hg} placeholders don't need Python

It does not make sense for these placeholder scripts to depend on Python
just because the real scripts do. At the example of Git for Windows, we
would not even be able to see those warnings as it does not ship with
Python. So just use plain shell scripts instead.

Signed-off-by: Sebastian Schuberth 
Reviewed-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 contrib/remote-helpers/git-remote-bzr | 16 +++-
 contrib/remote-helpers/git-remote-hg  | 16 +++-
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 712a1377e2..ccc4aea362 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh
 
-import sys
-
-sys.stderr.write('WARNING: git-remote-bzr is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit 
https://github.com/felipec/git-remote-bzr\n')
-
-sys.stderr.write('''WARNING:
+cat >&2 <<'EOT'
+WARNING: git-remote-bzr is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-bzr
+WARNING:
 WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING:   $ wget -O $HOME/bin/git-remote-bzr \\
+WARNING:   $ wget -O $HOME/bin/git-remote-bzr \
 WARNING: 
https://raw.github.com/felipec/git-remote-bzr/master/git-remote-bzr
 WARNING:   $ chmod +x $HOME/bin/git-remote-bzr
-''')
+EOT
diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 4255ad6312..dfda44f311 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh
 
-import sys
-
-sys.stderr.write('WARNING: git-remote-hg is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit 
https://github.com/felipec/git-remote-hg\n')
-
-sys.stderr.write('''WARNING:
+cat >&2 <<'EOT'
+WARNING: git-remote-hg is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-hg
+WARNING:
 WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING:   $ wget -O $HOME/bin/git-remote-hg \\
+WARNING:   $ wget -O $HOME/bin/git-remote-hg \
 WARNING: https://raw.github.com/felipec/git-remote-hg/master/git-remote-hg
 WARNING:   $ chmod +x $HOME/bin/git-remote-hg
-''')
+EOT
-- 
2.12.0-368-g85767a6c71

Re: [PATCH 3/3] Remove outdated info in difftool manpage

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Denton (or should I address you as Liu?),
>
> On Fri, 3 Mar 2017, Denton Liu wrote:
>
>> When difftool was rewritten in C, it removed the capability to read
>> fallback configs from mergetool. This changes the documentation to
>> reflect this.
>
> Thanks for pointing that out. But that is probably an oversight on my
> part, not an intentional change...

So, ... in the meantime we'll hold off of this removal from the
documentation and wait for the "capability" to get restored?

[PATCH v2 5/5] Remove outdated info in difftool manpage

[Denton Liu]

When difftool was rewritten in C, it removed the capability to read
fallback configs from mergetool. This changes the documentation to
reflect this.

Signed-off-by: Denton Liu 
---
 Documentation/git-difftool.txt | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa..a00cb033e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,9 +105,6 @@ See linkgit:git-diff[1] for the full list of supported 
options.
 
 CONFIG VARIABLES
 
-'git difftool' falls back to 'git mergetool' config variables when the
-difftool equivalents have not been defined.
-
 diff.tool::
The default diff tool to use.
 
-- 
2.12.0.5.gfbc750a84

Re: [PATCH] line-log.c: prevent crash during union of too many ranges

[Junio C Hamano]

Allan Xavier  writes:

> The existing implementation of range_set_union does not correctly
> reallocate memory, leading to a heap overflow when it attempts to union
> more than 24 separate line ranges.
> ...

I'll add "Reviewed-by: Jeff King " and queue, as I
know we already reviewed the change.

Thanks.

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Duy Nguyen  writes:

> The patch to fix it is this
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f0cd2056ba..e833939320 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect 
> includes' '
>  
>  test_expect_success 'missing include files are ignored' '
>   cat >.gitconfig <<-\EOF &&
> - [include]path = foo
> + [include]path = non-existent
>   [test]value = yes
>   EOF
>   echo yes >expect &&
>
> Junio could you squash this in?

Sure.

Re: [PATCH v7 3/3] config: add conditional include

[Junio C Hamano]

Jeff King  writes:

> That was how I had originally envisioned the namespace working when I
> introduced include.path long ago. And I think Duy's v1 used that, but
> the feedback was that it was not sufficiently obvious that the
> subsection was a conditional.

I am not sure about "obviousness", but I agree that we do not know
that "conditional include" would be the only thing we want for the
second level for "include.path" directive.  "include-if..path"
is better for that reason.

I presume that you could still do

[include "if:gitdir=$path"]
path = ...

i.e. design the second level to begin with a token that tells
readers what it means (and assign "if:" token for "conditional
include"), but I do not think it is worth it.

I also imagine that

[include]
condition = ...
path = ...

is easier to read and write by end-users, but it probably is not
feasible because it requires too invasive change to the current code
to teach it to grok such construct.

Between "include-if" and "includeIf", if people find the latter not
too ugly, I'd prefer to keep it the way Duy posted.  Because of the
way "include.path" and "include-if..path" work, we can declare
that they are not like ordinary configuration variable definition
at all but are higher-level directives and that may be a sufficient
justification to allow "-" in its name, though, if people find
"includeIf" too ugly a name to live.

Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts

[Junio C Hamano]

Jeff King  writes:

> On Fri, Mar 03, 2017 at 10:57:46AM +, Sebastian Schuberth wrote:
>
>> It does not make sense for these placeholder scripts to depend on Python
>> just because the real scripts do. At the example of Git for Windows, we
>> would not even be able to see those warnings as it does not ship with
>> Python. So just use plain shell scripts instead.
>
> Yeah, this seems like an obvious improvement. I think we got here
> because the originals issued a warning but kept working, and then it was
> slowly whittled down to remove the "working" part.
> ...
> OTOH, it is not really hurting much, so I do not mind keeping them
> around for another 3 years (or more) just to catch any stragglers.

Yup, let's queue it and remove it in a few years.

Thanks.

Re: [PATCH v1] Travis: also test on 32-bit Linux

[Junio C Hamano]

Johannes Schindelin  writes:

> On Thu, 2 Mar 2017, Junio C Hamano wrote:
>
>> Another question is which v3 people mean in the discussion, when you
>> and Dscho work on improvements at the same time and each post the
>> "next" version marked as "v3", and they comment on one of them?
>
> But then, Lars & I communicate in a more direct way than the mailing list
> when discussing teeny tiny details such as: "could you have a look at my
> mail" or "how would you change .travis.yml to do XYZ".
>
> With that level of communication, there is almost no danger of two v3s.

Sure, that is true between two (or more) people who communicate
privately.  The issue you raised on Lars's "v1" and your original
unversioned one can be seen either (1) as similar to having two v1's
or (2) having (an implicit) v0 and v1 that won't cause confusion.

As I said already, because the v0 and v1 (or v1 and v1) came from
two different people, it's not such a big deal if Lars named his
first one v1 or v2, just like it won't cause problem if his reroll
were done as v2 or v3.  

I see he did v2 which you Acked in a different thread.  Will replace
what's been on 'pu' and running with Travis the past few days with
it.  Let's wait for one or more Travis cycles and then merge it to
'next'.

Thanks.

[RFC PATCH 1/2] completion: add bash completion for 'git rev-list'

[Denton Liu]

Signed-off-by: Denton Liu 
---
This patch isn't strictly necessary since 'git rev-list' isn't a porcelain
command. However, it might be nice to include in case users interactively call
'git rev-list' anyway.
---
 contrib/completion/git-completion.bash | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 41ee52991..412485369 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2499,6 +2499,36 @@ _git_revert ()
__gitcomp_nl "$(__git_refs)"
 }
 
+__git_rev_list_options="
+ --max-count= --skip= --max-age= --min-age= --sparse --merges --no-merges
+ --min-parents= --no-min-parents --max-parents= --no-max-parents --first-parent
+ --remove-empty --full-history --not --all --branches= --tags= --remotes=
+ --glob= --ignore-missing --stdin --quiet --topo-order --parents --timestamp
+ --left-right --left-only --right-only --cherry-mark --cherry-pick --encoding=
+ --author= --committer= --grep= --regexp-ignore-case --extended-regexp
+ --fixed-strings --date= --objects --objects-edge --objects-edge-aggressive
+ --unpacked --pretty --header --bisect --bisect-vars --bisect-all --merge
+ --reverse --walk-reflogs --no-walk --do-walk --count --use-bitmap-index
+"
+
+__git_complete_rev_list_command ()
+{
+   case "$cur" in
+   --*)
+   __gitcomp "$__git_rev_list_options"
+   return 0
+   ;;
+   esac
+   return 1
+}
+
+_git_rev_list ()
+{
+   __git_has_doubledash && return
+
+   __git_complete_rev_list_command || __gitcomp_nl "$(__git_refs)"
+}
+
 _git_rm ()
 {
case "$cur" in
-- 
2.12.0.1.g5415fdfc5.dirty

Re: [PATCH] t/perf: export variable used in other blocks

[Junio C Hamano]

Jeff King  writes:

> Thanks, this is obviously the right thing to do, and the mistake is mine
> from ea97002fc (t/perf: time rev-list with UNINTERESTING commits,
> 2014-01-20). This is not the first time I've been confused by missing
> variables in t/perf scripts, since it behaves differently than the
> normal test suite. I wonder if we should turn on "set -a" during t/perf
> setup snippets. That's a bit of a blunt tool, but I suspect it would
> just be easier to work with.

I wonder if we can make t/perf to behave more similar to the normal
test suite to eliminate the need for this exporting, by the way.
t/perf/README does not say anything more than "for lack of better
options" throughout its history, which does not help very much.

Hello, i am Lindsey , How's everything with you, I picked interest on you after going through your profile I really want to have a good friendship with you.Beside i have something very vital to tell y

[Lindsey]

Re: log -S/-G (aka pickaxe) searches binary files by default

[Thomas Braun]

Am 03.03.2017 um 17:07 schrieb Junio C Hamano:
> Jeff King  writes:
> 
>> On Thu, Mar 02, 2017 at 05:36:17PM -0800, Junio C Hamano wrote:
>> ...
 Is that on purpose?
>>>
>>> No, it's a mere oversight (as I do not think I never even thought
>>> about special casing binary
>>> files from day one, it is unlikely that you would find _any_ old
>>> version of Git that behaves
>>> differently).
>>
>> The email focuses on "-G", and I think it is wrong to look in binary
>> files there, as "grep in diff" does not make sense for a binary file
>> that we would refuse to diff.
> 
> Yeah, I agree.
> 
>> But the subject also mentions "-S". I always assumed it was intentional
>> to look in binary files there, as it is searching for a pure byte
>> sequence. I would not mind an option to disable that, but I think the
>> default should remain on.
> 
> As the feature was built to be one of the core ingredients necessary
> towards the 'ideal SCM' envisioned in
> 
>   
> 
> 
> "-S" is about finding "a block of text". It was merely an oversight
> that we didn't add explicit code to ignore binary when we introduced
> the concept of "is this text?  is it worth finding things in and
> diffing binary files?".
> 
> I do agree that it may be too late and/or disruptive to change its
> behaviour now, as people may grew expectations different from the
> original motivation and design, though.

Thanks both for the encouraging answers.

I'll try to come up with patches in the next couple of weeks for the
following changes:
"log -G": disable looking in binaries
"log -S": add option to switch looking into binaries, defaults to true

Thomas

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Jeff King  writes:

> For those following on the mailing list, there is some discussion at:
>
>   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>
> I think that is mostly focused around another failing in the
> error-handling of the config code, and that does not need to be
> addressed by this series (though of course I'd welcome any fixes).

Thanks.  Without a message like this, the list may have never known
about the discussion taken elsewhere.  I'd appreciate such a report
to appear on list the next time much earlier ;-)

When built with FREAD_READS_DIRECTORIES=Yes on Linux, the error in
the test can easily reproduce.

In early days of UNIX it was sometimes handy to be able to read the
bytes off of directory to "investigate", but we are not a filesystem
application, and I do not offhand see any reason why we should be
relying on being able to successfully fopen() a directory for
reading.  A FILE * successfully opened that just returns EOF when
read is totally useless for any purpose anyway.  

When the path to be opened from the end user (either from the
command line or in a configuration file) is a directory, it is
better to diagnose it as a user error, and if the path was computed
by our code, it may be a bug.

I am wondering if we should enable this on Linux, at least in
DEVELOPER builds but possibly even on the release builds, to catch
these problems more easily.

Re: Finding a tag that introduced a submodule change

[Junio C Hamano]

Robert Dailey  writes:

> Sometimes I run into a situation where I need to find out which
> release of the product a submodule change was introduced in. This is
> nontrivial, since there are no tags in the submodule itself.

Does your superproject rewind the commit in the submodule project as
it goes forward?  That is, is this property guaranteed to hold by
your project's discipline:

Given any two commits C1 and C2 in the superproject, and the
commit in the submodule bound to C1's and C2's tree (call
them S1 and S2, respectively), if C1 is an ancestor of C2,
then S1 is the same as S2 or an ancestor of S2.

If so, I think you can do a bisection of the history in the
superproject.  Pick an old commit in the superproject that binds an
old commit from the submodule that does not have the change and call
it "good".  Similarly pick a new one in the superproject that binds
a newer commit from the submodule that does have the change, and
call it "bad".  Then do

$ git bisect start $bad $good -- $path_to_submodule

which would suggest you to test commits that change what commit is
bound at the submodule's path.

When testing each of these commits, you would see if the commit
bound at the submodule's path has the change or not.

$ current=$(git rev-parse HEAD:$path_to_submodule)

would give you the object name of that commit, and then

$ git -C $path_to_submodule merge-base --is-ancestor $change $current

would tell you if the $change you are interested in is already
contained in that $current commit.  Then you say "git bisect good"
if $current is too old to contain the $change, and "git bisect bad"
if $current is sufficiently new and contains the $change, to
continue.

If your superproject rewinds the commit in the submodule as it goes
forward, e.g. an older commit in the superproject used submodule
commit from day X, but somebody who made yesterday's commit in the
superproject realized that that submodule commit was broken and used
an older commit in the submodule from day (X-1), then you cannot
bisect.  In such a case, I think you would essentially need to check
all superproject commits that changed the commit bound at the
submodule's path.

$ git rev-list $bad..$good -- $path_to_submodule

would give a list of such commits, and you would do the "merge-base"
check for all them to see which ones have and do not have the
$change (replace "HEAD" with the commit you are testing in the
computation that gives you $current).

Re: [PATCH 1/3] Add --gui option to mergetool

[Rémi Galan Alfonso]

Hi,

Denton Liu  writes:
> [...]
>
> +test_expect_success 'gui mergetool' '
> +  test_when_finished "git reset --hard" &&
> +  test_when_finished "git config merge.tool mytool" &&
> +  test_when_finished "git config --unset merge.guitool" &&
> +  git config merge.tool badtool &&
> +  git config merge.guitool mytool &&

You should be able to squash the lines
  `test_when_finished "git config --unset merge.guitool" &&`
and
  `git config merge.guitool mytool &&`
into
  `test_config merge.guitool mytool`

(It is however not possible with merge.tool since you set it to a
specific value 'when_finished')

Thanks,
Rémi

Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

[Junio C Hamano]

Torsten Bögershausen  writes:

> Understood, thanks for the explanation.
>
> quiet is not quite any more..
>
> Does the following fix help ?
>
> --- a/diff.c
> +++ b/diff.c
> @@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,
> unsigned int flags)
> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
> ? SAFE_CRLF_WARN
> : safe_crlf);
> +   if (size_only)
> +   crlf_warn = SAFE_CRLF_FALSE;

If you were to go this route, it may be sufficient to change its
initialization from WARN to FALSE _unconditionally_, because this
function uses the convert_to_git() only to _show_ the differences by
computing canonical form out of working tree contents, and the
conversion is not done to _write_ into object database to create a
new object.

Having size_only here is not a sign of getting --quiet passed from
the command line, by the way.

Re: Finding a tag that introduced a submodule change

[Jacob Keller]

On Fri, Mar 3, 2017 at 7:40 AM, Robert Dailey  wrote:
> I have a repository with a single submodule in it. Since the parent
> repository represents the code base for an actual product, I tag
> release versions in the parent repository. I do not put tags in the
> submodule since multiple other products may be using it there and I
> wanted to avoid ambiguous tags.
>

Hi,

I agree you shouldn't use tags in the submodules.

> Sometimes I run into a situation where I need to find out which
> release of the product a submodule change was introduced in. This is
> nontrivial, since there are no tags in the submodule itself. This is
> one thing I tried:
>

I've run into this exact problem at $DAYJOB.

> 1. Do a `git log` in the submodule to find the SHA1 representing the
> change I want to check for
> 2. In the parent repository, do a git log with pickaxe to determine
> when the submodule itself changed to the value of that SHA1.
> 3. Based on the result of #2, do a `git tag --contains` to see the
> lowest-version tag that contains the SHA1, which will identify the
> first release that introduced that change
>
> However, I was not able to get past #2 because apparently there are
> cases where when we move the submodule "forward", we skip over
> commits, so the value of the submodule itself never was set to that
> SHA1.
>
> I'm at a loss here on how to easily do this. Can someone recommend a
> way to do this? Obviously the easier the better, as I have to somehow
> train my team how to do this on their own.
>
> Thanks in advance.

So there's better ways to do this, but I do think there would be value
in adding some plumbing to make it easier.

Here is how I would do this, best if written into a shell script or
similar to automate the tricky part of #2

1. Do a git-log of the *parent* project, filtering out to show only
the path to the submodule

2. For each commit here, you find the new and old values of the
submodule pointer.

3. Use git merge-base --is-ancestor to ensure that "old" is an
ancestor of "submodule sha1id" and then

4. Use git-merge-base to ensure that "submodule sha1id" is an
anscestor of "new".

If both these are tree, then you know that the commit was included
into the parent project at this point.

I've had to do this once or twice, but I don't actually remember
exactly how I did 3. One sneaky way would be to add new tags for each
submodule change something like the following might work and be more
efficient. I'm not really sure but here's how I would go that route:

1. git log   --pretty=%h | parallel git ls-tree {}
path-to-submodule

The above more or less prints every submodule value as it changed over
time in the parent project.

Next, for each submodule change:

2. git -C  tag parent/ 

Create a new tag prefixed by "parent" that includes the sha1id of the
parent commit, and create it inside the submodule

3. git -C submodule describe --contains --match="parent/*" 

Once you're done you can also delete all the tags that are in the
"parent" prefix if you dont' really wanna see them again.

Basically, re-use the machinery to tag and then use describe
--contains to find the commit.

I *really* think a similar algorithm could be embedded as a plumbing
subcommand, since I think this is tedious to do by hand.

I'm not really sure if this is the "best" algorithm either, but it's
pretty much what I've used in the past. Either the tag way or the log
yourself one at a time way.

Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

[Torsten Bögershausen]

Understood, thanks for the explanation.

quiet is not quite any more..

Does the following fix help ?

--- a/diff.c
+++ b/diff.c
@@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,
unsigned int flags)
enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
? SAFE_CRLF_WARN
: safe_crlf);
+   if (size_only)
+   crlf_warn = SAFE_CRLF_FALSE;

[PATCH v3 7/9] read_early_config(): avoid .git/config hack when unneeded

[Johannes Schindelin]

So far, we only look whether the startup_info claims to have seen a
git_dir.

However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR. And when
this is the case, the repository config is handled already, so we do not
have to do that again explicitly.

Let's just use the same function, have_git_dir(), to determine whether we
have to handle .git/config explicitly.

Signed-off-by: Johannes Schindelin 
---
 config.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 9cfbeafd04c..068fa4dcfa6 100644
--- a/config.c
+++ b/config.c
@@ -1427,14 +1427,15 @@ void read_early_config(config_fn_t cb, void *data)
 * core.repositoryformatversion), we'll read whatever is in .git/config
 * blindly. Similarly, if we _are_ in a repository, but not at the
 * root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
+* ../.git/config, etc), unless setup_git_directory() was already 
called.
+* See t7006 for a complete set of failures.
 *
 * However, we have historically provided this hack because it does
 * work some of the time (namely when you are at the top-level of a
 * valid repository), and would rarely make things worse (i.e., you do
 * not generally have a .git/config file sitting around).
 */
-   if (!startup_info->have_repository) {
+   if (!have_git_dir()) {
struct git_config_source repo_config;
 
memset(&repo_config, 0, sizeof(repo_config));
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 5/9] Export the discover_git_directory() function

[Johannes Schindelin]

The setup_git_directory_gently_1() function we modified earlier still
needs to return both the path to the .git/ directory as well as the
"cd-up" path to allow setup_git_directory() to retain its previous
behavior as if it changed the current working directory on its quest for
the .git/ directory.

This is a bit cumbersome to use if you only need to figure out the
(possibly absolute) path of the .git/ directory. Let's just provide
a convenient wrapper function with an easier signature that *just*
discovers the .git/ directory.

We will use it in a subsequent patch to support early config reading
better.

Signed-off-by: Johannes Schindelin 
---
 cache.h |  2 ++
 setup.c | 31 +++
 2 files changed, 33 insertions(+)

diff --git a/cache.h b/cache.h
index 80b6372cf76..4d8eb38de74 100644
--- a/cache.h
+++ b/cache.h
@@ -518,6 +518,8 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern void setup_work_tree(void);
+/* Find GIT_DIR without changing the working directory or other global state */
+extern const char *discover_git_directory(struct strbuf *gitdir);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/setup.c b/setup.c
index 9a09bb41ab5..32ce023638a 100644
--- a/setup.c
+++ b/setup.c
@@ -923,6 +923,37 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
}
 }
 
+const char *discover_git_directory(struct strbuf *gitdir)
+{
+   struct strbuf dir = STRBUF_INIT;
+   size_t gitdir_offset = gitdir->len, cwd_len;
+
+   if (strbuf_getcwd(&dir))
+   return NULL;
+
+   cwd_len = dir.len;
+   if (setup_git_directory_gently_1(&dir, gitdir) < 0) {
+   strbuf_release(&dir);
+   return NULL;
+   }
+
+   /*
+* The returned gitdir is relative to dir, and if dir does not reflect
+* the current working directory, we simply make the gitdir absolute.
+*/
+   if (dir.len < cwd_len && !is_absolute_path(gitdir->buf + 
gitdir_offset)) {
+   /* Avoid a trailing "/." */
+   if (!strcmp(".", gitdir->buf + gitdir_offset))
+   strbuf_setlen(gitdir, gitdir_offset);
+   else
+   strbuf_addch(&dir, '/');
+   strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
+   }
+   strbuf_release(&dir);
+
+   return gitdir->buf;
+}
+
 const char *setup_git_directory_gently(int *nongit_ok)
 {
struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = 
STRBUF_INIT;
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 4/9] setup_git_directory_1(): avoid changing global state

[Johannes Schindelin]

For historical reasons, Git searches for the .git/ directory (or the
.git file) by changing the working directory successively to the parent
directory of the current directory, until either anything was found or
until a ceiling or a mount point is hit.

Further global state may be changed in case a .git/ directory was found.

We do have a use case, though, where we would like to find the .git/
directory without having any global state touched, though: when we read
the early config e.g. for the pager or for alias expansion.

Let's just move all of code that changes any global state out of the
function `setup_git_directory_gently_1()` into
`setup_git_directory_gently()`.

In subsequent patches, we will use the _1() function in a new
`discover_git_directory()` function that we will then use for the early
config code.

Note: the new loop is a *little* tricky, as we have to handle the root
directory specially: we cannot simply strip away the last component
including the slash, as the root directory only has that slash. To remedy
that, we introduce the `min_offset` variable that holds the minimal length
of an absolute path, and using that to special-case the root directory,
including an early exit before trying to find the parent of the root
directory.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 189 ++--
 1 file changed, 113 insertions(+), 76 deletions(-)

diff --git a/setup.c b/setup.c
index 91d884b6746..9a09bb41ab5 100644
--- a/setup.c
+++ b/setup.c
@@ -818,50 +818,49 @@ static int canonicalize_ceiling_entry(struct 
string_list_item *item,
}
 }
 
+enum discovery_result {
+   GIT_DIR_NONE = 0,
+   GIT_DIR_EXPLICIT,
+   GIT_DIR_DISCOVERED,
+   GIT_DIR_BARE,
+   /* these are errors */
+   GIT_DIR_HIT_CEILING = -1,
+   GIT_DIR_HIT_MOUNT_POINT = -2
+};
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
+ *
+ * Also, we avoid changing any global state (such as the current working
+ * directory) to allow early callers.
+ *
+ * The directory where the search should start needs to be passed in via the
+ * `dir` parameter; upon return, the `dir` buffer will contain the path of
+ * the directory where the search ended, and `gitdir` will contain the path of
+ * the discovered .git/ directory, if any. This path may be relative against
+ * `dir` (i.e. *not* necessarily the cwd).
  */
-static const char *setup_git_directory_gently_1(int *nongit_ok)
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ struct strbuf *gitdir)
 {
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
-   static struct strbuf cwd = STRBUF_INIT;
-   const char *gitdirenv, *ret;
-   char *gitfile;
-   int offset, offset_parent, ceil_offset = -1;
+   const char *gitdirenv;
+   int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 
1;
dev_t current_device = 0;
int one_filesystem = 1;
 
/*
-* We may have read an incomplete configuration before
-* setting-up the git directory. If so, clear the cache so
-* that the next queries to the configuration reload complete
-* configuration (including the per-repo config file that we
-* ignored previously).
-*/
-   git_config_clear();
-
-   /*
-* Let's assume that we are in a git repository.
-* If it turns out later that we are somewhere else, the value will be
-* updated accordingly.
-*/
-   if (nongit_ok)
-   *nongit_ok = 0;
-
-   if (strbuf_getcwd(&cwd))
-   die_errno(_("Unable to read current working directory"));
-   offset = cwd.len;
-
-   /*
 * If GIT_DIR is set explicitly, we're not going
 * to do any discovery, but we still do repository
 * validation.
 */
gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-   if (gitdirenv)
-   return setup_explicit_git_dir(gitdirenv, &cwd, nongit_ok);
+   if (gitdirenv) {
+   strbuf_addstr(gitdir, gitdirenv);
+   return GIT_DIR_EXPLICIT;
+   }
 
if (env_ceiling_dirs) {
int empty_entry_found = 0;
@@ -869,15 +868,15 @@ static const char *setup_git_directory_gently_1(int 
*nongit_ok)
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, 
-1);
filter_string_list(&ceiling_dirs, 0,
   canonicalize_ceiling_entry, 
&empty_entry_found);
-   ceil_offset = longest_ancestor_length(cwd.buf, &ceiling_dirs);
+   ceil_offset = longest_ancestor_length(dir->buf, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0)

[PATCH v3 2/9] setup_git_directory(): use is_dir_sep() helper

[Johannes Schindelin]

It is okay in practice to test for forward slashes in the output of
getcwd(), because we go out of our way to convert backslashes to forward
slashes in getcwd()'s output on Windows.

Still, the correct way to test for a dir separator is by using the
helper function we introduced for that very purpose. It also serves as a
good documentation what the code tries to do (not "how").

Signed-off-by: Johannes Schindelin 
---
 setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 967f289f1ef..c47619605fb 100644
--- a/setup.c
+++ b/setup.c
@@ -910,7 +910,8 @@ static const char *setup_git_directory_gently_1(int 
*nongit_ok)
return setup_bare_git_dir(&cwd, offset, nongit_ok);
 
offset_parent = offset;
-   while (--offset_parent > ceil_offset && cwd.buf[offset_parent] 
!= '/');
+   while (--offset_parent > ceil_offset &&
+  !is_dir_sep(cwd.buf[offset_parent]));
if (offset_parent <= ceil_offset)
return setup_nongit(cwd.buf, nongit_ok);
if (one_filesystem) {
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 9/9] Test read_early_config()

[Johannes Schindelin]

So far, we had no explicit tests of that function.

Signed-off-by: Johannes Schindelin 
---
 t/helper/test-config.c  | 15 +++
 t/t1309-early-config.sh | 50 +
 2 files changed, 65 insertions(+)
 create mode 100755 t/t1309-early-config.sh

diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 83a4f2ab869..8e3ed6a76cb 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -66,6 +66,16 @@ static int iterate_cb(const char *var, const char *value, 
void *data)
return 0;
 }
 
+static int early_config_cb(const char *var, const char *value, void *vdata)
+{
+   const char *key = vdata;
+
+   if (!strcmp(key, var))
+   printf("%s\n", value);
+
+   return 0;
+}
+
 int cmd_main(int argc, const char **argv)
 {
int i, val;
@@ -73,6 +83,11 @@ int cmd_main(int argc, const char **argv)
const struct string_list *strptr;
struct config_set cs;
 
+   if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+   read_early_config(early_config_cb, (void *)argv[2]);
+   return 0;
+   }
+
setup_git_directory();
 
git_configset_init(&cs);
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
new file mode 100755
index 000..0c55dee514c
--- /dev/null
+++ b/t/t1309-early-config.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='Test read_early_config()'
+
+. ./test-lib.sh
+
+test_expect_success 'read early config' '
+   test_config early.config correct &&
+   test-config read_early_config early.config >output &&
+   test correct = "$(cat output)"
+'
+
+test_expect_success 'in a sub-directory' '
+   test_config early.config sub &&
+   mkdir -p sub &&
+   (
+   cd sub &&
+   test-config read_early_config early.config
+   ) >output &&
+   test sub = "$(cat output)"
+'
+
+test_expect_success 'ceiling' '
+   test_config early.config ceiling &&
+   mkdir -p sub &&
+   (
+   GIT_CEILING_DIRECTORIES="$PWD" &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd sub &&
+   test-config read_early_config early.config
+   ) >output &&
+   test -z "$(cat output)"
+'
+
+test_expect_success 'ceiling #2' '
+   mkdir -p xdg/git &&
+   git config -f xdg/git/config early.config xdg &&
+   test_config early.config ceiling &&
+   mkdir -p sub &&
+   (
+   XDG_CONFIG_HOME="$PWD"/xdg &&
+   GIT_CEILING_DIRECTORIES="$PWD" &&
+   export GIT_CEILING_DIRECTORIES XDG_CONFIG_HOME &&
+   cd sub &&
+   test-config read_early_config early.config
+   ) >output &&
+   test xdg = "$(cat output)"
+'
+
+test_done
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 8/9] read_early_config(): really discover .git/

[Johannes Schindelin]

Earlier, we punted and simply assumed that we are in the top-level
directory of the project, and that there is no .git file but a .git/
directory so that we can read directly from .git/config.

However, that is not necessarily true. We may be in a subdirectory. Or
.git may be a gitfile. Or the environment variable GIT_DIR may be set.

To remedy this situation, we just refactored the way
setup_git_directory() discovers the .git/ directory, to make it
reusable, and more importantly, to leave all global variables and the
current working directory alone.

Let's discover the .git/ directory correctly in read_early_config() by
using that new function.

This fixes 4 known breakages in t7006.

Signed-off-by: Johannes Schindelin 
---
 config.c | 33 ++---
 setup.c  | 14 +-
 t/t7006-pager.sh |  8 
 3 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/config.c b/config.c
index 068fa4dcfa6..749623a9649 100644
--- a/config.c
+++ b/config.c
@@ -1414,34 +1414,29 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
 
 void read_early_config(config_fn_t cb, void *data)
 {
+   struct strbuf buf = STRBUF_INIT;
+
git_config_with_options(cb, data, NULL, 1);
 
/*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc), unless setup_git_directory() was already 
called.
-* See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
+* When we are not about to create a repository ourselves (init or
+* clone) and when no .git/ directory was set up yet (in which case
+* git_config_with_options() would already have picked up the
+* repository config), we ask discover_git_directory() to figure out
+* whether there is any repository config we should use (but unlike
+* setup_git_directory_gently(), no global state is changed, most
+* notably, the current working directory is still the same after
+* the call).
 */
-   if (!have_git_dir()) {
+   if (!have_git_dir() && discover_git_directory(&buf)) {
struct git_config_source repo_config;
 
memset(&repo_config, 0, sizeof(repo_config));
-   repo_config.file = ".git/config";
+   strbuf_addstr(&buf, "/config");
+   repo_config.file = buf.buf;
git_config_with_options(cb, data, &repo_config, 1);
}
+   strbuf_release(&buf);
 }
 
 static void git_config_check_init(void);
diff --git a/setup.c b/setup.c
index 32ce023638a..5320ae37314 100644
--- a/setup.c
+++ b/setup.c
@@ -925,8 +925,9 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
 
 const char *discover_git_directory(struct strbuf *gitdir)
 {
-   struct strbuf dir = STRBUF_INIT;
+   struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
size_t gitdir_offset = gitdir->len, cwd_len;
+   struct repository_format candidate;
 
if (strbuf_getcwd(&dir))
return NULL;
@@ -949,8 +950,19 @@ const char *discover_git_directory(struct strbuf *gitdir)
strbuf_addch(&dir, '/');
strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
}
+
+   strbuf_reset(&dir);
+   strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
+   read_repository_format(&candidate, dir.buf);
strbuf_release(&dir);
 
+   if (verify_repository_format(&candidate, &err) < 0) {
+   warning("ignoring git dir '%s': %s",
+   gitdir->buf + gitdir_offset, err.buf);
+   strbuf_release(&err);
+   return NULL;
+   }
+
return gitdir->buf;
 }
 
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 304ae06c600..4f3794d415e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -360,19 +360,19 @@ test_pager_choices   'git aliasedlog'
 test_default_pagerexpect_success 'git -p aliasedlog'
 test_PAGER_overrides  expect_success 'git -p alia

[PATCH v3 6/9] Make read_early_config() reusable

[Johannes Schindelin]

The pager configuration needs to be read early, possibly before
discovering any .git/ directory.

Let's not hide this function in pager.c, but make it available to other
callers.

Signed-off-by: Johannes Schindelin 
---
 cache.h  |  1 +
 config.c | 31 +++
 pager.c  | 31 ---
 3 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/cache.h b/cache.h
index 4d8eb38de74..8a4580f921d 100644
--- a/cache.h
+++ b/cache.h
@@ -1799,6 +1799,7 @@ extern int git_config_from_blob_sha1(config_fn_t fn, 
const char *name,
 const unsigned char *sha1, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
+extern void read_early_config(config_fn_t cb, void *data);
 extern void git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
   struct git_config_source *config_source,
diff --git a/config.c b/config.c
index c6b874a7bf7..9cfbeafd04c 100644
--- a/config.c
+++ b/config.c
@@ -1412,6 +1412,37 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
}
 }
 
+void read_early_config(config_fn_t cb, void *data)
+{
+   git_config_with_options(cb, data, NULL, 1);
+
+   /*
+* Note that this is a really dirty hack that does the wrong thing in
+* many cases. The crux of the problem is that we cannot run
+* setup_git_directory() early on in git's setup, so we have no idea if
+* we are in a repository or not, and therefore are not sure whether
+* and how to read repository-local config.
+*
+* So if we _aren't_ in a repository (or we are but we would reject its
+* core.repositoryformatversion), we'll read whatever is in .git/config
+* blindly. Similarly, if we _are_ in a repository, but not at the
+* root, we'll fail to find .git/config (because it's really
+* ../.git/config, etc). See t7006 for a complete set of failures.
+*
+* However, we have historically provided this hack because it does
+* work some of the time (namely when you are at the top-level of a
+* valid repository), and would rarely make things worse (i.e., you do
+* not generally have a .git/config file sitting around).
+*/
+   if (!startup_info->have_repository) {
+   struct git_config_source repo_config;
+
+   memset(&repo_config, 0, sizeof(repo_config));
+   repo_config.file = ".git/config";
+   git_config_with_options(cb, data, &repo_config, 1);
+   }
+}
+
 static void git_config_check_init(void);
 
 void git_config(config_fn_t fn, void *data)
diff --git a/pager.c b/pager.c
index ae796433630..73ca8bc3b17 100644
--- a/pager.c
+++ b/pager.c
@@ -43,37 +43,6 @@ static int core_pager_config(const char *var, const char 
*value, void *data)
return 0;
 }
 
-static void read_early_config(config_fn_t cb, void *data)
-{
-   git_config_with_options(cb, data, NULL, 1);
-
-   /*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
-*/
-   if (!startup_info->have_repository) {
-   struct git_config_source repo_config;
-
-   memset(&repo_config, 0, sizeof(repo_config));
-   repo_config.file = ".git/config";
-   git_config_with_options(cb, data, &repo_config, 1);
-   }
-}
-
 const char *git_pager(int stdout_is_tty)
 {
const char *pager;
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 3/9] Prepare setup_discovered_git_directory() the root directory

[Johannes Schindelin]

Currently, the offset parameter (indicating what part of the cwd
parameter corresponds to the current directory after discovering the
.git/ directory) is set to 0 when we are running in the root directory.

However, in the next patches we will avoid changing the current working
directory while searching for the .git/ directory, meaning that the
offset corresponding to the root directory will have to be 1 to reflect
that this directory is characterized by the path "/" (and not "").

So let's make sure that setup_discovered_git_directory() only tries to
append the trailing slash to non-root directories.

Note: the setup_bare_git_directory() does not need a corresponding
change, as it does not want to return a prefix.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index c47619605fb..91d884b6746 100644
--- a/setup.c
+++ b/setup.c
@@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char 
*gitdir,
if (offset == cwd->len)
return NULL;
 
-   /* Make "offset" point to past the '/', and add a '/' at the end */
-   offset++;
+   /* Make "offset" point past the '/' (already the case for root dirs) */
+   if (offset != offset_1st_component(cwd->buf))
+   offset++;
+   /* Add a '/' at the end */
strbuf_addch(cwd, '/');
return cwd->buf + offset;
 }
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 1/9] t7006: replace dubious test

[Johannes Schindelin]

The idea of the test case "git -p - core.pager is not used from
subdirectory" was to verify that the setup_git_directory() function had
not been called just to obtain the core.pager setting.

However, we are about to fix the early config machinery so that it
*does* work, without messing up the global state.

Once that is done, the core.pager setting *will* be used, even when
running from a subdirectory, and that is a Good Thing.

The intention of that test case, however, was to verify that the
setup_git_directory() function has not run, because it changes global
state such as the current working directory.

To keep that spirit, but fix the incorrect assumption, this patch
replaces that test case by a new one that verifies that the pager is
run in the subdirectory, i.e. that the current working directory has
not been changed at the time the pager is configured and launched, even
if the `rev-parse` command requires a .git/ directory and *will* change
the working directory.

Signed-off-by: Johannes Schindelin 
---
 t/t7006-pager.sh | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c8dc665f2fd..304ae06c600 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -378,9 +378,19 @@ test_GIT_PAGER_overrides  expect_success test_must_fail 
'git -p request-pull'
 test_default_pagerexpect_success test_must_fail 'git -p'
 test_PAGER_overrides  expect_success test_must_fail 'git -p'
 test_local_config_ignored expect_failure test_must_fail 'git -p'
-test_no_local_config_subdir expect_success test_must_fail 'git -p'
 test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
+test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
+   sane_unset GIT_PAGER &&
+   test_config core.pager "cat >cwd-retained" &&
+   (
+   cd sub &&
+   rm -f cwd-retained &&
+   test_terminal git -p rev-parse HEAD &&
+   test_path_is_file cwd-retained
+   )
+'
+
 test_doesnt_paginate  expect_failure test_must_fail 'git -p nonsense'
 
 test_pager_choices   'git shortlog'
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v3 0/9] Fix the early config

[Johannes Schindelin]

These patches are an attempt to make Git's startup sequence a bit less
surprising.

The idea here is to discover the .git/ directory gently (i.e. without
changing the current working directory, or global variables), and to use
it to read the .git/config file early, before we actually called
setup_git_directory() (if we ever do that).

This also allows us to fix the early config e.g. to determine the pager
or to resolve aliases in a non-surprising manner.

My dirty little secret is that I need to discover the Git directory
early, without changing global state, for usage statistics gathering in
the GVFS Git project, so I actually do not care all that much about the
early config, although it is a welcome fallout (and a good reason for
accepting these patches and thereby releasing me of more maintenance
burden :-)).

Notable notes:

- In contrast to earlier versions, I no longer special-case init and
  clone. Peff pointed out that this adds technical debt, and that we can
  actually argue (for consistency's sake) that early config reads the
  current repository config (if any) even for init and clone.

- The read_early_config() function does not cache Git directory
  discovery nor read values. If needed, this can be implemented later,
  in a separate patch series.

- The alias handling in git.c could possibly benefit from this work, but
  again, this is a separate topic from the current patch series.

Changes since v2:

- replaced `test -e` by `test_path_is_file`

- fixed premature "cwd -> dir" in 2/9

- the setup_git_directory_gently_1() function is no longer renamed
  because it is not exported directly, anyway

- fixed the way setup_discovered_git_dir() expected the offset parameter
  to exclude the trailing slash (which is not true for root
  directories); also verified that setup_bare_git_dir() does not require
  a corresponding patch

- switched to using size_t instead of int to save the length of the
  strbuf in discover_git_directory()

- ensured that discover_git_directory() turns a relative gitdir into an
  absolute one even if there is already some text in the strbuf

- clarified under which circumstances we turn a relative gitdir into an
  absolute one

- avoided absolute gitdir with trailing "/." to be returned

- the commit that fixes the "really dirty hack" now rewords that comment
  to reflect that it is no longer a really dirty hack

- dropped the special-casing of init and clone

- the discover_git_directory() function now correctly checks the
  repository version, warning (and returning NULL) in case of a problem


Johannes Schindelin (9):
  t7006: replace dubious test
  setup_git_directory(): use is_dir_sep() helper
  Prepare setup_discovered_git_directory() the root directory
  setup_git_directory_1(): avoid changing global state
  Export the discover_git_directory() function
  Make read_early_config() reusable
  read_early_config(): avoid .git/config hack when unneeded
  read_early_config(): really discover .git/
  Test read_early_config()

 cache.h |   3 +
 config.c|  27 ++
 pager.c |  31 ---
 setup.c | 237 
 t/helper/test-config.c  |  15 +++
 t/t1309-early-config.sh |  50 ++
 t/t7006-pager.sh|  18 +++-
 7 files changed, 269 insertions(+), 112 deletions(-)
 create mode 100755 t/t1309-early-config.sh


base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
Published-As: https://github.com/dscho/git/releases/tag/early-config-v3
Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v3

Interdiff vs v2:

 diff --git a/cache.h b/cache.h
 index 0af7141242f..8a4580f921d 100644
 --- a/cache.h
 +++ b/cache.h
 @@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
  
  extern void setup_work_tree(void);
 +/* Find GIT_DIR without changing the working directory or other global state 
*/
  extern const char *discover_git_directory(struct strbuf *gitdir);
  extern const char *setup_git_directory_gently(int *);
  extern const char *setup_git_directory(void);
 @@ -2070,7 +2071,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
  
  /* setup.c */
  struct startup_info {
 -  int have_repository, creating_repository;
 +  int have_repository;
const char *prefix;
  };
  extern struct startup_info *startup_info;
 diff --git a/config.c b/config.c
 index bcda397d42e..749623a9649 100644
 --- a/config.c
 +++ b/config.c
 @@ -1419,25 +1419,16 @@ void read_early_config(config_fn_t cb, void *data)
git_config_with_options(cb, data, NULL, 1);
  
/*
 -   * Note that this is a really dirty hack that does the wrong thing in
 -   * many cases. The crux of the problem is that we cannot run
 -   * setup_git_directory() early on in git's setup, so we have no idea if
 -   * we are in a repository or not, and therefore are not sure whether
 -   

Re: [PATCH v5 24/24] t1406: new tests for submodule ref store

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  t/t1406-submodule-ref-store.sh (new +x) | 95 
> +
>  1 file changed, 95 insertions(+)
>  create mode 100755 t/t1406-submodule-ref-store.sh

I wonder if you could reduce some of the repetition between this test
file and t1405, at least for the functions that should work for both the
main repository and for submodules? For example, if you were to define

GIT='git'

in one case and

GIT='git -C sub'

in the other, then maybe some of the setup code could be made to look
alike? Then the division could be one test file for the functions that
work for both main and submodules, and a second for the functions that
only work for main. Just a thought...

Michael

Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

[Mike Crowe]

Hi Torsten,

Your patch has been superseded, but I thought I ought to answer your
questions rather than leave them hanging.

On Thursday 02 March 2017 at 19:17:00 +0100, Torsten Bögershausen wrote:
> On 2017-03-01 22:25, Mike Crowe wrote:
> > On Wednesday 01 March 2017 at 18:04:44 +0100, tbo...@web.de wrote:
> >> From: Junio C Hamano 
> >>
> >> git diff --quiet may take a short-cut to see if a file is changed
> >> in the working tree:
> >> Whenever the file size differs from what is recorded in the index,
> >> the file is assumed to be changed and git diff --quiet returns
> >> exit with code 1
> >>
> >> This shortcut must be suppressed whenever the line endings are converted
> >> or a filter is in use.
> >> The attributes say "* text=auto" and a file has
> >> "Hello\nWorld\n" in the index with a length of 12.
> >> The file in the working tree has "Hello\r\nWorld\r\n" with a length of 14.
> >> (Or even "Hello\r\nWorld\n").
> >> In this case "git add" will not do any changes to the index, and
> >> "git diff -quiet" should exit 0.
> >>
> >> Add calls to would_convert_to_git() before blindly saying that a different
> >> size means different content.
> >>
> >> Reported-By: Mike Crowe 
> >> Signed-off-by: Torsten Bögershausen 
> >> ---
> >> This is what I can come up with, collecting all the loose ends.
> >> I'm not sure if Mike wan't to have the Reported-By with a
> >> Signed-off-by ?
> >> The other question is, if the commit message summarizes the discussion
> >> well enough ?
> >>
> >> diff.c| 18 ++
> >>  t/t0028-diff-converted.sh | 27 +++
> >>  2 files changed, 41 insertions(+), 4 deletions(-)
> >>  create mode 100755 t/t0028-diff-converted.sh
> >>
> >> diff --git a/diff.c b/diff.c
> >> index 051761b..c264758 100644
> >> --- a/diff.c
> >> +++ b/diff.c
> >> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct 
> >> diff_filepair *p)
> >> *differences.
> >> *
> >> * 2. At this point, the file is known to be modified,
> >> -   *with the same mode and size, and the object
> >> -   *name of one side is unknown.  Need to inspect
> >> -   *the identical contents.
> >> +   *with the same mode and size, the object
> >> +   *name of one side is unknown, or size comparison
> >> +   *cannot be depended upon.  Need to inspect the
> >> +   *contents.
> >> */
> >>if (!DIFF_FILE_VALID(p->one) || /* (1) */
> >>!DIFF_FILE_VALID(p->two) ||
> >> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct 
> >> diff_filepair *p)
> >>(p->one->mode != p->two->mode) ||
> >>diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
> >>diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
> >> -  (p->one->size != p->two->size) ||
> >> +
> >> +  /*
> >> +   * only if eol and other conversions are not involved,
> >> +   * we can say that two contents of different sizes
> >> +   * cannot be the same without checking their contents.
> >> +   */
> >> +  (!would_convert_to_git(p->one->path) &&
> >> +   !would_convert_to_git(p->two->path) &&
> >> +   (p->one->size != p->two->size)) ||
> >> +
> >>!diff_filespec_is_identical(p->one, p->two)) /* (2) */
> >>p->skip_stat_unmatch_result = 1;
> >>return p->skip_stat_unmatch_result;
> >> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh
> >> new file mode 100755
> >> index 000..3d5ab95
> >> --- /dev/null
> >> +++ b/t/t0028-diff-converted.sh
> >> @@ -0,0 +1,27 @@
> >> +#!/bin/sh
> >> +#
> >> +# Copyright (c) 2017 Mike Crowe
> >> +#
> >> +# These tests ensure that files changing line endings in the presence
> >> +# of .gitattributes to indicate that line endings should be ignored
> >> +# don't cause 'git diff' or 'git diff --quiet' to think that they have
> >> +# been changed.
> >> +
> >> +test_description='git diff with files that require CRLF conversion'
> >> +
> >> +. ./test-lib.sh
> >> +
> >> +test_expect_success setup '
> >> +  echo "* text=auto" >.gitattributes &&
> >> +  printf "Hello\r\nWorld\r\n" >crlf.txt &&
> >> +  git add .gitattributes crlf.txt &&
> >> +  git commit -m "initial"
> >> +'
> >> +
> >> +test_expect_success 'quiet diff works on file with line-ending change 
> >> that has no effect on repository' '
> >> +  printf "Hello\r\nWorld\n" >crlf.txt &&
> >> +  git status &&
> >> +  git diff --quiet
> >> +'
> >> +
> >> +test_done
> >

[snip]

> > Also, I think I've found a behaviour change with this fix. Consider:
> > 
> >  echo "* text=auto" >.gitattributes
> >  printf "Hello\r\nWorld\r\n" >crlf.txt
> That should give
> "Hello\nWorld\n" in the index:
> 
> git add .gitattributes crlf.txt
> warning: CRLF will be replaced by LF in ttt/crlf.txt.
> The file will have its original line endings in your working directory.
> tb@mac:/tmp/ttt> git commit -m "initial"
> [master (root-commit) 354f657] initial
>  2 files changed, 3 insertions(+)
>  crea

Re: [PATCH v2 8/9] read_early_config(): really discover .git/

[Johannes Schindelin]

Hi Peff,

On Fri, 3 Mar 2017, Jeff King wrote:

> I think the "dirty hack..." comment in read_early_config() can be
> condensed (or go away) now.

Yes, I made that change in response to a comment you made about an earlier
patch in this series.

> I think we _could_ do away with read_early_config() entirely, and just
> have the regular config code do this lookup when we're not already in a
> repo. But then we'd really need to depend on the "creating_repository"
> flag being managed correctly.

Well, that would be a major design change. I'm not really all that
comfortable with that...

> I think I prefer the idea that a few "early" spots like pager and alias
> config need to use this special function to access the config. That's
> less likely to cause surprises when some config option is picked up
> before we have run setup_git_directory().

Exactly. There is semantic meaning in calling read_early_config() vs
git_config().

> There is one surprising case that I think we need to deal with even now,
> though. If I do:
> 
>   git init repo
>   git -C repo config pager.upload-pack 'echo whoops'
>   git upload-pack repo
>   cd repo && git upload-pack .
> 
> the first one is OK, but the second reads and executes the pager config
> from the repo, even though we usually consider upload-pack to be OK to
> run in an untrusted repo. This _isn't_ a new thing in your patch, but
> just something I noticed while we are here.
> 
> And maybe it is a non-issue. The early-config bits all happen via the
> git wrapper, and normally we use the direct dashed "git-upload-pack" to
> access the other repositories. Certainly I have been known to use "git
> -c ... upload-pack" while debugging various things.
> 
> So I dunno. You really have to jump through some hoops for it to matter,
> but I just wonder if the git wrapper should be special-casing
> upload-pack, too.

While I agree that it may sound surprising, I would like to point out that
there *is* a semantic difference between `git upload-pack repo` and `git
-C repo upload-pack .`: the working directory is different. And given that
so much in Git's operations depend on the working directory, it makes
sense that those two invocations have slightly different semantics, too.

I also agree, obviously, that this is something that would need a separate
patch series to tackle ;-)

Ciao,
Dscho

Re: [PATCH v5 00/24] Remove submodule from files-backend.c

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> v5 goes a bit longer than v4, basically:

I've reviewed all of the patches and left a bunch of comments, mostly
superficial. I'm very happy about the way it's going, and especially
want to say how much I appreciate that you've done so much legwork and
added so many tests.

Michael

Re: [PATCH v5 23/24] t1405: some basic tests on main ref store

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  t/t1405-main-ref-store.sh (new +x) | 123 
> +
>  1 file changed, 123 insertions(+)
>  create mode 100755 t/t1405-main-ref-store.sh
> 
> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
> new file mode 100755
> index 0..0999829f1
> --- /dev/null
> +++ b/t/t1405-main-ref-store.sh
> @@ -0,0 +1,123 @@
> +#!/bin/sh
> +
> +test_description='test main ref store api'
> +
> +. ./test-lib.sh
> +
> +RUN="test-ref-store main"
> +
> +test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
> + test_commit one &&
> + N=`find .git/refs -type f | wc -l` &&

I think we prefer $() to ``. (Same throughout this file and also t1406
in the next commit.)

It's notable that these tests grep around the filesystem, so they won't
be applicable to future refs backends. Of course, "pack-refs" is
intrinsically only applicable to the files backend, so for this test
it's not surprising. But some tests could conceivably be written in a
generic way, so that they should pass for any refs backend.

Just food for thought; no need to change anything now.

> + test "$N" != 0 &&
> + $RUN pack-refs 3 &&
> + N=`find .git/refs -type f | wc -l`

Didn't you mean to test the final `$N`?

> +'
> +
> +test_expect_success 'peel_ref(new-tag)' '
> + git rev-parse HEAD >expected &&
> + git tag -a -m new-tag new-tag HEAD &&
> + $RUN peel-ref refs/tags/new-tag >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'create_symref(FOO, refs/heads/master)' '
> + $RUN create-symref FOO refs/heads/master nothing &&
> + echo refs/heads/master >expected &&
> + git symbolic-ref FOO >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
> + git rev-parse FOO -- &&
> + git rev-parse refs/tags/new-tag -- &&
> + $RUN delete-refs 0 FOO refs/tags/new-tag &&
> + test_must_fail git rev-parse FOO -- &&
> + test_must_fail git rev-parse refs/tags/new-tag --
> +'
> +
> +test_expect_success 'rename_refs(master, new-master)' '
> + git rev-parse master >expected &&
> + $RUN rename-ref refs/heads/master refs/heads/new-master &&
> + git rev-parse new-master >actual &&
> + test_cmp expected actual &&
> + test_commit recreate-master
> +'

Isn't HEAD set to `refs/heads/master` prior to this test? If so, then I
think it should become detached when you rename `refs/heads/master`. Or
maybe it should be changed to point at `refs/heads/new-master`, I can't
remember. In either case, it might be useful for the test to check that
the behavior matches the status quo, so we notice if the behavior ever
changes inadvertently.

> +
> +test_expect_success 'for_each_ref(refs/heads/)' '
> + $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
> + cat >expected <<-\EOF &&
> + master 0x0
> + new-master 0x0
> + EOF
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'resolve_ref(new-master)' '
> + SHA1=`git rev-parse new-master` &&
> + echo "$SHA1 refs/heads/new-master 0x0" >expected &&
> + $RUN resolve-ref refs/heads/new-master 0 >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'verify_ref(new-master)' '
> + $RUN verify-ref refs/heads/new-master
> +'
> +
> +test_expect_success 'for_each_reflog()' '
> + $RUN for-each-reflog | cut -c 42- >actual &&
> + cat >expected <<-\EOF &&
> + refs/heads/master 0x0
> + refs/heads/new-master 0x0
> + HEAD 0x1
> + EOF
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'for_each_reflog_ent()' '
> + $RUN for-each-reflog-ent HEAD >actual &&
> + head -n1 actual | grep one &&
> + tail -n2 actual | head -n1 | grep recreate-master
> +'
> +
> +test_expect_success 'for_each_reflog_ent_reverse()' '
> + $RUN for-each-reflog-ent-reverse HEAD >actual &&
> + head -n1 actual | grep recreate-master &&
> + tail -n2 actual | head -n1 | grep one
> +'
> +
> +test_expect_success 'reflog_exists(HEAD)' '
> + $RUN reflog-exists HEAD
> +'
> +
> +test_expect_success 'delete_reflog(HEAD)' '
> + $RUN delete-reflog HEAD &&
> + ! test -f .git/logs/HEAD

Maybe also test the final state using `reflog-exists` to make sure that
function can also return false?

> +'
> +
> +test_expect_success 'create-reflog(HEAD)' '
> + $RUN create-reflog HEAD 1 &&
> + test -f .git/logs/HEAD
> +'
> +
> +test_expect_success 'delete_ref(refs/heads/foo)' '
> + git checkout -b foo &&
> + FOO_SHA1=`git rev-parse foo` &&
> + git checkout --detach &&
> + test_commit bar-commit &&
> + git checkout -b bar &&
> + BAR_SHA1=`git rev-parse bar` &&
> + $RUN update-ref updating refs/heads/foo $BAR_SHA1 $FOO_SHA1 0 &&
> + echo $BAR_SHA1 >expected &&
> + git rev-parse refs/heads/foo >actual &&
> + test_cmp expect

rebase: confusing behaviour since --fork-point

[Laszlo Kiss]

Hi all,

This will be a bit long, sorry in advance.

I've hit a problem with how rebase works since the introduction of the
--fork-point option. I initially thought it was a bug until the kind
folks over at git-for-windows patiently told me otherwise.

Consider the following:

---8<---
# On branch master which is at origin/master
# hack hack hack

git commit -am'First topic commit'
# optionally followed by more commits

# realize I want all this on a different branch instead
# (maybe I just forgot to create one; would be typical)

git checkout -b topic --track
git branch -f master origin/master

# optionally add more commits to 'topic' and/or update 'master'
# with changes to 'origin/master', then finally:

git rebase# or git pull --rebase
--->8---

What happens is that 'First topic commit' is discarded because rebase,
with this syntax, defaults to --fork-point, which then works out from
master's reflog that the commit in question was previously part of the
upstream branch. The rebase is then carried out under the assumption
that, even though that specific commit is no longer in master,
something equivalent to it or superseding it is, or else it is meant to
be dropped - which would likely be the case if a complicated history
rewrite happened, but is not the case here.

There are two reasons why I think this is confusing and should be
changed:

(1) My mental model of git after some 5 years of using it is such that
I view the reflog as auxiliary information for purposes of mining
history or recovering from mistakes, and I rely only on the
contents and parent-child relationships of commits to work out what
a command I'm about to run will do. I would expect the vast
majority of users to have a similar mental model, in which case the
fact that the same commits can be rebased differently depending on
the data in the reflog will be equally confusing to those users.

(2) Most people don't like to rebase too often, consequently the time
elapsed from a user creating the topic branch and resetting its
upstream until actually rebasing it can be days or weeks, by the
end of which they are unlikely to remember the circumstances of
creating the branch. Even more alarmingly, they may have committed
dozens of further changes, and therefore may not even notice that
the first few of those silently disappeared. (I'm not making this
up: see discussion links below in PS.)

I believe the correct design would be to always make --no-fork-point
the default for rebase, and only use --fork-point when explicitly
specified. This would potentially be inconvenient for those who rebase
on top of complicated history rewrites, but said inconvenience would be
mitigated by several factors:
- Anyone doing rebases like that will already know that they can go
  wrong in a million ways.
- Perhaps more crucially, they will have a way of noticing if it went
  wrong, with all relevant information in short-term memory, so would
  recover easily.
- If the rebase turns out to be really complex, they are likely to
  resort to rebase -i, which shows full details of what is about to
  happen, moreover it seems relatively simple to enhance the contents
  of the rebase-todo file so that full information about the merge-base
  and fork-point is readily available.

If changing the default is not an option, e.g. because of backwards
compatibility concerns, then some configurability could still be
helpful, e.g. rebase.useForkPoint = never / auto / always (default
auto, to keep the current behaviour). Although I suspect this would
just lead to everyone suggesting setting this to 'never'. In any case
this would provide a way to ensure that any git newbies in my
organization don't spend days trying to figure this out like I've just
done.

Assuming there is agreement to do one of the above (I don't even know
whose agreement is required), what's the process for getting it
implemented? Sorry, that probably counts as a dumb question, but I've
never been around open-source projects much & need someone to show me
the ropes.

Many thanks
Laszlo

PS. Further reading about the same topic if anyone is interested:
- http://marc.info/?l=git&m=140991293402880&w=2 (from this same mailing
  list 2+ years ago, but I can see no clear conclusion)
- https://github.com/git-for-windows/git/issues/1076 (my bug report
  where contacting this list was suggested)
- http://stackoverflow.com/questions/22790765 and
  http://stackoverflow.com/questions/35320740 (various SO users being
  confused and asking about / discussing the same thing)

bisect-helper: we do not bisect --objects

[Junio C Hamano]

Ever since "bisect--helper" was introduced in 1bf072e366
("bisect--helper: implement "git bisect--helper"", 2009-03-26),
after setting up the "rev-list $bad --not $good_ones" machinery, the
code somehow prepared to mark the trees and blobs at the good boundary
as uninteresting, only when --objects option was given.  This was kept
across a bit of refactoring done by 2ace9727be ("bisect: move common
bisect functionality to "bisect_common"", 2009-04-19) and survives
to this day.

However, "git bisect" does not care about tree/blob object
reachability at all---it purely works at the commit DAG level and
nobody passes (and nobody should pass) "--objects" option to the
underlying rev-list machinery.  Remove the dead code.

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

 * Christian, do you recall what we were thinking when we added this
   mark_edges_uninteresting() call in this program?  If you don't,
   don't worry--this was done more than 8 years ago.  I am just
   being curious and also a bit being cautious in case I am missing
   something.

   Thanks.

 bisect.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/bisect.c b/bisect.c
index 30808cadf7..86c5929a23 100644
--- a/bisect.c
+++ b/bisect.c
@@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)
 {
if (prepare_revision_walk(revs))
die("revision walk setup failed");
-   if (revs->tree_objects)
-   mark_edges_uninteresting(revs, NULL);
 }
 
 static void exit_if_skipped_commits(struct commit_list *tried,

Re: log -S/-G (aka pickaxe) searches binary files by default

[Junio C Hamano]

Jeff King  writes:

> On Thu, Mar 02, 2017 at 05:36:17PM -0800, Junio C Hamano wrote:
> ...
>> > Is that on purpose?
>> 
>> No, it's a mere oversight (as I do not think I never even thought
>> about special casing binary
>> files from day one, it is unlikely that you would find _any_ old
>> version of Git that behaves
>> differently).
>
> The email focuses on "-G", and I think it is wrong to look in binary
> files there, as "grep in diff" does not make sense for a binary file
> that we would refuse to diff.

Yeah, I agree.

> But the subject also mentions "-S". I always assumed it was intentional
> to look in binary files there, as it is searching for a pure byte
> sequence. I would not mind an option to disable that, but I think the
> default should remain on.

As the feature was built to be one of the core ingredients necessary
towards the 'ideal SCM' envisioned in

  

"-S" is about finding "a block of text". It was merely an oversight
that we didn't add explicit code to ignore binary when we introduced
the concept of "is this text?  is it worth finding things in and
diffing binary files?".

I do agree that it may be too late and/or disruptive to change its
behaviour now, as people may grew expectations different from the
original motivation and design, though.

Re: [PATCH v5 20/24] files-backend: avoid ref api targetting main ref store

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> A small step towards making files-backend works as a non-main ref store
> using the newly added store-aware API.
> 
> For the record, `join` and `nm` on refs.o and files-backend.o tell me
> that files-backend no longer uses functions that defaults to
> get_main_ref_store().

Nice!

> I'm not yet comfortable at the idea of removing
> files_assert_main_repository() (or converting REF_STORE_MAIN to
> REF_STORE_WRITE). More staring and testing is required before that can
> happen. Well, except peel_ref(). I'm pretty sure that function is safe.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 85 
> ++--
>  1 file changed, 49 insertions(+), 36 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index dafddefd3..09c280fd3 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -3873,8 +3883,10 @@ static int files_transaction_commit(struct ref_store 
> *ref_store,
>* head_ref within the transaction, then split_head_update()
>* arranges for the reflog of HEAD to be updated, too.
>*/
> - head_ref = resolve_refdup("HEAD", RESOLVE_REF_NO_RECURSE,
> -   head_oid.hash, &head_type);
> + head_ref = (char *)refs_resolve_ref_unsafe(ref_store, "HEAD",
> +RESOLVE_REF_NO_RECURSE,
> +head_oid.hash, &head_type);
> + head_ref = xstrdup_or_null(head_ref);

If you combine the last two statements, you can avoid the ugly cast.

> [...]

Michael

Re: [PATCH 3/3] Remove outdated info in difftool manpage

[Johannes Schindelin]

Hi Denton (or should I address you as Liu?),

On Fri, 3 Mar 2017, Denton Liu wrote:

> When difftool was rewritten in C, it removed the capability to read
> fallback configs from mergetool. This changes the documentation to
> reflect this.

Thanks for pointing that out. But that is probably an oversight on my
part, not an intentional change...

Ciao,
Johannes

Re: [PATCH v5 19/24] refs: new transaction related ref-store api

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> The transaction struct now takes a ref store at creation and will
> operate on that ref store alone.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.c   | 54 
> 
>  refs.h   |  8 
>  refs/refs-internal.h |  1 +
>  3 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 9137ac283..851b5e125 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -618,16 +618,19 @@ static int delete_pseudoref(const char *pseudoref, 
> const unsigned char *old_sha1
>   return 0;
>  }
>  
> -int delete_ref(const char *refname, const unsigned char *old_sha1,
> -unsigned int flags)
> +int refs_delete_ref(struct ref_store *refs, const char *refname,
> + const unsigned char *old_sha1,
> + unsigned int flags)
>  {
>   struct ref_transaction *transaction;
>   struct strbuf err = STRBUF_INIT;
>  
> - if (ref_type(refname) == REF_TYPE_PSEUDOREF)
> + if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> + assert(refs == get_main_ref_store());

Hmmm, I would have expected this to be checked via `(refs.flags &
REF_STORE_MAIN)`. I guess this is temporary; once we have compound ref
stores either version will need to be changed again. So I don't see a
strong argument for one vs. the other.

It might be more natural to make `delete_pseudoref()` take a `refs`
argument and do the check internally.

(Same comments below where `write_pseudoref()` is called.)

Michael

>   return delete_pseudoref(refname, old_sha1);
> + }
>  
> - transaction = ref_transaction_begin(&err);
> + transaction = ref_store_transaction_begin(refs, &err);
>   if (!transaction ||
>   ref_transaction_delete(transaction, refname, old_sha1,
>  flags, NULL, &err) ||
> @@ -642,6 +645,13 @@ int delete_ref(const char *refname, const unsigned char 
> *old_sha1,
>   return 0;
>  }
>  
> +int delete_ref(const char *refname, const unsigned char *old_sha1,
> +unsigned int flags)
> +{
> + return refs_delete_ref(get_main_ref_store(), refname,
> +old_sha1, flags);
> +}
> +
>  int copy_reflog_msg(char *buf, const char *msg)
>  {
>   char *cp = buf;
> @@ -801,11 +811,20 @@ int read_ref_at(const char *refname, unsigned int 
> flags, unsigned long at_time,
>   return 1;
>  }
>  
> -struct ref_transaction *ref_transaction_begin(struct strbuf *err)
> +struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
> + struct strbuf *err)
>  {
> + struct ref_transaction *tr;
>   assert(err);
>  
> - return xcalloc(1, sizeof(struct ref_transaction));
> + tr = xcalloc(1, sizeof(struct ref_transaction));
> + tr->ref_store = refs;
> + return tr;
> +}
> +
> +struct ref_transaction *ref_transaction_begin(struct strbuf *err)
> +{
> + return ref_store_transaction_begin(get_main_ref_store(), err);
>  }
>  
>  void ref_transaction_free(struct ref_transaction *transaction)
> @@ -922,18 +941,20 @@ int update_ref_oid(const char *msg, const char *refname,
>   old_oid ? old_oid->hash : NULL, flags, onerr);
>  }
>  
> -int update_ref(const char *msg, const char *refname,
> -const unsigned char *new_sha1, const unsigned char *old_sha1,
> -unsigned int flags, enum action_on_err onerr)
> +int refs_update_ref(struct ref_store *refs, const char *msg,
> + const char *refname, const unsigned char *new_sha1,
> + const unsigned char *old_sha1, unsigned int flags,
> + enum action_on_err onerr)
>  {
>   struct ref_transaction *t = NULL;
>   struct strbuf err = STRBUF_INIT;
>   int ret = 0;
>  
>   if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> + assert(refs == get_main_ref_store());
>   ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
>   } else {
> - t = ref_transaction_begin(&err);
> + t = ref_store_transaction_begin(refs, &err);
>   if (!t ||
>   ref_transaction_update(t, refname, new_sha1, old_sha1,
>  flags, msg, &err) ||
> @@ -964,6 +985,15 @@ int update_ref(const char *msg, const char *refname,
>   return 0;
>  }
>  
> +int update_ref(const char *msg, const char *refname,
> +const unsigned char *new_sha1,
> +const unsigned char *old_sha1,
> +unsigned int flags, enum action_on_err onerr)
> +{
> + return refs_update_ref(get_main_ref_store(), msg, refname, new_sha1,
> +old_sha1, flags, onerr);
> +}
> +
>  char *shorten_unambiguous_ref(const char *refname, int strict)
>  {
>   int i;
> @@ -1600,7 +1630,7 @@ int create_symref(const char *ref_target, const char 
> *refs_heads

Re: [PATCH v2] Travis: also test on 32-bit Linux

[Johannes Schindelin]

Hi Lars,

On Fri, 3 Mar 2017, Lars Schneider wrote:

> From: Johannes Schindelin 
> 
> When Git v2.9.1 was released, it had a bug that showed only on Windows
> and on 32-bit systems: our assumption that `unsigned long` can hold
> 64-bit values turned out to be wrong.
> 
> This could have been caught earlier if we had a Continuous Testing
> set up that includes a build and test run on 32-bit Linux.
> 
> Let's do this (and take care of the Windows build later). This patch
> asks Travis CI to install a Docker image with 32-bit libraries and then
> goes on to build and test Git using this 32-bit setup.
> 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Lars Schneider 

Thanks for keeping the ball rolling! I am totally fine with v2.

Ciao,
Dscho

Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts

[Marc Branchaud]

On 2017-03-03 05:57 AM, Sebastian Schuberth wrote:

It does not make sense for these placeholder scripts to depend on Python
just because the real scripts do. At the example of Git for Windows, we
would not even be able to see those warnings as it does not ship with
Python. So just use plain shell scripts instead.


Just a niggle:  This change moves the warning message from stderr to stdout.

M.


Signed-off-by: Sebastian Schuberth 
---
 contrib/remote-helpers/git-remote-bzr | 16 +++-
 contrib/remote-helpers/git-remote-hg  | 16 +++-
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 712a137..ccc4aea 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh

-import sys
-
-sys.stderr.write('WARNING: git-remote-bzr is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit 
https://github.com/felipec/git-remote-bzr\n')
-
-sys.stderr.write('''WARNING:
+cat <<'EOT'
+WARNING: git-remote-bzr is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-bzr
+WARNING:
 WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING:   $ wget -O $HOME/bin/git-remote-bzr \\
+WARNING:   $ wget -O $HOME/bin/git-remote-bzr \
 WARNING: 
https://raw.github.com/felipec/git-remote-bzr/master/git-remote-bzr
 WARNING:   $ chmod +x $HOME/bin/git-remote-bzr
-''')
+EOT
diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 4255ad6..dfda44f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh

-import sys
-
-sys.stderr.write('WARNING: git-remote-hg is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit 
https://github.com/felipec/git-remote-hg\n')
-
-sys.stderr.write('''WARNING:
+cat <<'EOT'
+WARNING: git-remote-hg is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-hg
+WARNING:
 WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING:   $ wget -O $HOME/bin/git-remote-hg \\
+WARNING:   $ wget -O $HOME/bin/git-remote-hg \
 WARNING: https://raw.github.com/felipec/git-remote-hg/master/git-remote-hg
 WARNING:   $ chmod +x $HOME/bin/git-remote-hg
-''')
+EOT

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

Finding a tag that introduced a submodule change

[Robert Dailey]

I have a repository with a single submodule in it. Since the parent
repository represents the code base for an actual product, I tag
release versions in the parent repository. I do not put tags in the
submodule since multiple other products may be using it there and I
wanted to avoid ambiguous tags.

Sometimes I run into a situation where I need to find out which
release of the product a submodule change was introduced in. This is
nontrivial, since there are no tags in the submodule itself. This is
one thing I tried:

1. Do a `git log` in the submodule to find the SHA1 representing the
change I want to check for
2. In the parent repository, do a git log with pickaxe to determine
when the submodule itself changed to the value of that SHA1.
3. Based on the result of #2, do a `git tag --contains` to see the
lowest-version tag that contains the SHA1, which will identify the
first release that introduced that change

However, I was not able to get past #2 because apparently there are
cases where when we move the submodule "forward", we skip over
commits, so the value of the submodule itself never was set to that
SHA1.

I'm at a loss here on how to easily do this. Can someone recommend a
way to do this? Obviously the easier the better, as I have to somehow
train my team how to do this on their own.

Thanks in advance.

Re: [PATCH v2 2/9] setup_git_directory(): use is_dir_sep() helper

[Johannes Schindelin]

Hi Peff,

On Fri, 3 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 12:16:31PM +0100, Johannes Schindelin wrote:
> 
> > > What is "dir"? I'm guessing this patch got reordered and it should
> > > stay as cwd.buf?
> > 
> > Oh drats. Usually I do a final `git rebase -x "make test"
> > upstream/master` run before submitting, but I was really, really tired
> > by the end of that stretch.
> 
> I usually do the same, and have done the "too tired" thing, too, only to
> have it bite me. That's why I so readily recognized the problem. :)

:-)

> I've recently switched to using Michael's "git test" program[1], which
> caches the test results for each tree in a git-note. That makes the
> final "rebase -x" a lot less painful if you've left the early commits
> alone.

Good point. I meant to have a look, but got really overwhelmed with other
things.

> The python dependency might be a blocker for you, but I suspect the
> caching parts would be easy to hack together with shell.

No, personally I have no problem with Python. If you asked me to include
it in Git for Windows' installer, increasing the size noticably, that
would be a totally different matter, of course.

Ciao,
Dscho

Re: [PATCH v2 0/9] Fix the early config

[Johannes Schindelin]

Hi Peff,

On Fri, 3 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:03:56AM +0100, Johannes Schindelin wrote:
> 
> > These patches are an attempt to make Git's startup sequence a bit less
> > surprising.
> > 
> > The idea here is to discover the .git/ directory gently (i.e. without
> > changing the current working directory, or global variables), and to
> > use it to read the .git/config file early, before we actually called
> > setup_git_directory() (if we ever do that).
> 
> Thanks for working on this. I think it's a huge improvement over the
> status quo, and over the earlier attempt. I don't see anything hugely
> wrong with this series, though I did note one bug, along with some minor
> refinements.

Thank you very much for your review. You did point out a couple of things
I overlooked, and while it was a pain in the back to go from v1 to v2
(i.e. avoiding code duplication, which immediately put the changed code to
a huge test by having the very central setup_git_directory() use it), I
really believe that we are better off because the patch series pays off
more technical debt now than it introduces.

> > My dirty little secret is that I actually need this for something else
> > entirely. I need to patch an internal version of Git to gather
> > statistics, and to that end I need to read the config before and after
> > running every Git command. Hence the need for a gentle, and correct
> > early config.
> 
> We do something similar at GitHub, but it falls into two categories:
> 
>   - stat-gathering that's on all the time, so doesn't need to look at
> config (I'm not sure in your case if you want to trigger the
> gathering with config, or if config is just one of the things you
> are gathering).
> 
>   - logging that is turned on selectively for some repos, but which
> doesn't have to be looked up until we know we are in a repo

You probably guessed what I need to do, anyway: for our GVFS usage, we
need some telemetry (i.e. usage statistics), and I basically want to run a
hook whenever any Git command is called, but only on GVFS-enabled
repositories.

So it is somewhat related, but slightly different from your usage.

> I looked into making something upstream-able, and my approach was to
> allow GIT_TRACE_* variables to be specified in the config. But of course
> that ran into the early-config problem (and I really wanted repo-level
> config there, because I'd like to be able to turn on tracing for just a
> single problematic repo).
> 
> So not really something you need to work on, but maybe food for thought
> as you work on your internal project.

Right, GIT_TRACE_* settings in the config should be doable now.

Ciao,
Dscho

Re: [PATCH v2 9/9] Test read_early_config()

[Johannes Schindelin]

Hi Peff,

On Fri, 3 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:36AM +0100, Johannes Schindelin wrote:
> 
> > So far, we had no explicit tests of that function.
> 
> Makes sense. The pager tests fixed in an earlier commit were effectively
> checking those, but I don't mind making it explicit.

Well, TBH I am a bit uncomfortable with t7006 doing those tests. Just
imagine that something breaks in that script, say, when working on
exporting the read_early_config() function. You probably see this coming:
debugging those breakages is half a nightmare. There are multiple levels
of shell script functions, a Perl script, and an isatty() call between the
bug hunter and the bug.

With the new tests, it all becomes much more straight-forward to debug.
And also less surprising (think about the fun you can have with test cases
that fail when running `make t7006-pager.sh` but not when running `bash
t7006-pager.sh -i -v -x`, just because of a forgotten `test_terminal`...).

And yes, you can debug t7006 with the good old "insert debug print
statements here and there, then compile and run, rinse & repeat" method.
But you know, getting to use a real IDE with real debugger/intellisense
integration after years of working on C code in the terminal with vim and
gdb [*1*], I kinda feel a bit pampered and ask myself how I could possibly
have put up with the awkwardness. ;-)

Ciao,
Dscho

Footnote *1*: for some years I developed Java code in Eclipse and already
then did I notice just how much faster development becomes when you have
powerful tools to help you...

Re: SHA1 collisions found

[Ian Jackson]

Jeff King writes ("Re: SHA1 collisions found"):
> I think you've read more into my "conversion" than I intended. The old
> history won't get rewritten. It will just be grafted onto the bottom of
> the commit history you've got, and the new trees will all be written
> with the new hash.
> 
> So you still have those old objects hanging around that refer to things
> by their sha1 (not to mention bug trackers, commit messages, etc, which
> all use commit ids). And you want to be able to quickly resolve those
> references.
> 
> What _does_ get rewritten is what's in your ref files, your pack .idx,
> etc. Those are all sha256 (or whatever), and work as sha1's do now.

This all sounds very similar to my proposal.

> Looking up a sha1 reference from an old object just goes through the
> extra level of indirection.

I don't understand why this is a level of indirection, rather than
simply a retention of the existing SHA-1 object database (in parallel,
but deprecated).

Perhaps I have misunderstood what you mean by "graft".  I assume you
don't mean info/grafts, because that is not conveyed by transfer
protocols.


Stepping back a bit, the key question is what the data structure will
look like after the transition.

Specifically, the parent reference in the first post-transition commit
has to refer to something.  What does it refer to ?  The possibilities
seem to be:

 1a. It names the SHA1 hash of an old commit object
 1b. It names the BLAKE[1] hash of an old commit object, which
object of course refers to its parents by SHA1.

 2. It names the BLAKE hash of a translated version of the
old commit object.

 3. It doesn't name the parent, and the old history is not
automatically transferred by clone and not readily accessible.

(1a) and (1b) are different variants of something like my mixed hash
proposal.  Old and new hashes live side by side.

(2) involves rewriting all of the old history, to recursively generate
new objects (with BLAKE names, and which in turn refer to other
rewritten old objects by BLAKE names).  The first time a particular
tree needs to look up an object by a BLAKE name, it needs to run a
conversion its own entire existing history.

For (2) there would have to be some kind of mapping table in every
tree, which allowed object names to be maped in both directions.  The
object content translation would have to be reversible, so that the
actual pre-translation objects would not need to be stored; rather
they would be reconstructed from the post-translation objects, when
someone asks for a pre-translation object.  In principle it would be
possible to convert future BLAKE commits to SHA-1 ones, again by
recursive rewriting.

I don't think anyone is seriously suggesting (3).


So there is a choice between:

(1) a unified hash tree containing a mixture of different hashes at
different reference points, where each object has one identity and one
name.

(2) parallel hash tree structures, each using only a single hash, with
at least every old object present in both tree structures.

I think (1) is preferable because it provides, to callers of git, the
existing object naming semantics.  Callers need to be taught to accept
an extension to the object name format.  Existing object names stored
elsewhere than in git remain valid.

Conversely, (2) requires many object names stored elsewhere than in
git to be updated.  It's possible with (2) to do ad-hoc lookups on
object names in mailing list messages or commit messages and so on.
Even if it is possible for the new git to answer questions like "does
this new branch with BLAKE hash X' contain the commit with SHA1 hash
Y" by implicitly looking up the corresponding BLAKE commit Y' and
answering the question with reference to Y', this isn't going to help
if external code does things like "have git compute the merge base of
X and Y' and check that it is equal to Z".  Either the external
database's record of Z would have to be changed to refer to Z', or the
external code would have to be taught to apply an object name
normalisation operation to turn Z into Z' each time.

Also, (2) has trouble with shallow clones.  This is because it's not
possible to translate old objects to new ones without descending to
the roots of the object tree and recursively translating everything
(or looking up the answer of a previous translation).


Then there is the question of naming syntax.

The distinction between (1) single unified mixed hash tree, and
(2) multiple parallel homogenous hash trees, is mostly orthogonal to
the command-line (and in-commit-object etc.) representation of new
hashes.

The main thing here is that, regardless of the choice between (1) or
(2), we need to choose whether object names specified on the git
command line, and printed by normal git commands, explicitly identify
the hash function.

I think there are roughly three options:

 (A) Decorate all new hashes with a hash function indication
 (sha256: or blake_ or H)

 (B) Infer the hash function f

Re: [PATCH v2 6/9] read_early_config(): special-case builtins that create a repository

[Johannes Schindelin]

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:24AM +0100, Johannes Schindelin wrote:
> 
> > When the command we are about to execute wants to create a repository
> > (i.e. the command is `init` or `clone`), we *must not* look for a
> > repository config.
> 
> Hmm. I'm not sure if this one is worth the hackery here.

I guess you're right. Let me think about this for a while in the back of
my head while I work on other parts of the patch series.

Ciao,
Dscho

Re: [PATCH v5 16/24] files-backend: replace submodule_allowed check in files_downcast()

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> files-backend.c is unlearning submodules. Instead of having a specific
> check for submodules to see what operation is allowed, files backend
> now takes a set of flags at init. Each operation will check if the
> required flags is present before performing.
> 
> For now we have four flags: read, write and odb access. Main ref store
> has all flags, obviously, while submodule stores are read-only and have
> access to odb (*).

I'm surprised that you have implemented a "read" flag. Do you expect
ever to support ref-stores that are unreadable? In other words, couldn't
"read" just be assumed for any ref store?

> The "main" flag stays because many functions in the backend calls
> frontend ones without a ref store, so these functions always target the
> main ref store. Ideally the flag should be gone after ref-store-aware
> api is in place and used by backends.
> 
> (*) Submodule code needs for_each_ref. Try take REF_STORE_ODB flag
> out. At least t3404 would fail. The "have access to odb" in submodule is
> a bit hacky since we don't know from he whether add_submodule_odb() has
> been called.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.c   | 15 ++---
>  refs/files-backend.c | 86 
> +---
>  refs/refs-internal.h |  9 +-
>  3 files changed, 73 insertions(+), 37 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 67acae60c..2dc97891a 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -1821,10 +1829,14 @@ static enum peel_status peel_entry(struct ref_entry 
> *entry, int repeel)
>  static int files_peel_ref(struct ref_store *ref_store,
> const char *refname, unsigned char *sha1)
>  {
> - struct files_ref_store *refs = files_downcast(ref_store, 0, "peel_ref");
> + struct files_ref_store *refs =
> + files_downcast(ref_store, REF_STORE_READ | REF_STORE_ODB,
> +"peel_ref");
>   int flag;
>   unsigned char base[20];
>  
> + files_assert_main_repository(refs, "peel_ref");

Instead of this call, couldn't you add `REF_STORE_MAIN` to the flags
passed to `files_downcase()`?

> +
>   if (current_ref_iter && current_ref_iter->refname == refname) {
>   struct object_id peeled;
>  
> [...]

Michael

Re: [PATCH v2 4/9] Export the discover_git_directory() function

[Johannes Schindelin]

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:15AM +0100, Johannes Schindelin wrote:
> 
> > diff --git a/cache.h b/cache.h
> > index 80b6372cf76..a104b76c02e 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
> >  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
> >  
> >  extern void setup_work_tree(void);
> > +extern const char *discover_git_directory(struct strbuf *gitdir);
> 
> Perhaps it's worth adding a short docstring describing the function.

Okay.

> > +const char *discover_git_directory(struct strbuf *gitdir)
> > +{
> > +   struct strbuf dir = STRBUF_INIT;
> > +   int len;
> 
> Nit: please use size_t for storing strbuf lengths.

Okay.

> > +   if (strbuf_getcwd(&dir))
> > +   return NULL;
> > +
> > +   len = dir.len;
> > +   if (discover_git_directory_1(&dir, gitdir) < 0) {
> > +   strbuf_release(&dir);
> > +   return NULL;
> > +   }
> > +
> > +   if (dir.len < len && !is_absolute_path(gitdir->buf)) {
> > +   strbuf_addch(&dir, '/');
> > +   strbuf_insert(gitdir, 0, dir.buf, dir.len);
> > +   }
> > +   strbuf_release(&dir);
> 
> I was confused by two things here.
> 
> One is that because I was wondering whether "gitdir" was supposed to be
> passed empty or not, it wasn't clear that this insert is doing the right
> thing.  If there _is_ something in it, then discover_git_directory_1()
> would just append to it, which makes sense. But then this insert blindly
> sticks the absolute-path bit at the front of the string, leaving
> whatever was originally there spliced into the middle of the path.
> Doing:
> 
>   size_t base = gitdir->len;
>   ...
>   strbuf_insert(gitdir, base, dir.buf, dir.len);
> 
> would solve that.

And of course the is_absolute_path() call also needs to offset `gitdir->buf
+ base`.

> It's probably not that likely for somebody to do:
> 
>   strbuf_addstr(&buf, "my git dir is ");
>   discover_git_directory(&buf);
> 
> but since it's not much effort, it might be worth making it work.

Plus, I have no assert()s in place to ensure any expectation to the
contrary. So I fixed it as you suggested.

> The second is that I don't quite understand why we only make the result
> absolute when we walked upwards. In git.git, the result of the function
> in various directories is:
> 
>   - ".git", when we're at the root of the worktree
>   - "/home/peff/git/.git, when we're in "t/"
>   - "." when we're already in ".git"
>   - "/home/peff/git/.git/." when we're in ".git/objects"
> > I'm not sure if some of those cases are not behaving as intended, or
> there's some reason for the differences that I don't quite understand.

Yes, some of those cases do not behave as intended: it is true that
setup_git_directory() sets git_dir to "/home/peff/git/.git" when we
(actually, you, given that my home directory is different) are in "t/",
but setup_git_directory_gently_1() would set gitdir to ".git" and dir to
"/home/peff/git". But the current directory is still what cwd says it is:
"/home/peff/git/t".

I added a comment:

/*
 * The returned gitdir is relative to dir, and if dir does not reflect
 * the current working directory, we simply make the gitdir
 * absolute.
 */

And thanks: you reminded me of another issue I wanted to address but
forgot: if gitdir is ".", I do not want the resulting absolute path to
have a trailing "/.". I fixed that, too.

Ciao,
Dscho

[PATCH v2 4/5] Add guitool completions for diff and mergetools

[Denton Liu]

Signed-off-by: Denton Liu 
---
 contrib/completion/git-completion.bash | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 41ee52991..c94e38a3a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1277,7 +1277,7 @@ _git_difftool ()
--base --ours --theirs
--no-renames --diff-filter= --find-copies-harder
--relative --ignore-submodules
-   --tool="
+   --tool= --gui"
return
;;
esac
@@ -1584,7 +1584,7 @@ _git_mergetool ()
return
;;
--*)
-   __gitcomp "--tool= --prompt --no-prompt"
+   __gitcomp "--tool= --prompt --no-prompt --gui"
return
;;
esac
@@ -2207,6 +2207,7 @@ _git_config ()
diff.submodule
diff.suppressBlankEmpty
diff.tool
+   diff.guitool
diff.wordRegex
diff.algorithm
difftool.
@@ -2308,6 +2309,7 @@ _git_config ()
merge.renormalize
merge.stat
merge.tool
+   merge.guitool
merge.verbosity
mergetool.
mergetool.keepBackup
-- 
2.12.0.5.gfbc750a84

Re: [PATCH v5 15/24] refs: move submodule code out of files-backend.c

[Michael Haggerty]

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> files-backend is now initialized with a $GIT_DIR. Converting a submodule
> path to where real submodule gitdir is located is done in get_ref_store().
> 
> This gives a slight performance improvement for submodules since we
> don't convert submodule path to gitdir at every backend call like
> before. We pay that once at ref-store creation.
> 
> More cleanup in files_downcast() and files_assert_main_repository()
> follows shortly. It's separate to keep noises from this patch.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.c   | 20 ++--
>  refs/files-backend.c | 24 ++--
>  refs/refs-internal.h |  9 -
>  3 files changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 562834fc0..67acae60c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -9,6 +9,7 @@
>  #include "refs/refs-internal.h"
>  #include "object.h"
>  #include "tag.h"
> +#include "submodule.h"
>  
>  /*
>   * List of all available backends
> @@ -1413,9 +1414,9 @@ static struct ref_store 
> *lookup_submodule_ref_store(const char *submodule)
>  
>  /*
>   * Create, record, and return a ref_store instance for the specified
> - * submodule (or the main repository if submodule is NULL).
> + * gitdir.
>   */
> -static struct ref_store *ref_store_init(const char *submodule)
> +static struct ref_store *ref_store_init(const char *gitdir)
>  {
>   const char *be_name = "files";
>   struct ref_storage_be *be = find_ref_storage_backend(be_name);
> @@ -1424,7 +1425,7 @@ static struct ref_store *ref_store_init(const char 
> *submodule)
>   if (!be)
>   die("BUG: reference backend %s is unknown", be_name);
>  
> - refs = be->init(submodule);
> + refs = be->init(gitdir);
>   return refs;
>  }
>  
> @@ -1435,7 +1436,7 @@ struct ref_store *get_main_ref_store(void)
>   if (main_ref_store)
>   return main_ref_store;
>  
> - refs = ref_store_init(NULL);
> + refs = ref_store_init(get_git_dir());
>   if (refs) {
>   if (main_ref_store)
>   die("BUG: main_ref_store initialized twice");
> @@ -1466,6 +1467,7 @@ struct ref_store *get_ref_store(const char *submodule)
>  {
>   struct strbuf submodule_sb = STRBUF_INIT;
>   struct ref_store *refs;
> + int ret;
>  
>   if (!submodule || !*submodule) {
>   return get_main_ref_store();
> @@ -1476,8 +1478,14 @@ struct ref_store *get_ref_store(const char *submodule)
>   return refs;
>  
>   strbuf_addstr(&submodule_sb, submodule);
> - if (is_nonbare_repository_dir(&submodule_sb))
> - refs = ref_store_init(submodule);
> + ret = is_nonbare_repository_dir(&submodule_sb);
> + strbuf_release(&submodule_sb);
> + if (!ret)
> + return refs;

`refs` is always NULL here, right? Then it would be more transparent to
return NULL. Or maybe use the `goto cleanup` pattern, which makes it
clearer which are error-handling paths (and lets you get avoid the
temporary variable `ret`)).

> +
> + ret = submodule_to_gitdir(&submodule_sb, submodule);
> + if (!ret)
> + refs = ref_store_init(submodule_sb.buf);
>   strbuf_release(&submodule_sb);
>  
>   if (refs)
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d80c27837..37443369b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -917,12 +917,6 @@ struct packed_ref_cache {
>  struct files_ref_store {
>   struct ref_store base;
>  
> - /*
> -  * The name of the submodule represented by this object, or
> -  * NULL if it represents the main repository's reference
> -  * store:
> -  */
> - const char *submodule;
>   char *gitdir;
>   char *gitcommondir;
>   char *packed_refs_path;
> @@ -982,22 +976,14 @@ static void clear_loose_ref_cache(struct 
> files_ref_store *refs)
>   * Create a new submodule ref cache and add it to the internal
>   * set of caches.
>   */
> -static struct ref_store *files_ref_store_create(const char *submodule)
> +static struct ref_store *files_ref_store_create(const char *gitdir)
>  {
>   struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
>   struct ref_store *ref_store = (struct ref_store *)refs;
>   struct strbuf sb = STRBUF_INIT;
> - const char *gitdir = get_git_dir();
>  
>   base_ref_store_init(ref_store, &refs_be_files);
>  
> - if (submodule) {
> - refs->submodule = xstrdup(submodule);
> - refs->packed_refs_path = git_pathdup_submodule(
> - refs->submodule, "packed-refs");
> - return ref_store;
> - }
> -
>   refs->gitdir = xstrdup(gitdir);
>   get_common_dir_noenv(&sb, gitdir);
>   refs->gitcommondir = strbuf_detach(&sb, NULL);
> @@ -1014,8 +1000,7 @@ static struct ref_store *files_ref_store_create(const 
> char *submodule)
>  static void files_assert_main_repository(

Re: [PATCH v2 5/9] Make read_early_config() reusable

[Johannes Schindelin]

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:20AM +0100, Johannes Schindelin wrote:
> 
> > The pager configuration needs to be read early, possibly before
> > discovering any .git/ directory.
> > 
> > Let's not hide this function in pager.c, but make it available to other
> > callers.
> > [...]
> > +* Note that this is a really dirty hack that does the wrong thing in
> > +* many cases. The crux of the problem is that we cannot run
> 
> Makes sense. I'll assume the words "dirty hack" disappear from this
> now-public function as you fix it up in a future patch. :)

Oops. I did not even think about that.

Fixed,
Dscho

Re: [PATCH v2 3/9] setup_git_directory(): avoid changing global state during discovery

[Johannes Schindelin]

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:11AM +0100, Johannes Schindelin wrote:
> 
> > For historical reasons, Git searches for the .git/ directory (or the
> > .git file) by changing the working directory successively to the
> > parent directory of the current directory, until either anything was
> > found or until a ceiling or a mount point is hit.
> 
> This is starting to get into the meat of changes we've been putting off
> writing for ages. I'll read with my fingers crossed. :)

Heh.

> > Further global state may be changed, depending on the actual type of
> > discovery, e.g. the global variable
> > `repository_format_precious_objects` is set in the
> > `check_repository_format_gently()` function (which is a bit
> > surprising, given the function name).
> 
> It's gentle in the sense that if it does not find a valid repo, it
> touches no state. I do suspect the functions you want are the ones it
> builds on: read_repository_format() and verify_repository_format(),
> which I added not too long ago for the exact purpose of checking repo
> config without touching anything global.

Okay. But I think that my interpretation of the word "gently" is as valid
as Git's source code's.

> > We do have a use case where we would like to find the .git/ directory
> > without having any global state touched, though: when we read the early
> > config e.g. for the pager or for alias expansion.
> > 
> > Let's just rename the function `setup_git_directory_gently_1()` to
> > `discover_git_directory()` and move all code that changes any global
> > state back into `setup_git_directory_gently()`.
> 
> Given the earlier paragraph, it sounds like you want to move the
> global-state-changing parts out of check_repository_format_gently(). But
> that wouldn't be right; it's triggered separate from the discovery
> process by things like enter_repo().

Oh, right. I really only meant to move the global-state-changing parts out
of the discover_git_directory().

> However, I don't see that happening in the patch, which is good. I just
> wonder if the earlier paragraph should really be complaining about how
> setup_git_directory() (and its callees) touches a lot of global state,
> not check_repository_format_gently(), whose use is but one of multiple
> global-state modifications.

Okay, I'll try my best to rephrase the commit message.

For good measure, I will also keep the name setup_git_directory_gently_1()
because it won't get exported directly (I made up my mind about wrapping
that function to allow for an easier interface that does *not* return the
"cdup").

> > Note: the new loop is a *little* tricky, as we have to handle the root
> > directory specially: we cannot simply strip away the last component
> > including the slash, as the root directory only has that slash. To
> > remedy that, we introduce the `min_offset` variable that holds the
> > minimal length of an absolute path, and using that to special-case the
> > root directory, including an early exit before trying to find the
> > parent of the root directory.
> 
> I wondered how t1509 fared with this, as it is the only test of
> repositories at the root directory (and it is not run by default because
> it involves a bunch of flaky and expensive chroot setup).

Oh, thanks. I allowed myself to forget about that test script (and did a
lot of testing by hand, but of course *during* the development of v2, not
when I had finished...).

> Unfortunately it seems to fail with your patch:
> 
>   expecting success: 
>   test_cmp_val "foo/" "$(git rev-parse --show-prefix)"
>   
>   --- expected
>   +++ result
>   @@ -1 +1 @@
>   -foo/
>   +oo/
>   not ok 66 - auto gitdir, foo: prefix

I can reproduce this failure here.

Side note: it took a while until I realized that the prepare-chroot.sh
script has to be run *every time* I change *anything* in either Git's
source code or in the test script.

> Could the problem be this:
> 
> > +   int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 
> > 1;
> > [...]
> > -   if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
> > -   ceil_offset = 1;
> > +   if (ceil_offset < 0)
> > +   ceil_offset = min_offset - 2;

Yes. The previous code did not need cwd.buf[0..offset] to be a valid path,
but it needed the offset to point to the trailing slash, if any.

> Interestingly, I don't think this is the bug, though. We still correctly
> find /.git as a repository. The problem seems to happen later, in
> setup_discovered_git_dir(), which does this:
> 
>   /* Make "offset" point to past the '/', and add a '/' at the end */
>   offset++;
>   strbuf_addch(cwd, '/');
>   return cwd->buf + offset;

I fixed this by ensuring that we only increment the offset if it is not
already pointing at the end of the first offset (which handles Windows
paths correctly, too).

> Other than this bug, I very much like the direction that this patch is
> taking us.

Awesome. I was anxious to 

[PATCH v2 3/5] Add --gui option to mergetool

[Denton Liu]

This fixes the discrepancy between difftool and mergetool where the
former has the --gui flag and the latter does not by adding the
functionality to mergetool.

Signed-off-by: Denton Liu 
---
 Documentation/git-mergetool.txt |  8 +++-
 git-mergetool.sh|  5 -
 t/t7610-mergetool.sh| 28 +++-
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 3622d6648..2ab56efcf 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve 
merge conflicts
 SYNOPSIS
 
 [verse]
-'git mergetool' [--tool=] [-y | --[no-]prompt] [...]
+'git mergetool' [--tool=] [-g|--gui] [-y | --[no-]prompt] [...]
 
 DESCRIPTION
 ---
@@ -64,6 +64,12 @@ variable `mergetool..trustExitCode` can be set to 
`true`.
 Otherwise, 'git mergetool' will prompt the user to indicate the
 success of the resolution after the custom tool has exited.
 
+-g::
+--gui::
+   When 'git-mergetool' is invoked with the `-g` or `--gui` option
+   the default diff tool will be read from the configured
+   `merge.guitool` variable instead of `merge.tool`.
+
 --tool-help::
Print a list of merge tools that may be used with `--tool`.
 
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c062e3de3..f3fce528b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
+USAGE='[--tool=tool] [-g|--gui] [--tool-help] [-y|--no-prompt|--prompt] 
[-O] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -414,6 +414,9 @@ main () {
shift ;;
esac
;;
+   -g|--gui)
+   merge_tool=$(git config merge.guitool)
+   ;;
-y|--no-prompt)
prompt=false
;;
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 01c1d44a9..31610f3b0 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -123,7 +123,9 @@ test_expect_success 'setup' '
git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
git config mergetool.mytool.trustExitCode true &&
git config mergetool.mybase.cmd "cat \"\$BASE\" >\"\$MERGED\"" &&
-   git config mergetool.mybase.trustExitCode true
+   git config mergetool.mybase.trustExitCode true &&
+   git config mergetool.badtool.cmd false &&
+   git config mergetool.badtool.trustExitCode true
 '
 
 test_expect_success 'custom mergetool' '
@@ -145,6 +147,30 @@ test_expect_success 'custom mergetool' '
git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool' '
+   test_when_finished "git reset --hard" &&
+   test_when_finished "git config merge.tool mytool" &&
+   test_when_finished "git config --unset merge.guitool" &&
+   git config merge.tool badtool &&
+   git config merge.guitool mytool &&
+   git checkout -b test$test_count branch1 &&
+   git submodule update -N &&
+   test_expect_code 1 git merge master >/dev/null 2>&1 &&
+   git mergetool -g both >/dev/null 2>&1 &&
+   git mergetool -g file1 file1 &&
+   git mergetool --gui file2 "spaced name" >/dev/null 2>&1 &&
+   git mergetool --gui subdir/file3 >/dev/null 2>&1 &&
+   ( yes "d" | git mergetool -g file11 >/dev/null 2>&1 ) &&
+   ( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
+   ( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
+   cat file1 &&
+   test "$(cat file1)" = "master updated" &&
+   test "$(cat file2)" = "master new" &&
+   test "$(cat subdir/file3)" = "master new sub" &&
+   test "$(cat submod/bar)" = "branch1 submodule" &&
+   git commit -m "branch1 resolved with gui mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
test_when_finished "git reset --hard" &&
# This test_config line must go after the above reset line so that
-- 
2.12.0.5.gfbc750a84

[PATCH v2 2/5] Use -y where possible in test t7610-mergetool

[Denton Liu]

In these tests, there are many situations where
'echo "" | git mergetool' is used. This replaces all of those
occurrences with 'git mergetool -y' for simplicity and readability.

Signed-off-by: Denton Liu 
---
 t/t7610-mergetool.sh | 62 ++--
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index a9e274add..01c1d44a9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -131,10 +131,10 @@ test_expect_success 'custom mergetool' '
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_expect_code 1 git merge master >/dev/null 2>&1 &&
-   ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool file1 file1 ) &&
-   ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+   git mergetool -y both >/dev/null 2>&1 &&
+   git mergetool -y file1 file1 &&
+   git mergetool -y file2 "spaced name" >/dev/null 2>&1 &&
+   git mergetool -y subdir/file3 >/dev/null 2>&1 &&
( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
@@ -154,11 +154,11 @@ test_expect_success 'mergetool crlf' '
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_expect_code 1 git merge master >/dev/null 2>&1 &&
-   ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+   git mergetool file1 >/dev/null 2>&1 &&
+   git mergetool file2 >/dev/null 2>&1 &&
+   git mergetool "spaced name" >/dev/null 2>&1 &&
+   git mergetool both >/dev/null 2>&1 &&
+   git mergetool subdir/file3 >/dev/null 2>&1 &&
( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "r" | git mergetool submod >/dev/null 2>&1 ) &&
@@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' '
(
cd subdir &&
test_expect_code 1 git merge master >/dev/null 2>&1 &&
-   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
+   git mergetool file3 >/dev/null 2>&1 &&
test "$(cat file3)" = "master new sub"
)
 '
@@ -189,10 +189,10 @@ test_expect_success 'mergetool on file in parent dir' '
(
cd subdir &&
test_expect_code 1 git merge master >/dev/null 2>&1 &&
-   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 
2>&1 ) &&
-   ( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
+   git mergetool file3 >/dev/null 2>&1 &&
+   git mergetool ../file1 >/dev/null 2>&1 &&
+   git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 &&
+   git mergetool ../both >/dev/null 2>&1 &&
( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) &&
@@ -302,7 +302,7 @@ test_expect_success 'mergetool takes partial path' '
git submodule update -N &&
test_expect_code 1 git merge master &&
 
-   ( yes "" | git mergetool subdir ) &&
+   git mergetool subdir &&
 
test "$(cat subdir/file3)" = "master new sub"
 '
@@ -370,8 +370,8 @@ test_expect_success 'deleted vs modified submodule' '
git checkout -b test$test_count.a test$test_count &&
test_expect_code 1 git merge master &&
test -n "$(git ls-files -u)" &&
-   ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
+   git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 &&
+   git mergetool both >/dev/null 2>&1 &&
( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
( yes "r" | git mergetool submod ) &&
rmdir submod && mv submod-movedaside submod &&
@@ -387,8 +387,8 @@ test_expect_success 'deleted vs modified submodule' '
git submodule update -N &&
test_expect_code 1 git merge master &&
test -n "$(git ls-files -u)" &&
-   ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
-   ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
+   git mergetool file1 file2 spaced\ name subdir/file3 >/dev

[PATCH v2 1/5] Detect merges specifically in test t7610-mergetool

[Denton Liu]

Prior to this, the test cases would use 'test_expect_failure' to detect
a merge conflict. This changes the test case to use 'test_expect_code 1'
which is more specific.

Signed-off-by: Denton Liu 
---
 t/t7610-mergetool.sh | 76 ++--
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 381b7df45..a9e274add 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -130,7 +130,7 @@ test_expect_success 'custom mergetool' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
-   test_must_fail git merge master >/dev/null 2>&1 &&
+   test_expect_code 1 git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
( yes "" | git mergetool file1 file1 ) &&
( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
@@ -153,7 +153,7 @@ test_expect_success 'mergetool crlf' '
# test_when_finished is LIFO.)
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
-   test_must_fail git merge master >/dev/null 2>&1 &&
+   test_expect_code 1 git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
@@ -176,7 +176,7 @@ test_expect_success 'mergetool in subdir' '
git submodule update -N &&
(
cd subdir &&
-   test_must_fail git merge master >/dev/null 2>&1 &&
+   test_expect_code 1 git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
test "$(cat file3)" = "master new sub"
)
@@ -188,7 +188,7 @@ test_expect_success 'mergetool on file in parent dir' '
git submodule update -N &&
(
cd subdir &&
-   test_must_fail git merge master >/dev/null 2>&1 &&
+   test_expect_code 1 git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 
2>&1 ) &&
@@ -207,7 +207,7 @@ test_expect_success 'mergetool skips autoresolved' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
-   test_must_fail git merge master &&
+   test_expect_code 1 git merge master &&
test -n "$(git ls-files -u)" &&
( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
@@ -222,7 +222,7 @@ test_expect_success 'mergetool merges all from subdir 
(rerere disabled)' '
test_config rerere.enabled false &&
(
cd subdir &&
-   test_must_fail git merge master &&
+   test_expect_code 1 git merge master &&
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
@@ -241,7 +241,7 @@ test_expect_success 'mergetool merges all from subdir 
(rerere enabled)' '
rm -rf .git/rr-cache &&
(
cd subdir &&
-   test_must_fail git merge master &&
+   test_expect_code 1 git merge master &&
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
@@ -259,7 +259,7 @@ test_expect_success 'mergetool skips resolved paths when 
rerere is active' '
rm -rf .git/rr-cache &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
-   test_must_fail git merge master >/dev/null 2>&1 &&
+   test_expect_code 1 git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
git submodule update -N &&
@@ -275,7 +275,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
git stash &&
 
git checkout --detach stash2 &&
-   test_must_fail git stash apply &&
+   test_expect_code 1 git stash apply &&
 
test -n "$(git ls-files -u)" &&
conflicts="$(git rerere remaining)" &&
@@ -286,7 +286,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
git commit -am "save the stash resolution" &&
 
git reset --hard stash2 &&
-   test_must_fail git stash apply &&
+   test_expect_code 1 git stash apply &&
 
test -n "$(git ls-files -u)" &&
conflicts="$(git rerere remaining)" &&
@@ -300,7 +300,7 @@ test_expect_su

Re: [PATCH 1/3] Add --gui option to mergetool

[David Aguilar]

On Fri, Mar 03, 2017 at 03:57:38AM -0800, Denton Liu wrote:
> This fixes the discrepancy between difftool and mergetool where the
> former has the --gui flag and the latter does not by adding the
> functionality to mergetool.
> 
> Signed-off-by: Denton Liu 
> ---
>  Documentation/git-mergetool.txt|  8 +++-
>  contrib/completion/git-completion.bash |  3 ++-
>  git-mergetool.sh   |  5 -
>  t/t7610-mergetool.sh   | 28 +++-
>  4 files changed, 40 insertions(+), 4 deletions(-)

Would you mind splitting up this patch so that the
completion part is done separately?


> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 381b7df45..5683907ab 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -145,6 +147,30 @@ test_expect_success 'custom mergetool' '
>   git commit -m "branch1 resolved with mergetool"
>  '
>  
> +test_expect_success 'gui mergetool' '
> + test_when_finished "git reset --hard" &&
> + test_when_finished "git config merge.tool mytool" &&
> + test_when_finished "git config --unset merge.guitool" &&
> + git config merge.tool badtool &&
> + git config merge.guitool mytool &&
> + git checkout -b test$test_count branch1 &&
> + git submodule update -N &&
> + test_must_fail git merge master >/dev/null 2>&1 &&

It'd probably be better to use test_expect_code instead of
test_must_fail here:

test_expect_code 1 git merge master ...


> + ( yes "" | git mergetool -g both >/dev/null 2>&1 ) &&
> + ( yes "" | git mergetool -g file1 file1 ) &&
> + ( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
> + ( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&

I realize that this test is based on an existing one, but I'm curious..
is "yes" used above because it's prompting, and using -y or --no-prompt
here would eliminate the need for the 'yes ""' parts?

Looks good otherwise.

Thanks,
-- 
David

[RFC PATCH 2/2] completion: add bash completion for 'filter-branch'

[Denton Liu]

Signed-off-by: Denton Liu 
---
If the last patch (PATCH 1/2) is not included, we can remove the call to
__git_complete_rev_list_command.
---
 contrib/completion/git-completion.bash | 17 +
 1 file changed, 17 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 412485369..933dac78b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1307,6 +1307,23 @@ _git_fetch ()
__git_complete_remote_or_refspec
 }
 
+__git_filter_branch_options="
+   --env-filter --tree-filter --index-filter --parent-filter --msg-filter
+   --commit-filter --tag-name-filter --subdirectory-filter --prune-empty
+   --original --force
+"
+_git_filter_branch ()
+{
+   __git_has_doubledash && __git_complete_rev_list_command && return
+
+   case "$cur" in
+   --*)
+   __gitcomp "$__git_filter_branch_options"
+   return
+   ;;
+   esac
+}
+
 __git_format_patch_options="
--stdout --attach --no-attach --thread --thread= --no-thread
--numbered --start-number --numbered-files --keep-subject --signoff
-- 
2.12.0.1.g5415fdfc5.dirty

Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts

[Jeff King]

On Fri, Mar 03, 2017 at 10:57:46AM +, Sebastian Schuberth wrote:

> It does not make sense for these placeholder scripts to depend on Python
> just because the real scripts do. At the example of Git for Windows, we
> would not even be able to see those warnings as it does not ship with
> Python. So just use plain shell scripts instead.

Yeah, this seems like an obvious improvement. I think we got here
because the originals issued a warning but kept working, and then it was
slowly whittled down to remove the "working" part.

At some point these can probably go away. It's been 3 years since they
turned into nothing but warnings, so presumably most people have
upgraded by now (I know people often go a long time on old
distro-packaged versions, but the distro packagers would presumably
figure this out in the meantime).

OTOH, it is not really hurting much, so I do not mind keeping them
around for another 3 years (or more) just to catch any stragglers.

-Peff

Re: [PATCH v2 1/9] t7006: replace dubious test

[Johannes Schindelin]

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:02AM +0100, Johannes Schindelin wrote:
> 
> > The intention of that test case, however, was to verify that the
> > setup_git_directory() function has not run, because it changes global
> > state such as the current working directory.
> > 
> > To keep that spirit, but fix the incorrect assumption, this patch
> > replaces that test case by a new one that verifies that the pager is
> > run in the subdirectory, i.e. that the current working directory has
> > not been changed at the time the pager is configured and launched,
> > even if the `rev-parse` command requires a .git/ directory and *will*
> > change the working directory.
> 
> This is a great solution to the question I had in v1 ("how do we know
> when setup_git_directory() is run?"). It not only checks the
> internal-code case that we care about, but it also shows how real users
> would get bit if we do the wrong thing.

Thanks!

> > +test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
> > +   sane_unset GIT_PAGER &&
> > +   test_config core.pager "cat >cwd-retained" &&
> > +   (
> > +   cd sub &&
> > +   rm -f cwd-retained &&
> > +   test_terminal git -p rev-parse HEAD &&
> > +   test -e cwd-retained
> > +   )
> > +'
> 
> Does this actually need TTY and test_terminal?

Sadly, yes. If git.c sees a --paginate or -p, it sets use_pager to 1 which
is later used as indicator that setup_pager() should be run. And the first
line of that function reads:

const char *pager = git_pager(isatty(1));

i.e. it does *not* turn on the pager unconditionally.

> (Also a minor nit: we usually prefer test_path_is_file to "test -e"
> these days).

Thanks, fixed.

Ciao,
Dscho

[PATCH 3/3] Remove outdated info in difftool manpage

[Denton Liu]

When difftool was rewritten in C, it removed the capability to read
fallback configs from mergetool. This changes the documentation to
reflect this.

Signed-off-by: Denton Liu 
---
 Documentation/git-difftool.txt | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa..a00cb033e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,9 +105,6 @@ See linkgit:git-diff[1] for the full list of supported 
options.
 
 CONFIG VARIABLES
 
-'git difftool' falls back to 'git mergetool' config variables when the
-difftool equivalents have not been defined.
-
 diff.tool::
The default diff tool to use.
 
-- 
2.12.0.1.g5415fdfc5.dirty

[PATCH 2/3] Add gui completions for difftool

[Denton Liu]

Signed-off-by: Denton Liu 
---
 contrib/completion/git-completion.bash | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d5f3b9aeb..c94e38a3a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1277,7 +1277,7 @@ _git_difftool ()
--base --ours --theirs
--no-renames --diff-filter= --find-copies-harder
--relative --ignore-submodules
-   --tool="
+   --tool= --gui"
return
;;
esac
@@ -2207,6 +2207,7 @@ _git_config ()
diff.submodule
diff.suppressBlankEmpty
diff.tool
+   diff.guitool
diff.wordRegex
diff.algorithm
difftool.
-- 
2.12.0.1.g5415fdfc5.dirty