* Re: [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-02 19:04 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Johannes Schindelin, Duy Nguyen, Jeff King, Junio C Hamano,
Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <85221b97-759f-b7a9-1256-21515d163cbf@jeffhostetler.com>
On Thu, Mar 2, 2017 at 10:37 AM, Jeff Hostetler <git@jeffhostetler.com> wrote:
>>
>> Now, if your _file_ index is 300-400MB (and I do think we check the
>> SHA fingerprint on that even on just reading it - verify_hdr() in
>> do_read_index()), then that's going to be a somewhat noticeable hit on
>> every normal "git diff" etc.
>
> Yes, the .git/index is 450MB with ~3.1M entries. verify_hdr() is called
> each time we read it into memory.
Ok. So that's really just a purely historical artifact.
The file index is actually the first part of git to have ever been
written. You can't even see it in the history, because the initial
revision from Apr 7, 2005, obviously depended on the actual object
hashing.
But the file index actually came first. You can _kind_ of see that in
the layout of the original git tree, and how the main header file is
still called "cache.h", and how the original ".git" directory was
actually called ".dircache".
And the two biggest files (by a fairly big margin) are "read-cache.c"
and "update-cache.c".
So that file index cache was in many ways _the_ central part of the
original git model. The sha1 file indexing and object database was
just the backing store for the file index.
But part of that history is then how much I worried about corruption
of that index (and, let's face it, general corruption resistance _was_
one of the primary design goals - performance was high up there too,
but safety in the face of filesystem corruption was and is a primary
issue).
But realistically, I don't think we've *ever* hit anything serious on
the index file, and it's obviously not a security issue. It also isn't
even a compatibility issue, so it would be trivial to just bump the
version header and saying that the signature changes the meaning of
the checksum.
That said:
> We have been testing a patch in GfW to run the verification in a separate thread
> while the main thread parses (and mallocs) the cache_entries. This does help
> offset the time.
Yeah, that seems an even better solution, honestly.
The patch would be cleaner without the NO_PTHREADS things.
I wonder how meaningful that thing even is today. Looking at what
seems to select NO_PTHREADS, I suspect that's all entirely historical.
For example, you'll see it for QNX etc, which seems wrong - QNX
definitely has pthreads according to their docs, for example.
Linus
^ permalink raw reply
* Re: [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec
From: Brandon Williams @ 2017-03-02 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, pclouds
In-Reply-To: <xmqqa896ja7z.fsf@gitster.mtv.corp.google.com>
On 02/28, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > Fix a bug which causes a child process for a submodule to error out when a
> > relative pathspec with a ".." is provided in the superproject.
> >
> > While at it, correctly construct the super-prefix to be used in a submodule
> > when not at the root of the repository.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > builtin/ls-files.c | 8 ++++++--
> > t/t3007-ls-files-recurse-submodules.sh | 2 +-
> > 2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> > index 159229081..89533ab8e 100644
> > --- a/builtin/ls-files.c
> > +++ b/builtin/ls-files.c
> > @@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag
> > static void show_gitlink(const struct cache_entry *ce)
> > {
> > struct child_process cp = CHILD_PROCESS_INIT;
> > + struct strbuf name = STRBUF_INIT;
> > int status;
> > int i;
> >
> > + quote_path_relative(ce->name, prefix, &name);
> > argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
>
> Same comment as 3/5. quote_path is to produce c-quote and is not
> even meant for shell script quoting. run_command() interface would
> do its own shell quoting when needed, so I think you just want the
> exact string you want to pass here.
>
Yeah I don't know what I was thinking when using that instead of
'relative_path()'. Will change for this and 3/5.
--
Brandon Williams
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-03-02 19:55 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFw6BLjPK-F0RGd9LT7X5xosKOXOxuhmKX65ZHn09r1xow@mail.gmail.com>
On Fri, Feb 24, 2017 at 4:39 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Honestly, I think that a primary goal for a new hash implementation
> absolutely needs to be to minimize mixing.
>
> Not for security issues, but because of combinatorics. You want to
> have a model that basically reads old data, but that very aggressively
> approaches "new data only" in order to avoid the situation where you
> have basically the exact same tree state, just _represented_
> differently.
>
> For example, what I would suggest the rules be is something like this:
Hmm. Having looked at this a fair amount, and in particularly having
looked at the code as part of the hash typesafety patch I did, I am
actually starting to think that it would not be too painful at all to
have a totally different approach, which might be a lot easier to do.
So bear with me, let me try to explain my thinking:
(a) if we want to be backwards compatible and not force people to
convert their trees on some flag day, we're going to be stuck with
having to have the SHA1 code around and all the existing object
parsing for basically forever
Now, this basically means that the _easy_ solution would be that we
just do the flag day, switch to sha-256, extend everything to 32-byte
hashes, and just have a "git2 fast-import" that makes it easy to
convert stuff.
But it really would be a completely different version of git, with a
new pack-file format and no real compatibility. Such a flag-day
approach would certainly have advantages: it would allow for just
re-architecting some bad choices:
- make the hashing be something that can be threaded (ie maybe we can
just block it up in 4MB chunks that you can hash in parallel, and make
the git object hash be the hash of hashes)
- replace zlib with something like zstd
- get rid of old pack formats etc.
but on the whole, I still think that the compatibility would be worth
much more than the possible technical advantages of a clean slate
restart.
(b) the SHA1 hash is actually still quite strong, and the collision
detection code effectively means that we don't really have to worry
about collisions in the immediate future.
In other words, the mitigation of the current attack is actually
really easy technically (modulo perhaps the performance concerns), and
there's still nothing fundamentally wrong with using SHA1 as a content
hash. It's still a great hash.
Now, my initial reaction (since it's been discussed for so long
anyway) was obviously "pick a different hash". That was everybody's
initial reaction, I think.
But I'm starting to think that maybe that initial obvious reaction was wrong.
The thing that makes collision attacks so nasty is that our reaction
to a collision is so deadly. But that's not necessarily fundamental:
we certainly uses hashes with collisions every day, and they work
fine. And they work fine because the code that uses those hashes is
designed to simply deal gracefully - although very possibly with a
performance degradation - with two different things hashing to the
same bucket.
So what if the solution to "SHA1 has potential collisions" is "any
hash can have collisions in theory, let's just make sure we handle
them gracefully"?
Because I'm looking at our data structures that have hashes in them,
and many of them are actually of the type where I go
"Hmm.. Replacing the hash entirely is really really painful - but
it wouldn't necessarily be all that nasty to extend the format to have
additional version information".
and the advantage of that approach is that it actually makes the
compatibility part trivial. No more "have to pick a different hash and
two different formats", and more of a "just the same format with
extended information that might not be there for old objects".
So we have a few different types of formats:
- the purely local data structures: the pack index file, the file
index, our refs etc
These we could in change completely, and it wouldn't even be all
that painful. The pack index has already gone through versions, and it
doesn't affect anything else.
- the actual objects.
These are fairly painful to change, particularly things like the
"tree" object which is probably the worst designed of the lot. Making
it contain a fixed-size binary thing was really a nasty mistake. My
bad.
- the pack format and the protocol to exchange "I have this" information
This is *really* painful to change, because it contains not just
the raw object data, but it obviously ends up being the wire format
for remote accesses.
and it turns out that *all* of these formats look like they would be
fairly easy to extend to having extra object version information. Some
of that extra object version information we already have and don't
use, in fact.
Even the tree format, with the annoying fixed-size binary blob. Yes,
it has that fixed size binary blob, but it has it in _addition_ to the
ASCII textual form that would be really easy to just extend upon. We
have that "tree entry type" that we've already made extensions with by
using it for submodules. It would be quite easy to just say that a
tree entry also has a "file version" field, so that you can have
multiple objects that just hash to the same SHA1, and git wouldn't
even *care*.
The transfer protocol is the same: yes, we transfer hashes around, but
it would not be all that hard to extend it to "transfer hash and
object version".
And the difference is that then the "backwards compatibility" part
just means interacting with somebody who didn't know to transfer the
object version. So suddenly being backwards compatible isn't a whole
different object parsing thing, it's just a small extension.
IOW, we could make it so that the SHA1 is just a hash into a list of
objects. Even the pack index format wouldn't need to change - right
now we assume that an index hit gives us the direct pointer into the
pack file, but we *could* just make it mean that it gives us a direct
pointer to the first object in the pack file with that SHA1 hash.
Exactly like you'd normally use a hash table with linear probing.
Linear probing is usually considered a horrible approach to hash
tables,. but it's actually a really useful one for the case where
collisions are very rare.
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?
Linus
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Mike Crowe @ 2017-03-02 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tboegi, git, Jeff King
In-Reply-To: <xmqqbmtjcyug.fsf@gitster.mtv.corp.google.com>
On Thursday 02 March 2017 at 10:33:59 -0800, Junio C Hamano wrote:
> Mike Crowe <mac@mcrowe.com> writes:
>
> > All the solutions presented so far do cause a small change in behaviour
> > when using git diff --quiet: they may now cause warning messages like:
> >
> > warning: CRLF will be replaced by LF in crlf.txt.
> > The file will have its original line endings in your working directory.
>
> That is actually a good thing, I think. As the test modifies a file
> that originally has "Hello\r\nWorld\r\n" in it to this:
>
> >> +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 &&
>
> If you did "git add" at this point, you would get the same warning,
> because the lack of CR on the second line could well be a mistake
> you may want to notice and fix before going forward. Otherwise
> you'd be losing information that _might_ matter to you (i.e. the
> fact that the first line had CRLF while the second had LF) and it is
> the whole point of safe_crlf setting.
Well, there is an argument that it's not very "--quiet" to emit this
message. Especially when it didn't used to come out. However, I can
understand that the message is useful if the line endings have changed
despite this.
However, I can make the message appear from "git diff --quiet" even if the
line endings have not changed. I merely need to touch a file where the line
endings do not match the canonical representation in the repository first.
Upon subsequent invocations of "git diff --quiet" the message does not come
out. (Note that in this may not be reproducible in a script without
sleeps.)
Perhaps this interactive log will make things clearer:
$ git init
Initialized empty Git repository in /tmp/test/.git/
$ echo "* text=auto" >.gitattributes
$ printf "Hello\r\nWorld\r\n" >crlf.txt
$ git add .gitattributes crlf.txt
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
The message was expected and useful there.
$ git commit -m "initial"
[master (root-commit) c3fb5a5] initial
2 files changed, 3 insertions(+)
create mode 100644 .gitattributes
create mode 100644 crlf.txt
$ git diff --quiet
$ touch crlf.txt
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
I didn't change the file - I just touched it. Why did the message come out here?
$ git diff --quiet
But then it didn't here. Which is correct?
$ printf "Hello\r\nWorld\n" >crlf.txt
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
If the line endings have genuinely changed then the message comes out every
time...
$ git add crlf.txt
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
...until the file is added to the index. That's probably the right thing to
do.
$ git diff --quiet
$ touch crlf.txt
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
$ git diff --quiet
But once the file has been added the previous behaviour of only emitting
the message on the first time after the touch occurs.
$ printf "Hello\r\nWorld\n" >crlf.txt
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
$ git diff --quiet
$ printf "Hello\r\nWorld\r\n" >crlf.txt
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
$ git diff --quiet
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
Hopefully that makes things a bit clearer.
Mike.
^ permalink raw reply
* Re: git status --> Out of memory, realloc failed
From: René Scharfe @ 2017-03-02 20:04 UTC (permalink / raw)
To: Carsten Fuchs; +Cc: git
In-Reply-To: <cbd281fc-3a4b-b4dc-5dff-145c97cd68d6@cafu.de>
Am 01.03.2017 um 21:12 schrieb Carsten Fuchs:
> Hi René,
>
> Am 01.03.2017 um 11:02 schrieb René Scharfe:
>>> I use Git at a web hosting service, where my user account has a memory
>>> limit of 768 MB:
>>>
>>> (uiserver):p7715773:~$ uname -a
>>> Linux infongp-de15 3.14.0-ui16322-uiabi1-infong-amd64 #1 SMP Debian
>>> 3.14.79-2~ui80+4 (2016-11-17) x86_64 GNU/Linux
>>
>> What's the output of "ulimit -a"?
>
> (uiserver):p7715773:~$ ulimit -a
> core file size (blocks, -c) 0
> data seg size (kbytes, -d) unlimited
> scheduling priority (-e) 1
> file size (blocks, -f) unlimited
> pending signals (-i) 16382
> max locked memory (kbytes, -l) 64
> max memory size (kbytes, -m) unlimited
> open files (-n) 512
> pipe size (512 bytes, -p) 8
> POSIX message queues (bytes, -q) 819200
> real-time priority (-r) 0
> stack size (kbytes, -s) 8192
> cpu time (seconds, -t) 1800
> max user processes (-u) 42
> virtual memory (kbytes, -v) 786432
> file locks (-x) unlimited
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").
>>> The repository is tracking about 19000 files which together take 260 MB.
>>> The git server version is 2.7.4.1.g5468f9e (Bitbucket)
>>
>> Is your repository publicly accessible?
>
> Unfortunately, no. There are no big secrets in there, but just a couple
> of database details so that I cannot make it universally available. I
> can gladly give you access though. (E.g. by adding your public SSH key?)
I'd rather not look at semi-confidential data, and you probably
shouldn't hand it to a stranger on the internet anyway. ;)
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?
Another one, darker yet: Does "git config core.preloadIndex 0" help?
René
^ permalink raw reply
* Re: Transition plan for git to move to a new hash function
From: Ian Jackson @ 2017-03-02 18:13 UTC (permalink / raw)
To: brian m. carlson
Cc: Jeff King, Ævar Arnfjörð Bjarmason, Linus Torvalds,
Jason Cooper, ankostis, Junio C Hamano, Git Mailing List,
Stefan Beller, David Lang, Joey Hess
In-Reply-To: <20170228214724.w7w5f6n4u6ehanzd@genre.crustytoothpaste.net>
brian m. carlson writes ("Re: Transition plan for git to move to a new hash function"):
> On Mon, Feb 27, 2017 at 01:00:01PM +0000, Ian Jackson wrote:
> > Objects of one hash may refer to objects named by a different hash
> > function to their own. Preference rules arrange that normally, new
> > hash objects refer to other new hash objects.
>
> The existing codebase isn't really intended with that in mind.
Yes. I've seen the attempts to start to replace char* with a hash
struct.
> I like Peff's suggested approach in which we essentially rewrite history
> under the hood, but have a lookup table which looks up the old hash
> based on the new hash. That allows us to refer to old objects, but not
> have to share serialized data that mentions both hashes.
I think this means that the when a project converts, every copy of the
history must be rewritten (separately). Also, this leaves the whole
system lacking in algorithm agililty. Meaning we may have to do all
of this again some time.
I also think that we need to distinguish old hashes from new hashes in
the command line interface etc. Otherwise there is a possible
ambiguity.
> > The object name textual syntax is extended. The new syntax may be
> > used in all textual git objects and protocols (commits, tags, command
> > lines, etc.).
> >
> > We declare that the object name syntax is henceforth
> > [A-Z]+[0-9a-z]+ | [0-9a-f]+
> > and that names [A-Z].* are deprecated as ref name components.
>
> I'd simply say that we have data always be in the new format if it's
> available, and tag the old SHA-1 versions instead. Otherwise, as Peff
> pointed out, we're going to be stuck typing a bunch of identical stuff
> every time. Again, this encourages migration.
The hash identifier is only one character. Object names are not
normally typed very much anyway.
If you say we must decorate old hashes, then all existing data
everywhere in the world which refers to any git objects by object name
will become invalid. I don't mean just data in git here. I mean CI
systems, mailing list archives, commit messages (perhaps in other
version control systems), test cases, and so on.
Ian.
^ permalink raw reply
* Re: SHA1 collisions found
From: Junio C Hamano @ 2017-03-02 20:43 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFwXaSAMF41Dz3u3nS+2S24umdUFv0+k+s18UyPoj+v31g@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> 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?
My reaction heavily depends on how that "object version" thing
works. When I think I have "variant #1" of an object and say
have 860cd699c285f02937a2edbdb78e8231292339a5#1
is there any guarantee that the other end has a (small) set of
different objects all sharing the same SHA-1 and it thinks it has
"variant #1" only when it has the same thing as I have (otherwise,
it may have "variant #2" that is an unrelated object but happens to
share the same hash)? If so, I think I understand how things would
work within your "concept". But otherwise, I am not really sure.
Would "object version" be like a truncated SHA-1 over the same data
but with different IV or something, i.e. something that guarantees
anybody would get the same result given the data to be hashed?
^ permalink raw reply
* Re: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area
From: Junio C Hamano @ 2017-03-02 21:11 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: Jeff King, Johannes Schindelin, git, Jeff Hostetler
In-Reply-To: <16b1259c-4cdc-8f4d-db47-d724386a3d2b@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
> On 2/14/2017 5:03 PM, Jeff King wrote:
>> On Tue, Feb 14, 2017 at 12:31:46PM +0100, Johannes Schindelin wrote:
>>
>>> On Windows, calls to memihash() and maintaining the istate.name_hash and
>>> istate.dir_hash HashMaps take significant time on very large
>>> repositories. This series of changes reduces the overall time taken for
>>> various operations by reducing the number calls to memihash(), moving
>>> some of them into multi-threaded code, and etc.
>>>
>>> Note: one commenter in https://github.com/git-for-windows/git/pull/964
>>> pointed out that memihash() only handles ASCII correctly. That is true.
>>> And fixing this is outside the purview of this patch series.
>> Out of curiosity, do you have numbers? Bonus points if the speedup can
>> be shown via a t/perf script.
>>
>> We have a read-cache perf-test already, but I suspect you'd want
>> something more like "git status" or "ls-files -o" that calls into
>> read_directory().
>
> I have some informal numbers in a spreadsheet. I was seeing
> a 8-9% speed up on a status on my gigantic repo.
>
> I'll try to put together a before/after perf-test to better
> demonstrate this.
Ping? I do not think there is anything wrong with the changes from
correctness point of view, but as the series is about performance,
it somewhat feels pointless to merge to 'next' without mentioning
the actual numbers.
It might be sufficient to mention the rough numbers in the log
messages, if additions to t/perf/ are too cumbersome to arrange.
^ permalink raw reply
* Re: [PATCH v3] Documentation: Improve description for core.quotePath
From: Junio C Hamano @ 2017-03-02 19:41 UTC (permalink / raw)
To: Andreas Heiduk; +Cc: git
In-Reply-To: <1488481432-25336-1-git-send-email-asheiduk@gmail.com>
This is now a single-patch, which makes sense, too.
Let's merge it to 'next'.
Thanks.
^ permalink raw reply
* RE: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area
From: Jeff Hostetler @ 2017-03-02 21:18 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler
Cc: Jeff King, Johannes Schindelin, git@vger.kernel.org,
Jeff Hostetler
In-Reply-To: <xmqqfuivbcz0.fsf@gitster.mtv.corp.google.com>
Sorry, $DAYJOB got in the way (again).
This is still on my short-list of things to take care of.
I should have something for you next week.
Thanks again,
Jeff
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Thursday, March 2, 2017 4:12 PM
To: Jeff Hostetler <git@jeffhostetler.com>
Cc: Jeff King <peff@peff.net>; Johannes Schindelin <johannes.schindelin@gmx.de>; git@vger.kernel.org; Jeff Hostetler <Jeff.Hostetler@microsoft.com>
Subject: Re: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area
Jeff Hostetler <git@jeffhostetler.com> writes:
> On 2/14/2017 5:03 PM, Jeff King wrote:
>> On Tue, Feb 14, 2017 at 12:31:46PM +0100, Johannes Schindelin wrote:
>>
>>> On Windows, calls to memihash() and maintaining the istate.name_hash and
>>> istate.dir_hash HashMaps take significant time on very large
>>> repositories. This series of changes reduces the overall time taken for
>>> various operations by reducing the number calls to memihash(), moving
>>> some of them into multi-threaded code, and etc.
>>>
>>> Note: one commenter in https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgit-for-windows%2Fgit%2Fpull%2F964&data=02%7C01%7CJeff.Hostetler%40microsoft.com%7C1d493f3031f74657f29308d461b0be80%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636240859121403139&sdata=16RQH1%2BrDonsanClb3%2Fwue7pcy9l7cUq9lDJenqCgbE%3D&reserved=0
>>> pointed out that memihash() only handles ASCII correctly. That is true.
>>> And fixing this is outside the purview of this patch series.
>> Out of curiosity, do you have numbers? Bonus points if the speedup can
>> be shown via a t/perf script.
>>
>> We have a read-cache perf-test already, but I suspect you'd want
>> something more like "git status" or "ls-files -o" that calls into
>> read_directory().
>
> I have some informal numbers in a spreadsheet. I was seeing
> a 8-9% speed up on a status on my gigantic repo.
>
> I'll try to put together a before/after perf-test to better
> demonstrate this.
Ping? I do not think there is anything wrong with the changes from
correctness point of view, but as the series is about performance,
it somewhat feels pointless to merge to 'next' without mentioning
the actual numbers.
It might be sufficient to mention the rough numbers in the log
messages, if additions to t/perf/ are too cumbersome to arrange.
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Devin J. Pohly @ 2017-03-02 21:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Charles Bailey, Jeff King, git
In-Reply-To: <xmqqshmvbhe5.fsf@gitster.mtv.corp.google.com>
On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote:
> "Devin J. Pohly" <djpohly@gmail.com> writes:
>
> > I think your point is interesting too, though. If a commit is also
> > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
> > reasonable that someone might want to leave it in the filtered branch as
> > an empty commit while pruning empt*ied* commits. I would imagine that
> > as another option (--prune-newly-empty?).
>
> I was hoping to hear from others who may care about filter-branch to
> comment on this topic to help me decide, but I haven't heard
> anything, so here is my tentative thinking.
>
> I am leaning to:
>
> * Take your series as-is, which would mean --prune-empty will
> change the behaviour to unconditionally lose the empty root.
>
> * Then, people who care deeply about it can add a new option that
> prunes commits that become empty while keeping the originally
> empty ones.
>
> Thoughts?
Sounds good to me. I would be willing to work on a new option if needed
(to "atone" for changing existing behavior), so you can loop me in if
there are any complaints.
--
<><
^ permalink raw reply
* Re: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area
From: Junio C Hamano @ 2017-03-02 21:40 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Jeff Hostetler, Jeff King, Johannes Schindelin,
git@vger.kernel.org
In-Reply-To: <MWHPR03MB29581B0EDDEDCA7D51EC396F8A280@MWHPR03MB2958.namprd03.prod.outlook.com>
Jeff Hostetler <Jeff.Hostetler@microsoft.com> writes:
> Sorry, $DAYJOB got in the way (again).
>
> This is still on my short-list of things to take care of.
> I should have something for you next week.
That's perfectly OK. I just wanted a newer articule in my
newsreader I can bookmark so that I won't forget ;-) No hurries.
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Junio C Hamano @ 2017-03-02 21:39 UTC (permalink / raw)
To: Devin J. Pohly; +Cc: Johannes Schindelin, Charles Bailey, Jeff King, git
In-Reply-To: <20170302211804.GB3490@prospect.localdomain>
"Devin J. Pohly" <djpohly@gmail.com> writes:
> On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote:
>> "Devin J. Pohly" <djpohly@gmail.com> writes:
>>
>> > I think your point is interesting too, though. If a commit is also
>> > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
>> > reasonable that someone might want to leave it in the filtered branch as
>> > an empty commit while pruning empt*ied* commits. I would imagine that
>> > as another option (--prune-newly-empty?).
>>
>> I was hoping to hear from others who may care about filter-branch to
>> comment on this topic to help me decide, but I haven't heard
>> anything, so here is my tentative thinking.
>>
>> I am leaning to:
>>
>> * Take your series as-is, which would mean --prune-empty will
>> change the behaviour to unconditionally lose the empty root.
>>
>> * Then, people who care deeply about it can add a new option that
>> prunes commits that become empty while keeping the originally
>> empty ones.
>>
>> Thoughts?
>
> Sounds good to me. I would be willing to work on a new option if needed
> (to "atone" for changing existing behavior), so you can loop me in if
> there are any complaints.
Thanks. I'll wait for others who know filter-branch better than me
to say something for a few days before doing anything, though.
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-03-02 22:27 UTC (permalink / raw)
To: Joey Hess; +Cc: Junio C Hamano, Jeff King, Ian Jackson, Git Mailing List
In-Reply-To: <20170302215457.l2zhxgnvhulw2hl5@kitenet.net>
On Thu, Mar 2, 2017 at 1:54 PM, Joey Hess <id@joeyh.name> wrote:
>
> There's a surprising result of combining iterated hash functions, that
> the combination is no more difficult to attack than the strongest hash
> function used.
Duh. I should actually have known that. I started reading the paper
and went "this seems very familiar". I'm pretty sure I've been pointed
at that paper before (or maybe just a similar one), and I just didn't
react enough for it to leave a lasting impact.
Linus
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Jacob Keller @ 2017-03-02 23:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Devin J. Pohly, Johannes Schindelin, Charles Bailey, Jeff King,
Git mailing list
In-Reply-To: <xmqqshmvbhe5.fsf@gitster.mtv.corp.google.com>
On Thu, Mar 2, 2017 at 11:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Devin J. Pohly" <djpohly@gmail.com> writes:
>
>> I think your point is interesting too, though. If a commit is also
>> TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
>> reasonable that someone might want to leave it in the filtered branch as
>> an empty commit while pruning empt*ied* commits. I would imagine that
>> as another option (--prune-newly-empty?).
>
> I was hoping to hear from others who may care about filter-branch to
> comment on this topic to help me decide, but I haven't heard
> anything, so here is my tentative thinking.
>
> I am leaning to:
>
> * Take your series as-is, which would mean --prune-empty will
> change the behaviour to unconditionally lose the empty root.
>
This new behavior is how I expected prune-empty to behave, so seeing
that it did not already behave this way was surprising.
Thanks,
Jake
^ permalink raw reply
* [PATCH] line-log.c: prevent crash during union of too many ranges
From: Allan Xavier @ 2017-03-02 17:29 UTC (permalink / raw)
To: gitster; +Cc: Allan Xavier, git
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.
For struct range_set *out to grow correctly it must have out->nr set to
the current size of the buffer when it is passed to range_set_grow.
However, the existing implementation of range_set_union only updates
out->nr at the end of the function, meaning that it is always zero
before this. This results in range_set_grow never growing the buffer, as
well as some of the union logic itself being incorrect as !out->nr is
always true.
The reason why 24 is the limit is that the first allocation of size 1
ends up allocating a buffer of size 24 (due to the call to alloc_nr in
ALLOC_GROW). This goes some way to explain why this hasn't been
caught before.
Fix the problem by correctly updating out->nr after reallocating the
range_set. As this results in out->nr containing the same value as the
variable o, replace o with out->nr as well.
Finally, add a new test to help prevent the problem reoccurring in the
future. Thanks to Vegard Nossum for writing the test.
Signed-off-by: Allan Xavier <allan.x.xavier@oracle.com>
---
Originally sent to git-security@googlegroups.com to give hosted
services a chance to apply this if they were affected.
line-log.c | 15 +++++++--------
t/t4211-line-log.sh | 10 ++++++++++
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/line-log.c b/line-log.c
index 65f3558b3..951029665 100644
--- a/line-log.c
+++ b/line-log.c
@@ -144,7 +144,7 @@ void sort_and_merge_range_set(struct range_set *rs)
static void range_set_union(struct range_set *out,
struct range_set *a, struct range_set *b)
{
- int i = 0, j = 0, o = 0;
+ int i = 0, j = 0;
struct range *ra = a->ranges;
struct range *rb = b->ranges;
/* cannot make an alias of out->ranges: it may change during grow */
@@ -167,16 +167,15 @@ static void range_set_union(struct range_set *out,
new = &rb[j++];
if (new->start == new->end)
; /* empty range */
- else if (!o || out->ranges[o-1].end < new->start) {
+ else if (!out->nr || out->ranges[out->nr-1].end < new->start) {
range_set_grow(out, 1);
- out->ranges[o].start = new->start;
- out->ranges[o].end = new->end;
- o++;
- } else if (out->ranges[o-1].end < new->end) {
- out->ranges[o-1].end = new->end;
+ out->ranges[out->nr].start = new->start;
+ out->ranges[out->nr].end = new->end;
+ out->nr++;
+ } else if (out->ranges[out->nr-1].end < new->end) {
+ out->ranges[out->nr-1].end = new->end;
}
}
- out->nr = o;
}
/*
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 9d87777b5..d0377fae5 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -106,4 +106,14 @@ test_expect_success '-L with --output' '
test_line_count = 70 log
'
+test_expect_success 'range_set_union' '
+ test_seq 500 > c.c &&
+ git add c.c &&
+ git commit -m "many lines" &&
+ test_seq 1000 > c.c &&
+ git add c.c &&
+ git commit -m "modify many lines" &&
+ git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)
+'
+
test_done
--
2.11.0
^ permalink raw reply related
* Re: SHA1 collisions found
From: Joey Hess @ 2017-03-02 21:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Jeff King, Ian Jackson, Git Mailing List
In-Reply-To: <CA+55aFziZRA29foAMbM-HS5fiup7T0TuYf4XQ1kNT_SR7FfSgw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 513 bytes --]
Linus Torvalds wrote:
> So you'd have to be able to attack both the full SHA1, _and_ whatever
> other different good hash to 128 bits.
There's a surprising result of combining iterated hash functions, that
the combination is no more difficult to attack than the strongest hash
function used.
https://www.iacr.org/cryptodb/archive/2004/CRYPTO/1472/1472.pdf
Perhaps you already knew about this, but I had only heard rumors
that was the case, until I found that reference recently.
--
see shy jo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: SHA1 collisions found
From: Brandon Williams @ 2017-03-02 21:46 UTC (permalink / raw)
To: Jeff King
Cc: Jason Cooper, Junio C Hamano, Linus Torvalds, Ian Jackson,
Joey Hess, Git Mailing List
In-Reply-To: <20170226051834.i37mlqv5wxwz3254@sigill.intra.peff.net>
On 02/26, Jeff King wrote:
> On Sun, Feb 26, 2017 at 01:13:59AM +0000, Jason Cooper wrote:
>
> > On Fri, Feb 24, 2017 at 10:10:01PM -0800, Junio C Hamano wrote:
> > > I was thinking we would need mixed mode support for smoother
> > > transition, but it now seems to me that the approach to stratify the
> > > history into old and new is workable.
> >
> > As someone looking to deploy (and having previously deployed) git in
> > unconventional roles, I'd like to add one caveat. The flag day in the
> > history is great, but I'd like to be able to confirm the integrity of
> > the old history.
> >
> > "Counter-hashing" the blobs is easy enough, but the trees, commits and
> > tags would need to have, iiuc, some sort of cross-reference. As in my
> > previous example, "git tag -v v3.16" also checks the counter hash to
> > further verify the integrity of the history (yes, it *really* needs to
> > check all of the old hashes, but I'd like to make sure I can do step one
> > first).
> >
> > Would there be opposition to counter-hashing the old commits at the flag
> > day?
>
> I don't think a counter-hash needs to be embedded into the git objects
> themselves. If the "modern" repo format stores everything primarily as
> sha-256, say, it will probably need to maintain a (local) mapping table
> of sha1/sha256 equivalence. That table can be generated at any time from
> the object data (though I suspect we'll keep it up to date as objects
> enter the repository).
>
> At the flag day[1], you can make a signed tag with the "correct" mapping
> in the tag body (so part of the actual GPG signed data, not referenced
> by sha1). Then later you can compare that mapping to the object content
> in the repo (or to the local copy of the mapping based on that data).
>
> -Peff
>
> [1] You don't even need to wait until the flag day. You can do it now.
> This is conceptually similar to the git-evtag tool, though it just
> signs the blob contents of the tag's current tree state. Signing the
> whole mapping lets you verify the entirety of history, but of course
> that mapping is quite big: 20 + 32 bytes per object for
> sha1/sha-256, which is ~250MB for the kernel. So you'd probably not
> want to do it more than once.
There were a few of us discussing this sort of approach internally. We
also figured that, given some performance hit, you could maintain your
repo in sha256 and do some translation to sha1 if you need to push or
fetch to a server which has the the repo in a sha1 format. This way you
can convert your repo independently of the rest of the world.
As for storing the translation table, you should really only need to
maintain the table until old clients are phased out and all of the repos
of a project have experienced flag day and have been converted to
sha256.
--
Brandon Williams
^ permalink raw reply
* log -S/-G (aka pickaxe) searches binary files by default
From: Thomas Braun @ 2017-03-03 0:52 UTC (permalink / raw)
To: GIT Mailing-list
Hi,
I happen to have quite large binary files in my repos.
Today I realized that a line like
git log -G a
searches also files found to be binary (or explicitly marked as binary).
Is that on purpose?
The documentation of "-G<regex>" states
"Look for differences whose patch text contains added/removed lines that
match <regex>."
which contradicts the current behaviour. At least for me text != binary.
To reproduce:
$ git init
$ echo -e "a\0b" > data.bin
$ git add data.bin
$ git commit -m "Add new data"
$ git log -p
[...]
diff --git a/data.bin b/data.bin
new file mode 100644
index 0000000..1a23e4b
Binary files /dev/null and b/data.bin differ
$ git log -G a
[...]
Add new data
I've verified the behaviour with git version 2.12.0.windows.1 and git
version 2.12.0.189.g3bc5322 on debian.
If it is on purpose is there a config option to disable that?
Thanks for reading,
Thomas
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-03-02 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <xmqqk287be9l.fsf@gitster.mtv.corp.google.com>
On Thu, Mar 2, 2017 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> My reaction heavily depends on how that "object version" thing
> works.
>
> Would "object version" be like a truncated SHA-1 over the same data
> but with different IV or something, i.e. something that guarantees
> anybody would get the same result given the data to be hashed?
Yes, it does need to be that in practice. So what I was thinking the
object version would be is:
(a) we actually take the object type into account explicitly.
(b) we explicitly add another truncated hash.
The first part we can already do without any actual data structure
changes, since basically all users already know the type of an object
when they look it up.
So we already have information that we could use to narrow down the
hash collision case if we saw one.
There are some (very few) cases where we don't already explicitly have
the object type (a tag reference can be any object, for example, and
existing scripts might ask for "give me the type of this SHA1 object
with "git cat-file -t"), but that just goes back to the whole "yeah,
we'll handle legacy uses and we will look up objects even _without_
the extra version data, so it actually integrates well into the whole
notion.
Basically, once you accept that "hey, we'll just have a list of
objects with that hash", it just makes sense to narrow it down by the
object type we also already have.
But yes, the object type is obviously only two bits of information
(actually, considering the type distribution, probably just one bit),
and it's already encoded in the first hash, so it doesn't actually
help much as "collision avoidance" particularly once you have a
particular attack against that hash in place.
It's just that it *is* extra information that we already have, and
that is very natural to use once you start thinking of the hash lookup
as returning a list of objects. It also mitigates one of the worst
_confusions_ in git, and so basically mitigates the worst-case
downside of an attack basically for free, so it seems like a
no-brainer.
But the real new piece of object version would be a truncated second
hash of the object.
I don't think it matters too much what that second hash is, I would
say that we'd just approximate having a total of 256 bits of hash.
Since we already have basically 160 bits of fairly good hashing, and
roughly 128 bits of that isn't known to be attackable, we'd just use
another hash and truncate that to 128 bits. That would be *way*
overkill in practice, but maybe overkill is what we want. And it
wouldn't really expand the objects all that much more than just
picking a new 256-bit hash would do.
So you'd have to be able to attack both the full SHA1, _and_ whatever
other different good hash to 128 bits.
Linus
PS. if people think that SHA1 is of a good _size_, and only worry
about the known weaknesses of the hashing itself, we'd only need to
get back the bits that the attacks take away from brute force. That's
currently the 80 -> ~63 bits attack, so you'd really only want about
40 bits of second hash to claw us back back up to 80 bits of brute
force (again: brute force is basically sqrt() of the search space, so
half the bits, so adding 40 bits of hash adds 20 bits to the brute
force cost and you'd get back up to the 2**80 we started with).
So 128 bits of secondary hash really is much more than we'd need. 64
bits would probably be fine.
^ permalink raw reply
* Re: SHA1 collisions found
From: Mike Hommey @ 2017-03-03 1:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: Joey Hess, Junio C Hamano, Jeff King, Ian Jackson,
Git Mailing List
In-Reply-To: <CA+55aFzdJcFZQdJ78rvZ92nNZRsKfcUKMCiXwTVYR34JAuznrA@mail.gmail.com>
On Thu, Mar 02, 2017 at 02:27:15PM -0800, Linus Torvalds wrote:
> On Thu, Mar 2, 2017 at 1:54 PM, Joey Hess <id@joeyh.name> wrote:
> >
> > There's a surprising result of combining iterated hash functions, that
> > the combination is no more difficult to attack than the strongest hash
> > function used.
>
> Duh. I should actually have known that. I started reading the paper
> and went "this seems very familiar". I'm pretty sure I've been pointed
> at that paper before (or maybe just a similar one), and I just didn't
> react enough for it to leave a lasting impact.
What if the "object version" is a hash of the content (as opposed to
header + content like the normal git hash)?
Mike
^ permalink raw reply
* [PATCH v2 3/9] setup_git_directory(): avoid changing global state during discovery
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
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, 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).
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()`.
In subsequent patches, we will export the `discover_git_directory()`
function and make use of it.
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 <johannes.schindelin@gmx.de>
---
setup.c | 187 ++++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 112 insertions(+), 75 deletions(-)
diff --git a/setup.c b/setup.c
index 89a0cef9231..edac3c27dc1 100644
--- a/setup.c
+++ b/setup.c
@@ -816,50 +816,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 discover_git_directory(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;
@@ -867,15 +866,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);
}
- if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
- ceil_offset = 1;
+ if (ceil_offset < 0)
+ ceil_offset = min_offset - 2;
/*
- * Test in the following order (relative to the cwd):
+ * Test in the following order (relative to the dir):
* - .git (file containing "gitdir: <path>")
* - .git/
* - ./ (bare)
@@ -887,62 +886,100 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
*/
one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
if (one_filesystem)
- current_device = get_device_or_die(".", NULL, 0);
+ current_device = get_device_or_die(dir->buf, NULL, 0);
for (;;) {
- gitfile = (char*)read_gitfile(DEFAULT_GIT_DIR_ENVIRONMENT);
- if (gitfile)
- gitdirenv = gitfile = xstrdup(gitfile);
- else {
- if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
- gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
- }
+ int offset = dir->len;
+ if (offset > min_offset)
+ strbuf_addch(dir, '/');
+ strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
+ gitdirenv = read_gitfile(dir->buf);
+ if (!gitdirenv && is_git_directory(dir->buf))
+ gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+ strbuf_setlen(dir, offset);
if (gitdirenv) {
- ret = setup_discovered_git_dir(gitdirenv,
- &cwd, offset,
- nongit_ok);
- free(gitfile);
- return ret;
+ strbuf_addstr(gitdir, gitdirenv);
+ return GIT_DIR_DISCOVERED;
}
- free(gitfile);
- if (is_git_directory("."))
- return setup_bare_git_dir(&cwd, offset, nongit_ok);
-
- offset_parent = offset;
- while (--offset_parent > ceil_offset &&
- !is_dir_sep(dir->buf[offset_parent]));
- if (offset_parent <= ceil_offset)
- return setup_nongit(cwd.buf, nongit_ok);
- if (one_filesystem) {
- dev_t parent_device = get_device_or_die("..", cwd.buf,
- offset);
- if (parent_device != current_device) {
- if (nongit_ok) {
- if (chdir(cwd.buf))
- die_errno(_("Cannot come back to cwd"));
- *nongit_ok = 1;
- return NULL;
- }
- strbuf_setlen(&cwd, offset);
- die(_("Not a git repository (or any parent up to mount point %s)\n"
- "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
- cwd.buf);
- }
- }
- if (chdir("..")) {
- strbuf_setlen(&cwd, offset);
- die_errno(_("Cannot change to '%s/..'"), cwd.buf);
+ if (is_git_directory(dir->buf)) {
+ strbuf_addstr(gitdir, ".");
+ return GIT_DIR_BARE;
}
- offset = offset_parent;
+
+ if (offset <= min_offset)
+ return GIT_DIR_HIT_CEILING;
+
+ while (--offset > ceil_offset && !is_dir_sep(dir->buf[offset]));
+ if (offset <= ceil_offset)
+ return GIT_DIR_HIT_CEILING;
+
+ strbuf_setlen(dir, offset > min_offset ? offset : min_offset);
+ if (one_filesystem &&
+ current_device != get_device_or_die(dir->buf, NULL, offset))
+ return GIT_DIR_HIT_MOUNT_POINT;
}
}
const char *setup_git_directory_gently(int *nongit_ok)
{
+ struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
const char *prefix;
- prefix = setup_git_directory_gently_1(nongit_ok);
+ /*
+ * 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"));
+ strbuf_addbuf(&dir, &cwd);
+
+ switch (discover_git_directory(&dir, &gitdir)) {
+ case GIT_DIR_NONE:
+ prefix = NULL;
+ break;
+ case GIT_DIR_EXPLICIT:
+ prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
+ break;
+ case GIT_DIR_DISCOVERED:
+ if (dir.len < cwd.len && chdir(dir.buf))
+ die(_("Cannot change to '%s'"), dir.buf);
+ prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
+ nongit_ok);
+ break;
+ case GIT_DIR_BARE:
+ if (dir.len < cwd.len && chdir(dir.buf))
+ die(_("Cannot change to '%s'"), dir.buf);
+ prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
+ break;
+ case GIT_DIR_HIT_CEILING:
+ prefix = setup_nongit(cwd.buf, nongit_ok);
+ break;
+ case GIT_DIR_HIT_MOUNT_POINT:
+ if (nongit_ok) {
+ *nongit_ok = 1;
+ return NULL;
+ }
+ die(_("Not a git repository (or any parent up to mount point %s)\n"
+ "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
+ dir.buf);
+ default:
+ die("BUG: unhandled discover_git_directory() result");
+ }
+
if (prefix)
setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
else
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 4/9] Export the discover_git_directory() function
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
The function we introduced earlier 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.
Let's rename it and export a 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 <johannes.schindelin@gmx.de>
---
cache.h | 1 +
setup.c | 29 ++++++++++++++++++++++++++---
2 files changed, 27 insertions(+), 3 deletions(-)
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);
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 edac3c27dc1..7ceca6cc6ef 100644
--- a/setup.c
+++ b/setup.c
@@ -839,8 +839,8 @@ enum discovery_result {
* the discovered .git/ directory, if any. This path may be relative against
* `dir` (i.e. *not* necessarily the cwd).
*/
-static enum discovery_result discover_git_directory(struct strbuf *dir,
- struct strbuf *gitdir)
+static enum discovery_result discover_git_directory_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;
@@ -921,6 +921,29 @@ static enum discovery_result discover_git_directory(struct strbuf *dir,
}
}
+const char *discover_git_directory(struct strbuf *gitdir)
+{
+ struct strbuf dir = STRBUF_INIT;
+ int len;
+
+ 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);
+
+ return gitdir->buf;
+}
+
const char *setup_git_directory_gently(int *nongit_ok)
{
struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
@@ -947,7 +970,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
die_errno(_("Unable to read current working directory"));
strbuf_addbuf(&dir, &cwd);
- switch (discover_git_directory(&dir, &gitdir)) {
+ switch (discover_git_directory_1(&dir, &gitdir)) {
case GIT_DIR_NONE:
prefix = NULL;
break;
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 2/9] setup_git_directory(): use is_dir_sep() helper
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
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 <johannes.schindelin@gmx.de>
---
setup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 967f289f1ef..89a0cef9231 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(dir->buf[offset_parent]));
if (offset_parent <= ceil_offset)
return setup_nongit(cwd.buf, nongit_ok);
if (one_filesystem) {
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 1/9] t7006: replace dubious test
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
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 <johannes.schindelin@gmx.de>
---
| 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c8dc665f2fd..427bfc605ad 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_pager expect_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 -e cwd-retained
+ )
+'
+
test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense'
test_pager_choices 'git shortlog'
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox