* Re: [PATCH v6 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Johannes Sixt @ 2016-12-01 7:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-6-git-send-email-bmwill@google.com>
Am 01.12.2016 um 02:28 schrieb Brandon Williams:
> + git init "su:b" &&
Don't do that. Colons in file names won't work on Windows.
-- Hannes
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Jeff King @ 2016-12-01 7:19 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Brandon Williams, Junio C Hamano, git
In-Reply-To: <fa54bb08-4206-ebd5-6808-f7de6cf4b9a2@kdbg.org>
On Thu, Dec 01, 2016 at 08:09:16AM +0100, Johannes Sixt wrote:
> Am 01.12.2016 um 00:40 schrieb Jeff King:
> > 20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
>
> Side note: where does this weirdness "su:b" come from? (I haven't looked at
> any of the patches, yet, let alone tested.) Colons in file or directory
> names won't work on Windows (in the way one would expect).
It's explicitly used in the test, I assume to check that the recursive
grep is not confused into treating the name as a tree-ish.
I think it would have to either be marked with a prereq on Windows, or
modified to do the whole thing in-index (and use "grep --cached").
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Johannes Sixt @ 2016-12-01 7:09 UTC (permalink / raw)
To: Jeff King, Brandon Williams; +Cc: Junio C Hamano, git
In-Reply-To: <20161130234056.iltitkszvccbjivp@sigill.intra.peff.net>
Am 01.12.2016 um 00:40 schrieb Jeff King:
> 20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
Side note: where does this weirdness "su:b" come from? (I haven't looked
at any of the patches, yet, let alone tested.) Colons in file or
directory names won't work on Windows (in the way one would expect).
-- Hannes
^ permalink raw reply
* Re: 'git repack' and repack.writeBitmaps=true with kept packs
From: Jeff King @ 2016-12-01 5:16 UTC (permalink / raw)
To: Steven Noonan; +Cc: git
In-Reply-To: <CAKbGBLjZ2WLVRM9f=by337xLhPgKCy10T8ra6Qz7OWA=QF-5yA@mail.gmail.com>
On Wed, Nov 30, 2016 at 04:15:33PM -0800, Steven Noonan wrote:
> It seems like it's behaving as though I've provided
> --pack-kept-objects. In order to ensure the .bitmap is created, it
> repacks everything, including everything in existing .pack files (not
> respecting .keep). But then it's not deleting the old .pack file
> (oddly, respecting .keep).
Right, that's exactly what's happening.
The bitmaps require a completely reachable set inside the pack, so if
you omit some objects that are in .keep packs, we cannot generate the
bitmap. So we have to either disable bitmaps, or pack the kept objects.
By default, we do the latter (and I'll explain why in a minute).
We can't delete the .keep packfiles because we don't know for sure that
we've included all of their contents in the new pack (not to mention
that somebody asked to keep them, and we don't know why; we should
respect that).
> What I'd expect it to do here is ignore the 'repack.writeBitmaps =
> true' value if there's a .keep that needs to be respected. Is this not
> a correct assumption?
In practice, I think that ends up worse. The .keep files are used by
receive-pack as lockfiles for incoming pushes. So imagine you kick off a
full repack just as somebody is pushing, and repack sees the temporary
.keep file. Your options are:
1. Disable bitmaps, leaving the repository with no bitmaps at all
until the next repack (because you're deleting the old bitmaps
along with the old, non-kept pack).
2. Duplicate the newly pushed objects in the pack (if they're even
reachable; you're also racing to see the ref updates). Now you have
bitmaps, but you're wasting a bit of space to store the racy push
twice (and it goes away next time you repack).
If you're running a Git server which depends on bitmaps for good
performance, then (2) is much better. And that's the default.
If you want to override it, you can pass --no-pack-kept-objects, or set
repack.packKeptObjects to false.
I think the documentation for --pack-kept-objects could be a bit more
clear for this case. It doesn't mention the default value, nor that what
you really want with "-b" is probably "--no-pack-kept-objects".
-Peff
^ permalink raw reply
* Re: [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Jeff King @ 2016-12-01 4:52 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <alpine.DEB.2.10.1611302310240.20145@buzzword-bingo.mit.edu>
On Wed, Nov 30, 2016 at 11:26:43PM -0500, Anders Kaseorg wrote:
> > So I suspect a better strategy in general is to just override the
> > uname_* variables when cross-compiling.
>
> The specific case of an i386 userspace on an x86_64 kernel is important
> independently of the general cross compilation problem (in fact, the words
> “cross compilation” may not even really apply here). And I don’t think
> one should have to manually tweak the build for this setup, especially
> since the compiler already has the needed information.
Ah, I mistook that you were really cross-compiling x86-64 from i386, in
which case you'd generally have to set CC, etc for the cross-compile
chain. I agree this is a much more subtle case, and it's nice for it to
work out of the box.
> diff --git a/Makefile b/Makefile
> index f53fcc90d..c237d4f91 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -341,7 +341,6 @@ all::
> # Define XDL_FAST_HASH to use an alternative line-hashing method in
> # the diff algorithm. It gives a nice speedup if your processor has
> # fast unaligned word loads. Does NOT work on big-endian systems!
> -# Enabled by default on x86_64.
This is a nice incremental step in the sense that people can still
enable it if they want to in order to time it, play with it, etc. But
given what we know, I wonder if the help text here should warn people.
Or I guess we could move straight to dropping it entirely.
Here's what that patch might look like (I retimed it just be sure, and
was sad to see that it really _is_ making some cases faster. But I still
think slower-but-predictable is a better default).
I didn't drop uname_M here. If we go this route, I think it would make
sense to do that in a separate patch on top, with your commit message
explaining why it is a bad idea versus using compiler-defined macros.
-- >8 --
Subject: [PATCH] xdiff: drop XDL_FAST_HASH
The xdiff code hashes every line of both sides of a diff,
and then compares those hashes to find duplicates. The
overall performance depends both on how fast we can compute
the hashes, but also on how many hash collisions we see.
The idea of XDL_FAST_HASH is to speed up the hash
computation. But the generated hashes have worse collision
behavior. This means that in some cases it speeds diffs up
(running "git log -p" on git.git improves by ~8% with it),
but in others it can slow things down. One pathological case
saw over a 100x slowdown[1].
There may be a better hash function that covers both
properties, but in the meantime we are better off with the
original hash. It's slightly slower in the common case, but
it has fewer surprising pathological cases.
[1] http://public-inbox.org/git/20141222041944.GA441@peff.net/
Signed-off-by: Jeff King <peff@peff.net>
---
Makefile | 9 -----
config.mak.uname | 3 --
xdiff/xutils.c | 106 -------------------------------------------------------
3 files changed, 118 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..f61076997 100644
--- a/Makefile
+++ b/Makefile
@@ -338,11 +338,6 @@ all::
#
# Define NATIVE_CRLF if your platform uses CRLF for line endings.
#
-# Define XDL_FAST_HASH to use an alternative line-hashing method in
-# the diff algorithm. It gives a nice speedup if your processor has
-# fast unaligned word loads. Does NOT work on big-endian systems!
-# Enabled by default on x86_64.
-#
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
#
@@ -1485,10 +1480,6 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
MSGFMT += --check --statistics
endif
-ifneq (,$(XDL_FAST_HASH))
- BASIC_CFLAGS += -DXDL_FAST_HASH
-endif
-
ifdef GMTIME_UNRELIABLE_ERRORS
COMPAT_OBJS += compat/gmtime.o
BASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..447f36ac2 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -17,9 +17,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 027192a1c..04d7b32e4 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -264,110 +264,6 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
return ha;
}
-#ifdef XDL_FAST_HASH
-
-#define REPEAT_BYTE(x) ((~0ul / 0xff) * (x))
-
-#define ONEBYTES REPEAT_BYTE(0x01)
-#define NEWLINEBYTES REPEAT_BYTE(0x0a)
-#define HIGHBITS REPEAT_BYTE(0x80)
-
-/* Return the high bit set in the first byte that is a zero */
-static inline unsigned long has_zero(unsigned long a)
-{
- return ((a - ONEBYTES) & ~a) & HIGHBITS;
-}
-
-static inline long count_masked_bytes(unsigned long mask)
-{
- if (sizeof(long) == 8) {
- /*
- * Jan Achrenius on G+: microoptimized version of
- * the simpler "(mask & ONEBYTES) * ONEBYTES >> 56"
- * that works for the bytemasks without having to
- * mask them first.
- */
- /*
- * return mask * 0x0001020304050608 >> 56;
- *
- * Doing it like this avoids warnings on 32-bit machines.
- */
- long a = (REPEAT_BYTE(0x01) / 0xff + 1);
- return mask * a >> (sizeof(long) * 7);
- } else {
- /* Carl Chatfield / Jan Achrenius G+ version for 32-bit */
- /* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */
- long a = (0x0ff0001 + mask) >> 23;
- /* Fix the 1 for 00 case */
- return a & mask;
- }
-}
-
-unsigned long xdl_hash_record(char const **data, char const *top, long flags)
-{
- unsigned long hash = 5381;
- unsigned long a = 0, mask = 0;
- char const *ptr = *data;
- char const *end = top - sizeof(unsigned long) + 1;
-
- if (flags & XDF_WHITESPACE_FLAGS)
- return xdl_hash_record_with_whitespace(data, top, flags);
-
- ptr -= sizeof(unsigned long);
- do {
- hash += hash << 5;
- hash ^= a;
- ptr += sizeof(unsigned long);
- if (ptr >= end)
- break;
- a = *(unsigned long *)ptr;
- /* Do we have any '\n' bytes in this word? */
- mask = has_zero(a ^ NEWLINEBYTES);
- } while (!mask);
-
- if (ptr >= end) {
- /*
- * There is only a partial word left at the end of the
- * buffer. Because we may work with a memory mapping,
- * we have to grab the rest byte by byte instead of
- * blindly reading it.
- *
- * To avoid problems with masking in a signed value,
- * we use an unsigned char here.
- */
- const char *p;
- for (p = top - 1; p >= ptr; p--)
- a = (a << 8) + *((const unsigned char *)p);
- mask = has_zero(a ^ NEWLINEBYTES);
- if (!mask)
- /*
- * No '\n' found in the partial word. Make a
- * mask that matches what we read.
- */
- mask = 1UL << (8 * (top - ptr) + 7);
- }
-
- /* The mask *below* the first high bit set */
- mask = (mask - 1) & ~mask;
- mask >>= 7;
- hash += hash << 5;
- hash ^= a & mask;
-
- /* Advance past the last (possibly partial) word */
- ptr += count_masked_bytes(mask);
-
- if (ptr < top) {
- assert(*ptr == '\n');
- ptr++;
- }
-
- *data = ptr;
-
- return hash;
-}
-
-#else /* XDL_FAST_HASH */
-
unsigned long xdl_hash_record(char const **data, char const *top, long flags) {
unsigned long ha = 5381;
char const *ptr = *data;
@@ -384,8 +280,6 @@ unsigned long xdl_hash_record(char const **data, char const *top, long flags) {
return ha;
}
-#endif /* XDL_FAST_HASH */
-
unsigned int xdl_hashbits(unsigned int size) {
unsigned int val = 1, bits = 0;
--
2.11.0.319.gd1e73eb
^ permalink raw reply related
* [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Anders Kaseorg @ 2016-12-01 4:30 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <20161201035914.kftxb4vqmzcqed5r@sigill.intra.peff.net>
Although XDL_FAST_HASH computes hashes slightly faster on some
architectures, its collision characteristics are much worse, resulting
in some pathological diffs running over 100x slower
(http://public-inbox.org/git/20141222041944.GA441@peff.net/).
Furthermore, it was being enabled when ‘uname -m’ returns x86_64, even
if we are cross-compiling for a different architecture. This mistake
was also causing the Debian build reproducibility test to fail
(https://tests.reproducible-builds.org/debian/index_variations.html).
Future architecture-specific definitions should be based on compiler
macros such as __x86_64__ rather than uname.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
[Oops, also resending for Thomas’s new email address. Sorry for the
spam.]
On Wed, 30 Nov 2016, Jeff King wrote:
> However, I think this might be the tip of the iceberg. There are lots of
> Makefile knobs whose defaults are tweaked based on uname output. This
> one caught you because you are cross-compiling across architectures, but
> in theory you could cross-compile for FreeBSD from Linux, or whatever.
>
> So I suspect a better strategy in general is to just override the
> uname_* variables when cross-compiling.
The specific case of an i386 userspace on an x86_64 kernel is important
independently of the general cross compilation problem (in fact, the words
“cross compilation” may not even really apply here). And I don’t think
one should have to manually tweak the build for this setup, especially
since the compiler already has the needed information.
> All that being said, I actually think an easier fix for this particular
> case might be to drop XDL_FAST_HASH entirely.
Works for me.
Anders
Makefile | 1 -
config.mak.uname | 5 -----
2 files changed, 6 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..c237d4f91 100644
--- a/Makefile
+++ b/Makefile
@@ -341,7 +341,6 @@ all::
# Define XDL_FAST_HASH to use an alternative line-hashing method in
# the diff algorithm. It gives a nice speedup if your processor has
# fast unaligned word loads. Does NOT work on big-endian systems!
-# Enabled by default on x86_64.
#
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..2831a68c3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -1,10 +1,8 @@
# Platform specific Makefile tweaks based on uname detection
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
ifdef MSVC
@@ -17,9 +15,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jeff King @ 2016-12-01 4:29 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-2-git-send-email-bmwill@google.com>
On Wed, Nov 30, 2016 at 05:28:29PM -0800, Brandon Williams wrote:
> +/*
> + * Determine if a submodule has been populated at a given 'path'
> + */
> +int is_submodule_populated(const char *path)
> +{
> + int ret = 0;
> + struct stat st;
> + char *gitdir = xstrfmt("%s/.git", path);
> +
> + if (!stat(gitdir, &st))
> + ret = 1;
> +
> + free(gitdir);
> + return ret;
> +}
I don't know if it's worth changing or not, but this could be a bit
shorter:
int is_submodule_populated(const char *path)
{
return !access(mkpath("%s/.git", path), F_OK);
}
There is a file_exists() helper, but it uses lstat(), which I think you
don't want (because you'd prefer to bail on a broken .git symlink). But
access(F_OK) does what you want, I think.
mkpath() is generally an unsafe function because it uses a static
buffer, but it's handy and safe for handing values to syscalls like
this.
I say "I don't know if it's worth it" because what you've written is
fine, and while more lines, it's fairly obvious and safe.
-Peff
^ permalink raw reply
* [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Anders Kaseorg @ 2016-12-01 4:26 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Thomas Rast
Although XDL_FAST_HASH computes hashes slightly faster on some
architectures, its collision characteristics are much worse, resulting
in some pathological diffs running over 100x slower
(http://public-inbox.org/git/20141222041944.GA441@peff.net/).
Furthermore, it was being enabled when ‘uname -m’ returns x86_64, even
if we are cross-compiling for a different architecture. This mistake
was also causing the Debian build reproducibility test to fail
(https://tests.reproducible-builds.org/debian/index_variations.html).
Future architecture-specific definitions should be based on compiler
macros such as __x86_64__ rather than uname.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
On Wed, 30 Nov 2016, Jeff King wrote:
> However, I think this might be the tip of the iceberg. There are lots of
> Makefile knobs whose defaults are tweaked based on uname output. This
> one caught you because you are cross-compiling across architectures, but
> in theory you could cross-compile for FreeBSD from Linux, or whatever.
>
> So I suspect a better strategy in general is to just override the
> uname_* variables when cross-compiling.
The specific case of an i386 userspace on an x86_64 kernel is important
independently of the general cross compilation problem (in fact, the words
“cross compilation” may not even really apply here). And I don’t think
one should have to manually tweak the build for this setup, especially
since the compiler already has the needed information.
> All that being said, I actually think an easier fix for this particular
> case might be to drop XDL_FAST_HASH entirely.
Works for me.
Anders
Makefile | 1 -
config.mak.uname | 5 -----
2 files changed, 6 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..c237d4f91 100644
--- a/Makefile
+++ b/Makefile
@@ -341,7 +341,6 @@ all::
# Define XDL_FAST_HASH to use an alternative line-hashing method in
# the diff algorithm. It gives a nice speedup if your processor has
# fast unaligned word loads. Does NOT work on big-endian systems!
-# Enabled by default on x86_64.
#
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..2831a68c3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -1,10 +1,8 @@
# Platform specific Makefile tweaks based on uname detection
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
ifdef MSVC
@@ -17,9 +15,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v6 0/6] recursively grep across submodules
From: Jeff King @ 2016-12-01 4:22 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
On Wed, Nov 30, 2016 at 05:28:28PM -0800, Brandon Williams wrote:
> v6 fixes a race condition which existed in the 'is_submodule_populated'
> function. Instead of calling 'resolve_gitdir' to check for the existance of a
> .git file/directory, use 'stat'. 'resolve_gitdir' calls 'chdir' which can
> affect other running threads trying to load thier files into a buffer in
> memory.
This one passes my stress-test for t7814 (though I imagine you already
knew that).
I tried to think of things that could go wrong by using a simple stat()
instead of resolve_gitdir(). They should only differ when ".git" for
some reason does not point to a git repository. My initial thought is
that this might be more vocal about errors, because the child process
will complain. But actually, the original would already die if the
".git" file is funny, so we were pretty vocal already.
I also wondered whether the sub-process might skip a bogus ".git" file
and keep looking upward in the filesystem tree (which would confusingly
end up back in the super-project!). But it looks like we bail hard when
we see a ".git" file but it's bogus. Which is probably a good thing in
general for submodules.
I'm not sure any of that is actually even worth worrying about, as such
a setup is broken by definition. I just wanted to think it through as a
devil's advocate, and even that seems pretty reasonable.
-Peff
^ permalink raw reply
* Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Jeff King @ 2016-12-01 4:02 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Johannes Schindelin, GIT Mailing-list
In-Reply-To: <59da5383-16a0-b327-75a8-b4c4ad7bd479@ramsayjones.plus.com>
On Thu, Dec 01, 2016 at 01:18:35AM +0000, Ramsay Jones wrote:
> >> I forgot, we ended up reversing course later and silencing them:
> >>
> >> http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
> >>
> >> By the rationale of that conversation, we should be doing:
> >>
> >> warning("%s", "");
> >>
> >> here.
> >
> > I forgot too. Thanks for digging up that thread.
>
> Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022
> as well.
>
> So, by the same rationale, we should remove -Wno-format-zero-length
> from DEVELOPER_CFLAGS. yes?
I don't have a preference on which direction we go, but yes, right now
we are in an awkward middle ground. We should do one of:
1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure
future patches to do not violate it.
2. Declare warning("") as OK.
I still think the warning is silly, but (1) has value in that it
produces the least surprise and annoyance to various people building
Git.
-Peff
^ permalink raw reply
* Re: [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64
From: Jeff King @ 2016-12-01 3:59 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu>
[resend; the original had an outdated address for Thomas, and I would
definitely like his blessing before removing XDL_FAST_HASH].
On Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote:
> Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,
> even if we are cross-compiling for a different architecture. Check
> the __x86_64__ compiler macro instead.
>
> In addition to fixing the cross compilation bug, this is needed to
> pass the Debian build reproducibility test
> (https://tests.reproducible-builds.org/debian/index_variations.html).
I don't think this is a good approach to fix it. Right now XDL_FAST_HASH
is a Makefile knob that can be turned by the user, and can be used
either to explicitly enable or explicitly disable the feature.
With your patch, building with "make XDL_FAST_HASH=Yes" will still
explicitly enable it, but "make XDL_FAST_HASH=" would no longer disable
it (because even if unset, the compiler would turn it on when it sees
__x86_64__).
And being able to turn it off is important; more on that in a second.
So I think if we wanted to auto-detect based on __x86_64__, we'd
probably need to be able to set it to "auto" or something, and then
#if defined(XDL_FAST_HASH_AUTO) && __x86_64__
#define XDL_FAST_HASH
#endif
or something.
However, I think this might be the tip of the iceberg. There are lots of
Makefile knobs whose defaults are tweaked based on uname output. This
one caught you because you are cross-compiling across architectures, but
in theory you could cross-compile for FreeBSD from Linux, or whatever.
So I suspect a better strategy in general is to just override the
uname_* variables when cross-compiling.
All that being said, I actually think an easier fix for this particular
case might be to drop XDL_FAST_HASH entirely. It computes the hashes
slightly faster, but its collision characteristics are much worse. About
2 years ago I ran across a pathological diff that ran over 100x slower
with XDL_FAST_HASH:
http://public-inbox.org/git/20141222041944.GA441@peff.net/
The discussion veered into whether we should have a randomized hash
secured against DoS attacks. I played around with some alternatives, but
never found anything quite as fast for the "normal" case. And having
disabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for
me.
I'd be happy if somebody wanted to investigate other hash functions
further. But barring that, I think we should drop XDL_FAST_HASH (or at
the very least stop turning it on by default) in the meantime. It's just
not a good tradeoff.
-Peff
^ permalink raw reply
* Re: [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64
From: Jeff King @ 2016-12-01 3:56 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu>
On Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote:
> Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,
> even if we are cross-compiling for a different architecture. Check
> the __x86_64__ compiler macro instead.
>
> In addition to fixing the cross compilation bug, this is needed to
> pass the Debian build reproducibility test
> (https://tests.reproducible-builds.org/debian/index_variations.html).
I don't think this is a good approach to fix it. Right now XDL_FAST_HASH
is a Makefile knob that can be turned by the user, and can be used
either to explicitly enable or explicitly disable the feature.
With your patch, building with "make XDL_FAST_HASH=Yes" will still
explicitly enable it, but "make XDL_FAST_HASH=" would no longer disable
it (because even if unset, the compiler would turn it on when it sees
__x86_64__).
And being able to turn it off is important; more on that in a second.
So I think if we wanted to auto-detect based on __x86_64__, we'd
probably need to be able to set it to "auto" or something, and then
#if defined(XDL_FAST_HASH_AUTO) && __x86_64__
#define XDL_FAST_HASH
#endif
or something.
However, I think this might be the tip of the iceberg. There are lots of
Makefile knobs whose defaults are tweaked based on uname output. This
one caught you because you are cross-compiling across architectures, but
in theory you could cross-compile for FreeBSD from Linux, or whatever.
So I suspect a better strategy in general is to just override the
uname_* variables when cross-compiling.
All that being said, I actually think an easier fix for this particular
case might be to drop XDL_FAST_HASH entirely. It computes the hashes
slightly faster, but its collision characteristics are much worse. About
2 years ago I ran across a pathological diff that ran over 100x slower
with XDL_FAST_HASH:
http://public-inbox.org/git/20141222041944.GA441@peff.net/
The discussion veered into whether we should have a randomized hash
secured against DoS attacks. I played around with some alternatives, but
never found anything quite as fast for the "normal" case. And having
disabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for
me.
I'd be happy if somebody wanted to investigate other hash functions
further. But barring that, I think we should drop XDL_FAST_HASH (or at
the very least stop turning it on by default) in the meantime. It's just
not a good tradeoff.
-Peff
^ permalink raw reply
* [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64
From: Anders Kaseorg @ 2016-12-01 3:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Rast
Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,
even if we are cross-compiling for a different architecture. Check
the __x86_64__ compiler macro instead.
In addition to fixing the cross compilation bug, this is needed to
pass the Debian build reproducibility test
(https://tests.reproducible-builds.org/debian/index_variations.html).
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
config.mak.uname | 5 -----
xdiff/xutils.c | 4 ++++
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..2831a68c3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -1,10 +1,8 @@
# Platform specific Makefile tweaks based on uname detection
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
ifdef MSVC
@@ -17,9 +15,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 027192a1c..f46351fe4 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -264,6 +264,10 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
return ha;
}
+#ifdef __x86_64__
+#define XDL_FAST_HASH
+#endif
+
#ifdef XDL_FAST_HASH
#define REPEAT_BYTE(x) ((~0ul / 0xff) * (x))
--
2.11.0
^ permalink raw reply related
* [PATCH v6 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
Teach grep to recursively search in submodules when provided with a
<tree> object. This allows grep to search a submodule based on the state
of the submodule that is present in a commit of the super project.
When grep is provided with a <tree> object, the name of the object is
prefixed to all output. In order to provide uniformity of output
between the parent and child processes the option `--parent-basename`
has been added so that the child can preface all of it's output with the
name of the parent's object instead of the name of the commit SHA1 of
the submodule. This changes output from the command
`git grep -e. -l --recurse-submodules HEAD`
from:
HEAD:file
<commit sha1 of submodule>:sub/file
to:
HEAD:file
HEAD:sub/file
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-grep.txt | 13 ++++-
builtin/grep.c | 76 ++++++++++++++++++++++++---
t/t7814-grep-recurse-submodules.sh | 103 ++++++++++++++++++++++++++++++++++++-
tree-walk.c | 28 ++++++++++
4 files changed, 211 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 17aa1ba..71f32f3 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -26,7 +26,7 @@ SYNOPSIS
[--threads <num>]
[-f <file>] [-e] <pattern>
[--and|--or|--not|(|)|-e <pattern>...]
- [--recurse-submodules]
+ [--recurse-submodules] [--parent-basename <basename>]
[ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
[--] [<pathspec>...]
@@ -91,7 +91,16 @@ OPTIONS
--recurse-submodules::
Recursively search in each submodule that has been initialized and
- checked out in the repository.
+ checked out in the repository. When used in combination with the
+ <tree> option the prefix of all submodule output will be the name of
+ the parent project's <tree> object.
+
+--parent-basename <basename>::
+ For internal use only. In order to produce uniform output with the
+ --recurse-submodules option, this option can be used to provide the
+ basename of a parent's <tree> object to a submodule so the submodule
+ can prefix its output with the parent's name rather than the SHA1 of
+ the submodule.
-a::
--text::
diff --git a/builtin/grep.c b/builtin/grep.c
index dca0be6..5918a26 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -19,6 +19,7 @@
#include "dir.h"
#include "pathspec.h"
#include "submodule.h"
+#include "submodule-config.h"
static char const * const grep_usage[] = {
N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
@@ -28,6 +29,7 @@ static char const * const grep_usage[] = {
static const char *super_prefix;
static int recurse_submodules;
static struct argv_array submodule_options = ARGV_ARRAY_INIT;
+static const char *parent_basename;
static int grep_submodule_launch(struct grep_opt *opt,
const struct grep_source *gs);
@@ -534,19 +536,53 @@ static int grep_submodule_launch(struct grep_opt *opt,
{
struct child_process cp = CHILD_PROCESS_INIT;
int status, i;
+ const char *end_of_base;
+ const char *name;
struct work_item *w = opt->output_priv;
+ end_of_base = strchr(gs->name, ':');
+ if (gs->identifier && end_of_base)
+ name = end_of_base + 1;
+ else
+ name = gs->name;
+
prepare_submodule_repo_env(&cp.env_array);
/* Add super prefix */
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
- gs->name);
+ name);
argv_array_push(&cp.args, "grep");
+ /*
+ * Add basename of parent project
+ * When performing grep on a tree object the filename is prefixed
+ * with the object's name: 'tree-name:filename'. In order to
+ * provide uniformity of output we want to pass the name of the
+ * parent project's object name to the submodule so the submodule can
+ * prefix its output with the parent's name and not its own SHA1.
+ */
+ if (gs->identifier && end_of_base)
+ argv_array_pushf(&cp.args, "--parent-basename=%.*s",
+ (int) (end_of_base - gs->name),
+ gs->name);
+
/* Add options */
- for (i = 0; i < submodule_options.argc; i++)
+ for (i = 0; i < submodule_options.argc; i++) {
+ /*
+ * If there is a tree identifier for the submodule, add the
+ * rev after adding the submodule options but before the
+ * pathspecs. To do this we listen for the '--' and insert the
+ * sha1 before pushing the '--' onto the child process argv
+ * array.
+ */
+ if (gs->identifier &&
+ !strcmp("--", submodule_options.argv[i])) {
+ argv_array_push(&cp.args, sha1_to_hex(gs->identifier));
+ }
+
argv_array_push(&cp.args, submodule_options.argv[i]);
+ }
cp.git_cmd = 1;
cp.dir = gs->path;
@@ -673,12 +709,22 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
enum interesting match = entry_not_interesting;
struct name_entry entry;
int old_baselen = base->len;
+ struct strbuf name = STRBUF_INIT;
+ int name_base_len = 0;
+ if (super_prefix) {
+ strbuf_addstr(&name, super_prefix);
+ name_base_len = name.len;
+ }
while (tree_entry(tree, &entry)) {
int te_len = tree_entry_len(&entry);
if (match != all_entries_interesting) {
- match = tree_entry_interesting(&entry, base, tn_len, pathspec);
+ strbuf_addstr(&name, base->buf + tn_len);
+ match = tree_entry_interesting(&entry, &name,
+ 0, pathspec);
+ strbuf_setlen(&name, name_base_len);
+
if (match == all_entries_not_interesting)
break;
if (match == entry_not_interesting)
@@ -690,8 +736,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
if (S_ISREG(entry.mode)) {
hit |= grep_sha1(opt, entry.oid->hash, base->buf, tn_len,
check_attr ? base->buf + tn_len : NULL);
- }
- else if (S_ISDIR(entry.mode)) {
+ } else if (S_ISDIR(entry.mode)) {
enum object_type type;
struct tree_desc sub;
void *data;
@@ -707,12 +752,18 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
hit |= grep_tree(opt, pathspec, &sub, base, tn_len,
check_attr);
free(data);
+ } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
+ hit |= grep_submodule(opt, entry.oid->hash, base->buf,
+ base->buf + tn_len);
}
+
strbuf_setlen(base, old_baselen);
if (hit && opt->status_only)
break;
}
+
+ strbuf_release(&name);
return hit;
}
@@ -736,6 +787,10 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
if (!data)
die(_("unable to read tree (%s)"), oid_to_hex(&obj->oid));
+ /* Use parent's name as base when recursing submodules */
+ if (recurse_submodules && parent_basename)
+ name = parent_basename;
+
len = name ? strlen(name) : 0;
strbuf_init(&base, PATH_MAX + len + 1);
if (len) {
@@ -762,6 +817,12 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
for (i = 0; i < nr; i++) {
struct object *real_obj;
real_obj = deref_tag(list->objects[i].item, NULL, 0);
+
+ /* load the gitmodules file for this rev */
+ if (recurse_submodules) {
+ submodule_free();
+ gitmodules_config_sha1(real_obj->oid.hash);
+ }
if (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path)) {
hit = 1;
if (opt->status_only)
@@ -902,6 +963,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
N_("ignore files specified via '.gitignore'"), 1),
OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
N_("recursivley search in each submodule")),
+ OPT_STRING(0, "parent-basename", &parent_basename,
+ N_("basename"),
+ N_("prepend parent project's basename to output")),
OPT_GROUP(""),
OPT_BOOL('v', "invert-match", &opt.invert,
N_("show non-matching lines")),
@@ -1154,7 +1218,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
}
- if (recurse_submodules && (!use_index || untracked || list.nr))
+ if (recurse_submodules && (!use_index || untracked))
die(_("option not supported with --recurse-submodules."));
if (!show_in_pager && !opt.status_only)
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 1019125..9e93fe7 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -84,6 +84,108 @@ test_expect_success 'grep and multiple patterns' '
test_cmp expect actual
'
+test_expect_success 'basic grep tree' '
+ cat >expect <<-\EOF &&
+ HEAD:a:foobar
+ HEAD:b/b:bar
+ HEAD:submodule/a:foobar
+ HEAD:submodule/sub/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep tree HEAD^' '
+ cat >expect <<-\EOF &&
+ HEAD^:a:foobar
+ HEAD^:b/b:bar
+ HEAD^:submodule/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD^ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep tree HEAD^^' '
+ cat >expect <<-\EOF &&
+ HEAD^^:a:foobar
+ HEAD^^:b/b:bar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD^^ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep tree and pathspecs' '
+ cat >expect <<-\EOF &&
+ HEAD:submodule/a:foobar
+ HEAD:submodule/sub/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD -- submodule >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep tree and pathspecs' '
+ cat >expect <<-\EOF &&
+ HEAD:submodule/a:foobar
+ HEAD:submodule/sub/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD -- "submodule*a" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep tree and more pathspecs' '
+ cat >expect <<-\EOF &&
+ HEAD:submodule/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD -- "submodul?/a" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep tree and more pathspecs' '
+ cat >expect <<-\EOF &&
+ HEAD:submodule/sub/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules HEAD -- "submodul*/sub/a" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep recurse submodule colon in name' '
+ git init parent &&
+ test_when_finished "rm -rf parent" &&
+ echo "foobar" >"parent/fi:le" &&
+ git -C parent add "fi:le" &&
+ git -C parent commit -m "add fi:le" &&
+
+ git init "su:b" &&
+ test_when_finished "rm -rf su:b" &&
+ echo "foobar" >"su:b/fi:le" &&
+ git -C "su:b" add "fi:le" &&
+ git -C "su:b" commit -m "add fi:le" &&
+
+ git -C parent submodule add "../su:b" "su:b" &&
+ git -C parent commit -m "add submodule" &&
+
+ cat >expect <<-\EOF &&
+ fi:le:foobar
+ su:b/fi:le:foobar
+ EOF
+ git -C parent grep -e "foobar" --recurse-submodules >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ HEAD:fi:le:foobar
+ HEAD:su:b/fi:le:foobar
+ EOF
+ git -C parent grep -e "foobar" --recurse-submodules HEAD >actual &&
+ test_cmp expect actual
+'
+
test_incompatible_with_recurse_submodules ()
{
test_expect_success "--recurse-submodules and $1 are incompatible" "
@@ -94,6 +196,5 @@ test_incompatible_with_recurse_submodules ()
test_incompatible_with_recurse_submodules --untracked
test_incompatible_with_recurse_submodules --no-index
-test_incompatible_with_recurse_submodules HEAD
test_done
diff --git a/tree-walk.c b/tree-walk.c
index 828f435..ff77605 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
*/
if (ps->recursive && S_ISDIR(entry->mode))
return entry_interesting;
+
+ /*
+ * When matching against submodules with
+ * wildcard characters, ensure that the entry
+ * at least matches up to the first wild
+ * character. More accurate matching can then
+ * be performed in the submodule itself.
+ */
+ if (ps->recursive && S_ISGITLINK(entry->mode) &&
+ !ps_strncmp(item, match + baselen,
+ entry->path,
+ item->nowildcard_len - baselen))
+ return entry_interesting;
}
continue;
@@ -1040,6 +1053,21 @@ static enum interesting do_match(const struct name_entry *entry,
strbuf_setlen(base, base_offset + baselen);
return entry_interesting;
}
+
+ /*
+ * When matching against submodules with
+ * wildcard characters, ensure that the entry
+ * at least matches up to the first wild
+ * character. More accurate matching can then
+ * be performed in the submodule itself.
+ */
+ if (ps->recursive && S_ISGITLINK(entry->mode) &&
+ !ps_strncmp(item, match, base->buf + base_offset,
+ item->nowildcard_len)) {
+ strbuf_setlen(base, base_offset + baselen);
+ return entry_interesting;
+ }
+
strbuf_setlen(base, base_offset + baselen);
/*
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v6 4/6] grep: optionally recurse into submodules
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
Allow grep to recognize submodules and recursively search for patterns in
each submodule. This is done by forking off a process to recursively
call grep on each submodule. The top level --super-prefix option is
used to pass a path to the submodule which can in turn be used to
prepend to output or in pathspec matching logic.
Recursion only occurs for submodules which have been initialized and
checked out by the parent project. If a submodule hasn't been
initialized and checked out it is simply skipped.
In order to support the existing multi-threading infrastructure in grep,
output from each child process is captured in a strbuf so that it can be
later printed to the console in an ordered fashion.
To limit the number of theads that are created, each child process has
half the number of threads as its parents (minimum of 1), otherwise we
potentailly have a fork-bomb.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-grep.txt | 5 +
builtin/grep.c | 300 ++++++++++++++++++++++++++++++++++---
git.c | 2 +-
t/t7814-grep-recurse-submodules.sh | 99 ++++++++++++
4 files changed, 386 insertions(+), 20 deletions(-)
create mode 100755 t/t7814-grep-recurse-submodules.sh
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 0ecea6e..17aa1ba 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -26,6 +26,7 @@ SYNOPSIS
[--threads <num>]
[-f <file>] [-e] <pattern>
[--and|--or|--not|(|)|-e <pattern>...]
+ [--recurse-submodules]
[ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
[--] [<pathspec>...]
@@ -88,6 +89,10 @@ OPTIONS
mechanism. Only useful when searching files in the current directory
with `--no-index`.
+--recurse-submodules::
+ Recursively search in each submodule that has been initialized and
+ checked out in the repository.
+
-a::
--text::
Process binary files as if they were text.
diff --git a/builtin/grep.c b/builtin/grep.c
index 8887b6a..dca0be6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -18,12 +18,20 @@
#include "quote.h"
#include "dir.h"
#include "pathspec.h"
+#include "submodule.h"
static char const * const grep_usage[] = {
N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
NULL
};
+static const char *super_prefix;
+static int recurse_submodules;
+static struct argv_array submodule_options = ARGV_ARRAY_INIT;
+
+static int grep_submodule_launch(struct grep_opt *opt,
+ const struct grep_source *gs);
+
#define GREP_NUM_THREADS_DEFAULT 8
static int num_threads;
@@ -174,7 +182,10 @@ static void *run(void *arg)
break;
opt->output_priv = w;
- hit |= grep_source(opt, &w->source);
+ if (w->source.type == GREP_SOURCE_SUBMODULE)
+ hit |= grep_submodule_launch(opt, &w->source);
+ else
+ hit |= grep_source(opt, &w->source);
grep_source_clear_data(&w->source);
work_done(w);
}
@@ -300,6 +311,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
if (opt->relative && opt->prefix_length) {
quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);
strbuf_insert(&pathbuf, 0, filename, tree_name_len);
+ } else if (super_prefix) {
+ strbuf_add(&pathbuf, filename, tree_name_len);
+ strbuf_addstr(&pathbuf, super_prefix);
+ strbuf_addstr(&pathbuf, filename + tree_name_len);
} else {
strbuf_addstr(&pathbuf, filename);
}
@@ -328,10 +343,13 @@ static int grep_file(struct grep_opt *opt, const char *filename)
{
struct strbuf buf = STRBUF_INIT;
- if (opt->relative && opt->prefix_length)
+ if (opt->relative && opt->prefix_length) {
quote_path_relative(filename, opt->prefix, &buf);
- else
+ } else {
+ if (super_prefix)
+ strbuf_addstr(&buf, super_prefix);
strbuf_addstr(&buf, filename);
+ }
#ifndef NO_PTHREADS
if (num_threads) {
@@ -378,31 +396,260 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
exit(status);
}
-static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int cached)
+static void compile_submodule_options(const struct grep_opt *opt,
+ const struct pathspec *pathspec,
+ int cached, int untracked,
+ int opt_exclude, int use_index,
+ int pattern_type_arg)
+{
+ struct grep_pat *pattern;
+ int i;
+
+ if (recurse_submodules)
+ argv_array_push(&submodule_options, "--recurse-submodules");
+
+ if (cached)
+ argv_array_push(&submodule_options, "--cached");
+ if (!use_index)
+ argv_array_push(&submodule_options, "--no-index");
+ if (untracked)
+ argv_array_push(&submodule_options, "--untracked");
+ if (opt_exclude > 0)
+ argv_array_push(&submodule_options, "--exclude-standard");
+
+ if (opt->invert)
+ argv_array_push(&submodule_options, "-v");
+ if (opt->ignore_case)
+ argv_array_push(&submodule_options, "-i");
+ if (opt->word_regexp)
+ argv_array_push(&submodule_options, "-w");
+ switch (opt->binary) {
+ case GREP_BINARY_NOMATCH:
+ argv_array_push(&submodule_options, "-I");
+ break;
+ case GREP_BINARY_TEXT:
+ argv_array_push(&submodule_options, "-a");
+ break;
+ default:
+ break;
+ }
+ if (opt->allow_textconv)
+ argv_array_push(&submodule_options, "--textconv");
+ if (opt->max_depth != -1)
+ argv_array_pushf(&submodule_options, "--max-depth=%d",
+ opt->max_depth);
+ if (opt->linenum)
+ argv_array_push(&submodule_options, "-n");
+ if (!opt->pathname)
+ argv_array_push(&submodule_options, "-h");
+ if (!opt->relative)
+ argv_array_push(&submodule_options, "--full-name");
+ if (opt->name_only)
+ argv_array_push(&submodule_options, "-l");
+ if (opt->unmatch_name_only)
+ argv_array_push(&submodule_options, "-L");
+ if (opt->null_following_name)
+ argv_array_push(&submodule_options, "-z");
+ if (opt->count)
+ argv_array_push(&submodule_options, "-c");
+ if (opt->file_break)
+ argv_array_push(&submodule_options, "--break");
+ if (opt->heading)
+ argv_array_push(&submodule_options, "--heading");
+ if (opt->pre_context)
+ argv_array_pushf(&submodule_options, "--before-context=%d",
+ opt->pre_context);
+ if (opt->post_context)
+ argv_array_pushf(&submodule_options, "--after-context=%d",
+ opt->post_context);
+ if (opt->funcname)
+ argv_array_push(&submodule_options, "-p");
+ if (opt->funcbody)
+ argv_array_push(&submodule_options, "-W");
+ if (opt->all_match)
+ argv_array_push(&submodule_options, "--all-match");
+ if (opt->debug)
+ argv_array_push(&submodule_options, "--debug");
+ if (opt->status_only)
+ argv_array_push(&submodule_options, "-q");
+
+ switch (pattern_type_arg) {
+ case GREP_PATTERN_TYPE_BRE:
+ argv_array_push(&submodule_options, "-G");
+ break;
+ case GREP_PATTERN_TYPE_ERE:
+ argv_array_push(&submodule_options, "-E");
+ break;
+ case GREP_PATTERN_TYPE_FIXED:
+ argv_array_push(&submodule_options, "-F");
+ break;
+ case GREP_PATTERN_TYPE_PCRE:
+ argv_array_push(&submodule_options, "-P");
+ break;
+ case GREP_PATTERN_TYPE_UNSPECIFIED:
+ break;
+ }
+
+ for (pattern = opt->pattern_list; pattern != NULL;
+ pattern = pattern->next) {
+ switch (pattern->token) {
+ case GREP_PATTERN:
+ argv_array_pushf(&submodule_options, "-e%s",
+ pattern->pattern);
+ break;
+ case GREP_AND:
+ case GREP_OPEN_PAREN:
+ case GREP_CLOSE_PAREN:
+ case GREP_NOT:
+ case GREP_OR:
+ argv_array_push(&submodule_options, pattern->pattern);
+ break;
+ /* BODY and HEAD are not used by git-grep */
+ case GREP_PATTERN_BODY:
+ case GREP_PATTERN_HEAD:
+ break;
+ }
+ }
+
+ /*
+ * Limit number of threads for child process to use.
+ * This is to prevent potential fork-bomb behavior of git-grep as each
+ * submodule process has its own thread pool.
+ */
+ argv_array_pushf(&submodule_options, "--threads=%d",
+ (num_threads + 1) / 2);
+
+ /* Add Pathspecs */
+ argv_array_push(&submodule_options, "--");
+ for (i = 0; i < pathspec->nr; i++)
+ argv_array_push(&submodule_options,
+ pathspec->items[i].original);
+}
+
+/*
+ * Launch child process to grep contents of a submodule
+ */
+static int grep_submodule_launch(struct grep_opt *opt,
+ const struct grep_source *gs)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int status, i;
+ struct work_item *w = opt->output_priv;
+
+ prepare_submodule_repo_env(&cp.env_array);
+
+ /* Add super prefix */
+ argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
+ super_prefix ? super_prefix : "",
+ gs->name);
+ argv_array_push(&cp.args, "grep");
+
+ /* Add options */
+ for (i = 0; i < submodule_options.argc; i++)
+ argv_array_push(&cp.args, submodule_options.argv[i]);
+
+ cp.git_cmd = 1;
+ cp.dir = gs->path;
+
+ /*
+ * Capture output to output buffer and check the return code from the
+ * child process. A '0' indicates a hit, a '1' indicates no hit and
+ * anything else is an error.
+ */
+ status = capture_command(&cp, &w->out, 0);
+ if (status && (status != 1)) {
+ /* flush the buffer */
+ write_or_die(1, w->out.buf, w->out.len);
+ die("process for submodule '%s' failed with exit code: %d",
+ gs->name, status);
+ }
+
+ /* invert the return code to make a hit equal to 1 */
+ return !status;
+}
+
+/*
+ * Prep grep structures for a submodule grep
+ * sha1: the sha1 of the submodule or NULL if using the working tree
+ * filename: name of the submodule including tree name of parent
+ * path: location of the submodule
+ */
+static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
+ const char *filename, const char *path)
+{
+ if (!is_submodule_initialized(path))
+ return 0;
+ if (!is_submodule_populated(path))
+ return 0;
+
+#ifndef NO_PTHREADS
+ if (num_threads) {
+ add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
+ return 0;
+ } else
+#endif
+ {
+ struct work_item w;
+ int hit;
+
+ grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
+ filename, path, sha1);
+ strbuf_init(&w.out, 0);
+ opt->output_priv = &w;
+ hit = grep_submodule_launch(opt, &w.source);
+
+ write_or_die(1, w.out.buf, w.out.len);
+
+ grep_source_clear(&w.source);
+ strbuf_release(&w.out);
+ return hit;
+ }
+}
+
+static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
+ int cached)
{
int hit = 0;
int nr;
+ struct strbuf name = STRBUF_INIT;
+ int name_base_len = 0;
+ if (super_prefix) {
+ name_base_len = strlen(super_prefix);
+ strbuf_addstr(&name, super_prefix);
+ }
+
read_cache();
for (nr = 0; nr < active_nr; nr++) {
const struct cache_entry *ce = active_cache[nr];
- if (!S_ISREG(ce->ce_mode))
- continue;
- if (!ce_path_match(ce, pathspec, NULL))
+ strbuf_setlen(&name, name_base_len);
+ strbuf_addstr(&name, ce->name);
+
+ if (S_ISREG(ce->ce_mode) &&
+ match_pathspec(pathspec, name.buf, name.len, 0, NULL,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ /*
+ * If CE_VALID is on, we assume worktree file and its
+ * cache entry are identical, even if worktree file has
+ * been modified, so use cache version instead
+ */
+ if (cached || (ce->ce_flags & CE_VALID) ||
+ ce_skip_worktree(ce)) {
+ if (ce_stage(ce) || ce_intent_to_add(ce))
+ continue;
+ hit |= grep_sha1(opt, ce->oid.hash, ce->name,
+ 0, ce->name);
+ } else {
+ hit |= grep_file(opt, ce->name);
+ }
+ } else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(pathspec, name.buf, NULL)) {
+ hit |= grep_submodule(opt, NULL, ce->name, ce->name);
+ } else {
continue;
- /*
- * If CE_VALID is on, we assume worktree file and its cache entry
- * are identical, even if worktree file has been modified, so use
- * cache version instead
- */
- if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {
- if (ce_stage(ce) || ce_intent_to_add(ce))
- continue;
- hit |= grep_sha1(opt, ce->oid.hash, ce->name, 0,
- ce->name);
}
- else
- hit |= grep_file(opt, ce->name);
+
if (ce_stage(ce)) {
do {
nr++;
@@ -413,6 +660,8 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
if (hit && opt->status_only)
break;
}
+
+ strbuf_release(&name);
return hit;
}
@@ -651,6 +900,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
N_("search in both tracked and untracked files")),
OPT_SET_INT(0, "exclude-standard", &opt_exclude,
N_("ignore files specified via '.gitignore'"), 1),
+ OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+ N_("recursivley search in each submodule")),
OPT_GROUP(""),
OPT_BOOL('v', "invert-match", &opt.invert,
N_("show non-matching lines")),
@@ -755,6 +1006,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
init_grep_defaults();
git_config(grep_cmd_config, NULL);
grep_init(&opt, prefix);
+ super_prefix = get_super_prefix();
/*
* If there is no -- then the paths must exist in the working
@@ -872,6 +1124,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
pathspec.max_depth = opt.max_depth;
pathspec.recursive = 1;
+ if (recurse_submodules) {
+ gitmodules_config();
+ compile_submodule_options(&opt, &pathspec, cached, untracked,
+ opt_exclude, use_index,
+ pattern_type_arg);
+ }
+
if (show_in_pager && (cached || list.nr))
die(_("--open-files-in-pager only works on the worktree"));
@@ -895,6 +1154,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
}
+ if (recurse_submodules && (!use_index || untracked || list.nr))
+ die(_("option not supported with --recurse-submodules."));
+
if (!show_in_pager && !opt.status_only)
setup_pager();
diff --git a/git.c b/git.c
index efa1059..a156efd 100644
--- a/git.c
+++ b/git.c
@@ -434,7 +434,7 @@ static struct cmd_struct commands[] = {
{ "fsck-objects", cmd_fsck, RUN_SETUP },
{ "gc", cmd_gc, RUN_SETUP },
{ "get-tar-commit-id", cmd_get_tar_commit_id },
- { "grep", cmd_grep, RUN_SETUP_GENTLY },
+ { "grep", cmd_grep, RUN_SETUP_GENTLY | SUPPORT_SUPER_PREFIX },
{ "hash-object", cmd_hash_object },
{ "help", cmd_help },
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
new file mode 100755
index 0000000..1019125
--- /dev/null
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='Test grep recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly greps across
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodule' '
+ echo "foobar" >a &&
+ mkdir b &&
+ echo "bar" >b/b &&
+ git add a b &&
+ git commit -m "add a and b" &&
+ git init submodule &&
+ echo "foobar" >submodule/a &&
+ git -C submodule add a &&
+ git -C submodule commit -m "add a" &&
+ git submodule add ./submodule &&
+ git commit -m "added submodule"
+'
+
+test_expect_success 'grep correctly finds patterns in a submodule' '
+ cat >expect <<-\EOF &&
+ a:foobar
+ b/b:bar
+ submodule/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep and basic pathspecs' '
+ cat >expect <<-\EOF &&
+ submodule/a:foobar
+ EOF
+
+ git grep -e. --recurse-submodules -- submodule >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep and nested submodules' '
+ git init submodule/sub &&
+ echo "foobar" >submodule/sub/a &&
+ git -C submodule/sub add a &&
+ git -C submodule/sub commit -m "add a" &&
+ git -C submodule submodule add ./sub &&
+ git -C submodule add sub &&
+ git -C submodule commit -m "added sub" &&
+ git add submodule &&
+ git commit -m "updated submodule" &&
+
+ cat >expect <<-\EOF &&
+ a:foobar
+ b/b:bar
+ submodule/a:foobar
+ submodule/sub/a:foobar
+ EOF
+
+ git grep -e "bar" --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep and multiple patterns' '
+ cat >expect <<-\EOF &&
+ a:foobar
+ submodule/a:foobar
+ submodule/sub/a:foobar
+ EOF
+
+ git grep -e "bar" --and -e "foo" --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep and multiple patterns' '
+ cat >expect <<-\EOF &&
+ b/b:bar
+ EOF
+
+ git grep -e "bar" --and --not -e "foo" --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_incompatible_with_recurse_submodules ()
+{
+ test_expect_success "--recurse-submodules and $1 are incompatible" "
+ test_must_fail git grep -e. --recurse-submodules $1 2>actual &&
+ test_i18ngrep 'not supported with --recurse-submodules' actual
+ "
+}
+
+test_incompatible_with_recurse_submodules --untracked
+test_incompatible_with_recurse_submodules --no-index
+test_incompatible_with_recurse_submodules HEAD
+
+test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v6 3/6] grep: add submodules as a grep source type
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
type in the various switch statements in grep.c.
When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
identifier can either be NULL (to indicate that the working tree will be
used) or a SHA1 (the REV of the submodule to be grep'd). If the
identifier is a SHA1 then we want to fall through to the
`GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
grep.c | 16 +++++++++++++++-
grep.h | 1 +
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/grep.c b/grep.c
index 1194d35..0dbdc1d 100644
--- a/grep.c
+++ b/grep.c
@@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
case GREP_SOURCE_FILE:
gs->identifier = xstrdup(identifier);
break;
+ case GREP_SOURCE_SUBMODULE:
+ if (!identifier) {
+ gs->identifier = NULL;
+ break;
+ }
+ /*
+ * FALL THROUGH
+ * If the identifier is non-NULL (in the submodule case) it
+ * will be a SHA1 that needs to be copied.
+ */
case GREP_SOURCE_SHA1:
gs->identifier = xmalloc(20);
hashcpy(gs->identifier, identifier);
break;
case GREP_SOURCE_BUF:
gs->identifier = NULL;
+ break;
}
}
@@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
switch (gs->type) {
case GREP_SOURCE_FILE:
case GREP_SOURCE_SHA1:
+ case GREP_SOURCE_SUBMODULE:
free(gs->buf);
gs->buf = NULL;
gs->size = 0;
@@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
return grep_source_load_sha1(gs);
case GREP_SOURCE_BUF:
return gs->buf ? 0 : -1;
+ case GREP_SOURCE_SUBMODULE:
+ break;
}
- die("BUG: invalid grep_source type");
+ die("BUG: invalid grep_source type to load");
}
void grep_source_load_driver(struct grep_source *gs)
diff --git a/grep.h b/grep.h
index 5856a23..267534c 100644
--- a/grep.h
+++ b/grep.h
@@ -161,6 +161,7 @@ struct grep_source {
GREP_SOURCE_SHA1,
GREP_SOURCE_FILE,
GREP_SOURCE_BUF,
+ GREP_SOURCE_SUBMODULE,
} type;
void *identifier;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v6 2/6] submodules: load gitmodules file from commit sha1
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
teach submodules to load a '.gitmodules' file from a commit sha1. This
enables the population of the submodule_cache to be based on the state
of the '.gitmodules' file from a particular commit.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
cache.h | 2 ++
config.c | 8 ++++----
submodule-config.c | 6 +++---
submodule-config.h | 3 +++
submodule.c | 12 ++++++++++++
submodule.h | 1 +
6 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index 1be6526..559a461 100644
--- a/cache.h
+++ b/cache.h
@@ -1690,6 +1690,8 @@ extern int git_default_config(const char *, const char *, void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
const char *name, const char *buf, size_t len, void *data);
+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 git_config(config_fn_t fn, void *);
diff --git a/config.c b/config.c
index 83fdecb..4d78e72 100644
--- a/config.c
+++ b/config.c
@@ -1214,10 +1214,10 @@ int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_typ
return do_config_from(&top, fn, data);
}
-static int git_config_from_blob_sha1(config_fn_t fn,
- const char *name,
- const unsigned char *sha1,
- void *data)
+int git_config_from_blob_sha1(config_fn_t fn,
+ const char *name,
+ const unsigned char *sha1,
+ void *data)
{
enum object_type type;
char *buf;
diff --git a/submodule-config.c b/submodule-config.c
index 098085b..8b9a2ef 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -379,9 +379,9 @@ static int parse_config(const char *var, const char *value, void *data)
return ret;
}
-static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
- unsigned char *gitmodules_sha1,
- struct strbuf *rev)
+int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
+ unsigned char *gitmodules_sha1,
+ struct strbuf *rev)
{
int ret = 0;
diff --git a/submodule-config.h b/submodule-config.h
index d05c542..78584ba 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -29,6 +29,9 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
const char *name);
const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path);
+extern int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
+ unsigned char *gitmodules_sha1,
+ struct strbuf *rev);
void submodule_free(void);
#endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index f336ca9..8516ab0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,6 +198,18 @@ void gitmodules_config(void)
}
}
+void gitmodules_config_sha1(const unsigned char *commit_sha1)
+{
+ struct strbuf rev = STRBUF_INIT;
+ unsigned char sha1[20];
+
+ if (gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {
+ git_config_from_blob_sha1(submodule_config, rev.buf,
+ sha1, NULL);
+ }
+ strbuf_release(&rev);
+}
+
/*
* Determine if a submodule has been initialized at a given 'path'
*/
diff --git a/submodule.h b/submodule.h
index 6ec5f2f..9203d89 100644
--- a/submodule.h
+++ b/submodule.h
@@ -37,6 +37,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
extern int is_submodule_initialized(const char *path);
extern int is_submodule_populated(const char *path);
int parse_submodule_update_strategy(const char *value,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v6 6/6] grep: search history of moved submodules
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
If a submodule was renamed at any point since it's inception then if you
were to try and grep on a commit prior to the submodule being moved, you
wouldn't be able to find a working directory for the submodule since the
path in the past is different from the current path.
This patch teaches grep to find the .git directory for a submodule in
the parents .git/modules/ directory in the event the path to the
submodule in the commit that is being searched differs from the state of
the currently checked out commit. If found, the child process that is
spawned to grep the submodule will chdir into its gitdir instead of a
working directory.
In order to override the explicit setting of submodule child process's
gitdir environment variable (which was introduced in '10f5c526')
`GIT_DIR_ENVIORMENT` needs to be pushed onto child process's env_array.
This allows the searching of history from a submodule's gitdir, rather
than from a working directory.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/grep.c | 20 +++++++++++++++++--
t/t7814-grep-recurse-submodules.sh | 41 ++++++++++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 5918a26..2c727ef 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -547,6 +547,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
name = gs->name;
prepare_submodule_repo_env(&cp.env_array);
+ argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
/* Add super prefix */
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
@@ -615,8 +616,23 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
{
if (!is_submodule_initialized(path))
return 0;
- if (!is_submodule_populated(path))
- return 0;
+ if (!is_submodule_populated(path)) {
+ /*
+ * If searching history, check for the presense of the
+ * submodule's gitdir before skipping the submodule.
+ */
+ if (sha1) {
+ const struct submodule *sub =
+ submodule_from_path(null_sha1, path);
+ if (sub)
+ path = git_path("modules/%s", sub->name);
+
+ if (!(is_directory(path) && is_git_directory(path)))
+ return 0;
+ } else {
+ return 0;
+ }
+ }
#ifndef NO_PTHREADS
if (num_threads) {
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 9e93fe7..0507771 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -186,6 +186,47 @@ test_expect_success 'grep recurse submodule colon in name' '
test_cmp expect actual
'
+test_expect_success 'grep history with moved submoules' '
+ git init parent &&
+ test_when_finished "rm -rf parent" &&
+ echo "foobar" >parent/file &&
+ git -C parent add file &&
+ git -C parent commit -m "add file" &&
+
+ git init sub &&
+ test_when_finished "rm -rf sub" &&
+ echo "foobar" >sub/file &&
+ git -C sub add file &&
+ git -C sub commit -m "add file" &&
+
+ git -C parent submodule add ../sub dir/sub &&
+ git -C parent commit -m "add submodule" &&
+
+ cat >expect <<-\EOF &&
+ dir/sub/file:foobar
+ file:foobar
+ EOF
+ git -C parent grep -e "foobar" --recurse-submodules >actual &&
+ test_cmp expect actual &&
+
+ git -C parent mv dir/sub sub-moved &&
+ git -C parent commit -m "moved submodule" &&
+
+ cat >expect <<-\EOF &&
+ file:foobar
+ sub-moved/file:foobar
+ EOF
+ git -C parent grep -e "foobar" --recurse-submodules >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ HEAD^:dir/sub/file:foobar
+ HEAD^:file:foobar
+ EOF
+ git -C parent grep -e "foobar" --recurse-submodules HEAD^ >actual &&
+ test_cmp expect actual
+'
+
test_incompatible_with_recurse_submodules ()
{
test_expect_success "--recurse-submodules and $1 are incompatible" "
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
Add two helper functions to submodules.c.
`is_submodule_initialized()` checks if a submodule has been initialized
at a given path and `is_submodule_populated()` check if a submodule
has been checked out at a given path.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
submodule.c | 39 +++++++++++++++++++++++++++++++++++++++
submodule.h | 2 ++
2 files changed, 41 insertions(+)
diff --git a/submodule.c b/submodule.c
index 6f7d883..f336ca9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,6 +198,45 @@ void gitmodules_config(void)
}
}
+/*
+ * Determine if a submodule has been initialized at a given 'path'
+ */
+int is_submodule_initialized(const char *path)
+{
+ int ret = 0;
+ const struct submodule *module = NULL;
+
+ module = submodule_from_path(null_sha1, path);
+
+ if (module) {
+ char *key = xstrfmt("submodule.%s.url", module->name);
+ char *value = NULL;
+
+ ret = !git_config_get_string(key, &value);
+
+ free(value);
+ free(key);
+ }
+
+ return ret;
+}
+
+/*
+ * Determine if a submodule has been populated at a given 'path'
+ */
+int is_submodule_populated(const char *path)
+{
+ int ret = 0;
+ struct stat st;
+ char *gitdir = xstrfmt("%s/.git", path);
+
+ if (!stat(gitdir, &st))
+ ret = 1;
+
+ free(gitdir);
+ return ret;
+}
+
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
{
diff --git a/submodule.h b/submodule.h
index d9e197a..6ec5f2f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+extern int is_submodule_initialized(const char *path);
+extern int is_submodule_populated(const char *path);
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v6 0/6] recursively grep across submodules
From: Brandon Williams @ 2016-12-01 1:28 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1479840397-68264-1-git-send-email-bmwill@google.com>
v6 fixes a race condition which existed in the 'is_submodule_populated'
function. Instead of calling 'resolve_gitdir' to check for the existance of a
.git file/directory, use 'stat'. 'resolve_gitdir' calls 'chdir' which can
affect other running threads trying to load thier files into a buffer in
memory.
Thanks to Stefan and Jeff for help debugging this problem.
Brandon Williams (6):
submodules: add helper functions to determine presence of submodules
submodules: load gitmodules file from commit sha1
grep: add submodules as a grep source type
grep: optionally recurse into submodules
grep: enable recurse-submodules to work on <tree> objects
grep: search history of moved submodules
Documentation/git-grep.txt | 14 ++
builtin/grep.c | 386 ++++++++++++++++++++++++++++++++++---
cache.h | 2 +
config.c | 8 +-
git.c | 2 +-
grep.c | 16 +-
grep.h | 1 +
submodule-config.c | 6 +-
submodule-config.h | 3 +
submodule.c | 51 +++++
submodule.h | 3 +
t/t7814-grep-recurse-submodules.sh | 241 +++++++++++++++++++++++
tree-walk.c | 28 +++
13 files changed, 730 insertions(+), 31 deletions(-)
create mode 100755 t/t7814-grep-recurse-submodules.sh
--- interdiff based on 'bw/grep-recurse-submodules'
diff --git a/submodule.c b/submodule.c
index 062e58b..8516ab0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -239,9 +239,10 @@ int is_submodule_initialized(const char *path)
int is_submodule_populated(const char *path)
{
int ret = 0;
+ struct stat st;
char *gitdir = xstrfmt("%s/.git", path);
- if (resolve_gitdir(gitdir))
+ if (!stat(gitdir, &st))
ret = 1;
free(gitdir);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion
From: Austin English @ 2016-12-01 1:22 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 207 bytes --]
Found via shellcheck
In Documentation/install-webdoc.sh line 21:
mkdir -p $(dirname "$T/$h")
^-- SC2046: Quote this to prevent word splitting.
--
-Austin
GPG: 14FB D7EA A041 937B
[-- Attachment #2: 0001-Documentation-install-webdoc.sh-quote-a-potentially-.patch --]
[-- Type: text/x-patch, Size: 767 bytes --]
From 1050538f252d22311185065ab8837c71b17003fb Mon Sep 17 00:00:00 2001
From: Austin English <austinenglish@gmail.com>
Date: Wed, 30 Nov 2016 19:21:25 -0600
Subject: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe
shell expansion
Signed-off-by: Austin English <austinenglish@gmail.com>
---
Documentation/install-webdoc.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
index ed8b4ff..5fb2dc5 100755
--- a/Documentation/install-webdoc.sh
+++ b/Documentation/install-webdoc.sh
@@ -18,7 +18,7 @@ do
else
echo >&2 "# install $h $T/$h"
rm -f "$T/$h"
- mkdir -p $(dirname "$T/$h")
+ mkdir -p "$(dirname "$T/$h")"
cp "$h" "$T/$h"
fi
done
--
2.7.3
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Brandon Williams @ 2016-12-01 1:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAGZ79kaaJgsa8SsvWiGkmKQU7Wv_PD6O50ybm=rxPySk=z9N7Q@mail.gmail.com>
On 11/30, Stefan Beller wrote:
> > Oh interesting, I wonder if there is a way to not have to perform a
> > chdir since taking a lock to lstat wouldn't be ideal.
>
> I think we could rewrite is_submodule_populated to be
>
> int is_submodule_populated_cheap_with_no_chdir(char *path)
> {
> return stat(path + ".git")
> }
>
> i.e. just take the presence of the .git file/dir as a hint to run
> the child process?
I like this approach, its a quick (thread-safe) check to see if the
submodule is interesting. If there happens to be an error with the
submodule's .git file/directory then the child process will fail out.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Ramsay Jones @ 2016-12-01 1:18 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Johannes Schindelin, GIT Mailing-list
In-Reply-To: <xmqqinr4bkf4.fsf@gitster.mtv.corp.google.com>
On 30/11/16 23:46, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> I forgot, we ended up reversing course later and silencing them:
>>
>> http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
>>
>> By the rationale of that conversation, we should be doing:
>>
>> warning("%s", "");
>>
>> here.
>
> I forgot too. Thanks for digging up that thread.
Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022
as well.
So, by the same rationale, we should remove -Wno-format-zero-length
from DEVELOPER_CFLAGS. yes?
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH] transport-helper: drop broken "unchanged" feature
From: Jonathan Tan @ 2016-12-01 1:13 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, barkalow
In-Reply-To: <1480553664-12804-1-git-send-email-jonathantanmy@google.com>
Commit f8ec916 ("Allow helpers to report in "list" command that the ref
is unchanged", 2009-11-17) allowed a remote helper to report that a ref
is unchanged even if it did not know the contents of a ref. However,
that commit also made Git wrongly assume that such a remote ref always
has the contents of the local ref of the same name.
(Git also cannot assume that the remote ref has the value of the current
destination local ref, or any other ref, since the previous import/fetch
could have been made using a different refspec.)
Drop that assumption.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Here is a patch that would remove the assumption (if it is indeed
wrong).
Documentation/gitremote-helpers.txt | 3 +++
transport-helper.c | 43 -------------------------------------
2 files changed, 3 insertions(+), 43 deletions(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 9e8681f..c862339 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -393,6 +393,9 @@ attributes are defined.
'unchanged'::
This ref is unchanged since the last import or fetch, although
the helper cannot necessarily determine what value that produced.
+ Git may import or fetch this ref anyway, because it does not
+ keep a record of the last values corresponding to the refs of a
+ specific remote.
OPTIONS
-------
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35..6ab8e2f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -392,9 +392,6 @@ static int fetch_with_fetch(struct transport *transport,
for (i = 0; i < nr_heads; i++) {
const struct ref *posn = to_fetch[i];
- if (posn->status & REF_STATUS_UPTODATE)
- continue;
-
strbuf_addf(&buf, "fetch %s %s\n",
oid_to_hex(&posn->old_oid),
posn->symref ? posn->symref : posn->name);
@@ -492,9 +489,6 @@ static int fetch_with_import(struct transport *transport,
for (i = 0; i < nr_heads; i++) {
posn = to_fetch[i];
- if (posn->status & REF_STATUS_UPTODATE)
- continue;
-
strbuf_addf(&buf, "import %s\n",
posn->symref ? posn->symref : posn->name);
sendline(data, &buf);
@@ -531,8 +525,6 @@ static int fetch_with_import(struct transport *transport,
for (i = 0; i < nr_heads; i++) {
char *private, *name;
posn = to_fetch[i];
- if (posn->status & REF_STATUS_UPTODATE)
- continue;
name = posn->symref ? posn->symref : posn->name;
if (data->refspecs)
private = apply_refspecs(data->refspecs, data->refspec_nr, name);
@@ -649,21 +641,12 @@ static int fetch(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
struct helper_data *data = transport->data;
- int i, count;
if (process_connect(transport, 0)) {
do_take_over(transport);
return transport->fetch(transport, nr_heads, to_fetch);
}
- count = 0;
- for (i = 0; i < nr_heads; i++)
- if (!(to_fetch[i]->status & REF_STATUS_UPTODATE))
- count++;
-
- if (!count)
- return 0;
-
if (data->check_connectivity &&
data->transport_options.check_self_contained_and_connected)
set_helper_option(transport, "check-connectivity", "true");
@@ -1009,23 +992,6 @@ static int push_refs(struct transport *transport,
return -1;
}
-
-static int has_attribute(const char *attrs, const char *attr) {
- int len;
- if (!attrs)
- return 0;
-
- len = strlen(attr);
- for (;;) {
- const char *space = strchrnul(attrs, ' ');
- if (len == space - attrs && !strncmp(attrs, attr, len))
- return 1;
- if (!*space)
- return 0;
- attrs = space + 1;
- }
-}
-
static struct ref *get_refs_list(struct transport *transport, int for_push)
{
struct helper_data *data = transport->data;
@@ -1067,15 +1033,6 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
(*tail)->symref = xstrdup(buf.buf + 1);
else if (buf.buf[0] != '?')
get_oid_hex(buf.buf, &(*tail)->old_oid);
- if (eon) {
- if (has_attribute(eon + 1, "unchanged")) {
- (*tail)->status |= REF_STATUS_UPTODATE;
- if (read_ref((*tail)->name,
- (*tail)->old_oid.hash) < 0)
- die(_("Could not read ref %s"),
- (*tail)->name);
- }
- }
tail = &((*tail)->next);
}
if (debug)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [BUG?] Remote helper's wrong assumption of unchanged ref
From: Jonathan Tan @ 2016-12-01 0:54 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, barkalow
Commit f8ec916 ("Allow helpers to report in "list" command that the ref
is unchanged", 2009-11-17) allowed a remote helper to report that a ref
is unchanged even if it did not know the contents of a ref. However,
that commit also made Git assume that such a remote ref has the contents
of the local ref of the same name. If I'm not missing anything, this
assumption seems wrong; the attached test illustrates one case of local
edits being made after cloning with default parameters.
The original e-mail thread [1] seems to indicate that this feature is
meant for a remote helper with no Git-specific code (which is possible
if it supports "import" but not "fetch" - in this case, it would not
deal with SHA-1s at all) to nevertheless indicate "unchanged", most
likely to support optimizations on the client side.
But it seems to me that Git cannot perform this optimization. In other
words, it should just ignore "unchanged". If this makes sense, I'll
prepare a patch to do this.
[1] "[PATCH 00/13] Native and foreign helpers"
<alpine.LNX.2.00.0908050052390.2147@iabervon.org>
---
git-remote-testgit.sh | 8 +++++++-
t/t5801-remote-helpers.sh | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index 752c763..6357868 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -52,7 +52,13 @@ do
echo
;;
list)
- git for-each-ref --format='? %(refname)' 'refs/heads/'
+ if test -n "$GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX"
+ then
+ git for-each-ref --format='? %(refname)' 'refs/heads/' |
+ sed "/${GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX}/s/$/ unchanged/"
+ else
+ git for-each-ref --format='? %(refname)' 'refs/heads/'
+ fi
head=$(git symbolic-ref HEAD)
echo "@$head HEAD"
echo
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 362b158..4a48f2b 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -301,4 +301,44 @@ test_expect_success 'fetch url' '
compare_refs server HEAD local FETCH_HEAD
'
+test_expect_success 'setup remote repository and divergent clone' '
+ git init s2 &&
+ (
+ cd s2 &&
+ test_commit M1 &&
+ git checkout -b mybranch &&
+ test_commit B1
+ ) &&
+ git clone "testgit::${PWD}/s2" divergent &&
+
+ (
+ cd divergent &&
+ git checkout master &&
+ test_commit M2 &&
+ git checkout mybranch &&
+ test_commit B2
+ )
+'
+
+test_expect_success 'fetch with unchanged claims' '
+ rm -rf local &&
+ cp -r divergent local &&
+
+ # No unchanged branches
+
+ GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX=ABCDE git -C local fetch &&
+ compare_refs s2 M1 local refs/remotes/origin/master &&
+ compare_refs s2 B1 local refs/remotes/origin/mybranch &&
+
+ # One unchanged branch
+
+ GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX=mybranch git -C local fetch &&
+ compare_refs s2 M1 local refs/remotes/origin/master &&
+
+ # I (Jonathan Tan) would expect refs/remotes/origin/mybranch to be B1,
+ # but it is B2.
+ test_must_fail compare_refs s2 B1 local refs/remotes/origin/mybranch &&
+ compare_refs local B2 local refs/remotes/origin/mybranch
+'
+
test_done
--
2.8.0.rc3.226.g39d4020
^ 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