* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Eric Wong @ 2016-10-28 5:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Git Mailing List, Lars Schneider,
Johannes Schindelin
In-Reply-To: <xmqq60od42s0.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >
> >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>> Would the best endgame shape for this function be to open with
> >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
> >>> but ignoring an error from it, I guess? That would be the closest
> >>> to what we historically had, I would think.
> >>
> >> I think that's the best model.
Actually, I would flip the order of flags. O_CLOEXEC is more
important from a correctness standpoint.
> > OK, so perhaps like this.
>
> Hmph. This may not fly well in practice, though.
>
> To Unix folks, CLOEXEC is not a huge correctness issue. A child
> process may hold onto an open file descriptor a bit longer than the
> lifetime of the parent but as long as the child eventually exits,
I'm not too familiar with C internals of git; but I know we use
threads in some places, and fork+execve in others.
If our usage of threads and execve intersects, and we run
untrusted code in an execve-ed child, then only having cloexec
on open() will save us time when auditing for leaking FDs.
fcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other
threads doing execve; so I wouldn't rely on it as a first
choice.
So I suppose something like this:
static int noatime = 1;
int fd = open(... | O_CLOEXEC);
...error checking and retrying...
if (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0)
noatime = 0;
return fd;
^ permalink raw reply
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Stefan Beller @ 2016-10-28 3:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqq1sz1425w.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 7:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Unless you are imagining "git grep" to initialize and checkout a
> submodule that is not checked out on-demand, I do not think you have
> any reason to even look at ".gitmodules" for the purpose of "I want
> to grep both in superproject and submodules that are checked out."
In tree-ish mode you may have this example:
git -C superproject rm path/to/submodule
git -C superproject commit -a -m "delete submodule"
... time passes ...
git -C superproject grep --recurse-submodule -e <expression> \
HEAD~42 path/to/submodule
In the last command you need to map the path to submodule to
the name of the submodule to find out the place of the object store
for that submodule and see if it exists.
> If it is working with a tree-ish, again, go look at the object store
> in that submodule repository.
and to find out the object store for that submodule you need the
path -> name mapping at that point in time, i.e. you want to look
at the .gitmodules file at the given tree-ish.
^ permalink raw reply
* "git push" says "src refspec XYZ matches more than one" even without explicit XYZ argument.
From: Kannan Goundan @ 2016-10-28 3:00 UTC (permalink / raw)
To: git
1. My repo has a branch named 'v1' that is tracking 'origin/v1'.
2. My repo has a tag named 'v1'.
3. I have "push.default" set to "upstream".
I made a commit on branch 'v1' and tried doing a push:
# git push
error: src refspec v1 matches more than one.
error: failed to push some refs to 'git@github.com:whatever/ns1-go.git'
If I rename the branch to 'v1-dev', then the push goes through.
I understand why the command "git push origin/v1 v1" is ambiguous.
But if I do a plain "git push", I thought Git would know to push my
current branch.
[Git version 2.10.1 from Homebrew on Mac OS 10.11.6.]
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Junio C Hamano @ 2016-10-28 2:54 UTC (permalink / raw)
To: Jeff King; +Cc: Aaron Pelly, git
In-Reply-To: <20161027210753.btc7zbndhdocsbwa@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> However, as I said elsewhere, I'm not convinced this feature is all that
> helpful for in-repository .gitignore files, and I think it does
> introduce compatibility complications. People with older git will not
> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
> matter.
As I do not see the point of making in-tree .gitignore to a forest
of .gitignore.d/ at all, compatibility complications is not worth
even thinking about, I would have to say.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Junio C Hamano @ 2016-10-28 2:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kYm1txscyBpmfJQceCLFrZAN09y-2nV1zCjE2a1+_jrLA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> Just a few brief comments, before reading the patches carefully.
>>
>> * It is somewhat surprising that [1/5] is even needed (in other
>> words, I would have expected something like this to be already
>> there, and my knee-jerk reaction was "Heh, how does 'git status'
>> know how to show submodules that are and are not initialized
>> differently without this?"
>
> The issue with much of the existing code is that it is submodule centric,
> i.e. it is written to not care about the rest.
>
> git status for example just calls "git submodule summary" to
> parse and display the submodule information additionally.
> It doesn't integrate submodules and treats them "just like files".
Oh, I know all that after/while writing the above "it is somewhat
surprising" and reading what wt-status.c does. It was just that it
was somewhat surprising ;-)
> My reaction to 1/5 was that the implementation is sound,
> but the design may need rethinking.
>
> Instead of asking all these question, "Is a submodule
> * initialized
> * checked out (== have a working dir)
> * have a .git dir (think of deleted submodules that keep the
> historical git dir around)
> (* have commit X)
> we would want to either extend the submodule-config API
> to also carry these informations just like
> name/path/sha1/url/shallow clone recommendation.
I think you are going in a wrong direction with all the above.
Unless you are imagining "git grep" to initialize and checkout a
submodule that is not checked out on-demand, I do not think you have
any reason to even look at ".gitmodules" for the purpose of "I want
to grep both in superproject and submodules that are checked out."
You only need to detect .gitlink that exists in the index of the
superproject, and then there would be only two cases:
* $path has an empty directory (not even .git in there). The user
is not interested in that submodule.
* $path has ".git", either a directory (old layout or we are
dealing with the repository that originated the submodule) or a
"gitdir:" file that points into .git/modules of the repository of
the superproject. The user is interested in the submodule.
If $path has ".git" and nothing else, the only explanation is that
the user removed the working tree files in the submodule. If your
grep is looking at working tree files, it is correct not to find
anything in there. If it is working with "--cached", go look at the
index of the submodule repository (either the ".git" directory, or
the stashed-away repository in .git/modules/ in the superproject).
If it is working with a tree-ish, again, go look at the object store
in that submodule repository.
>> * It is somewhat surprising that [4/5] does not even use the
>> previous ls-files to find out the paths. Also it is a bit
>> disappointing to see that the way processes are spawned and
>> managed does not share much with Stefan's earlier work, i.e.
>> run_processes_parallel(). I was somehow hoping that it can be
>> extended to support this use case, but apparently there aren't
>> much to be shared.
>
> I think there are 2 issues here:
There is no issue here. I was just giving my impressions (i.e.
"somewhat surprising").
> * git-grep already has its own thread pool
I know. I was expecting that the previous "ls-files" that recurses
will be used to feed into that thread pool, but I didn't find that
in my cursory look at the patch, hence "somewhat surprising".
I hate it when people become overly defensive and start making
excuses when given harmless observations.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-28 2:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <xmqqa8dp46wx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Would the best endgame shape for this function be to open with
>>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
>>> but ignoring an error from it, I guess? That would be the closest
>>> to what we historically had, I would think.
>>
>> I think that's the best model.
>
> OK, so perhaps like this.
Hmph. This may not fly well in practice, though.
To Unix folks, CLOEXEC is not a huge correctness issue. A child
process may hold onto an open file descriptor a bit longer than the
lifetime of the parent but as long as the child eventually exits,
nothing is affected. Over there, things are different. The parent
cannot even rename(2) or unlink(2) a file it created and closed
while the child is still holding the file descriptor open and the
lack of CLOEXEC will make the parent fail. I do not know how well
fcntl(2) emulation works on Windows, but I would not be surprised
if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD
would not work while O_CLOEXEC given to open(2) does.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-28 1:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFw83E+zOd+z5h-CA-3NhrLjVr-anL6pubrSWttYx3zu8g@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Would the best endgame shape for this function be to open with
>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
>> but ignoring an error from it, I guess? That would be the closest
>> to what we historically had, I would think.
>
> I think that's the best model.
OK, so perhaps like this.
-- >8 --
Subject: git_open(): untangle possible NOATIME and CLOEXEC interactions
The way we structured the fallback-retry for opening with O_NOATIME
and O_CLOEXEC meant that if we failed due to lack of support to open
the file with O_NOATIME option (i.e. EINVAL), we would still try to
drop O_CLOEXEC first and retry, and then drop O_NOATIME. A platform
on which O_NOATIME is defined in the header without support from the
kernel wouldn't have a chance to open with O_CLOEXEC option due to
this code structure.
Arguably, O_CLOEXEC is more important than O_NOATIME, as the latter
is mostly about performance, while the former can affect correctness.
Let's revert the recent changes to the way git_open() attempts to
open a file with O_NOATIME and retries without to the original
sequence, and then use a separate fcntl(fd, F_SETFD, FD_CLOEXEC) on
the resulting file descriptor. The helper to do the latter can be
usable in the codepath in ce_compare_data() that was recently added
to open a file descriptor with O_CLOEXEC, so let's refactor that
codepath with the helper while we are at it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-compat-util.h | 5 +++--
read-cache.c | 12 ++++--------
sha1_file.c | 49 ++++++++++++++++++++++++++++++-------------------
3 files changed, 37 insertions(+), 29 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718dabae..a751630db5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -679,9 +679,10 @@ char *gitstrdup(const char *s);
#define getpagesize() sysconf(_SC_PAGESIZE)
#endif
-#ifndef O_CLOEXEC
-#define O_CLOEXEC 0
+#ifndef FD_CLOEXEC
+#define FD_CLOEXEC 0
#endif
+extern int git_set_cloexec(int);
#ifdef FREAD_READS_DIRECTORIES
#ifdef fopen
diff --git a/read-cache.c b/read-cache.c
index db5d910642..fb91514885 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,17 +156,13 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
- static int cloexec = O_CLOEXEC;
- int fd = open(ce->name, O_RDONLY | cloexec);
-
- if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
- /* Try again w/o O_CLOEXEC: the kernel might not support it */
- cloexec &= ~O_CLOEXEC;
- fd = open(ce->name, O_RDONLY | cloexec);
- }
+ int fd = open(ce->name, O_RDONLY);
if (fd >= 0) {
unsigned char sha1[20];
+
+ /* do not let child processes to hold onto the open fd */
+ git_set_cloexec(fd);
if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
match = hashcmp(sha1, ce->oid.hash);
/* index_fd() closed the file descriptor already */
diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dc..41383a6c20 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1559,31 +1559,42 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open(const char *name)
+int git_set_cloexec(int fd)
{
- static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
+ static int cloexec = FD_CLOEXEC;
- for (;;) {
- int fd;
+ if (cloexec) {
+ if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)
+ cloexec = 0;
+ /*
+ * We might want to diagnose and complain upon seeing
+ * an error from this call, but let's keep the same
+ * behaviour as before for now.
+ */
+ }
+ return 0;
+}
- errno = 0;
- fd = open(name, O_RDONLY | sha1_file_open_flag);
- if (fd >= 0)
- return fd;
+int git_open(const char *name)
+{
+ static int noatime = O_NOATIME;
+ int fd;
- /* Try again w/o O_CLOEXEC: the kernel might not support it */
- if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
- sha1_file_open_flag &= ~O_CLOEXEC;
- continue;
- }
+ errno = 0;
+ fd = open(name, O_RDONLY | noatime);
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
- sha1_file_open_flag &= ~O_NOATIME;
- continue;
- }
- return -1;
+ /* Might the failure be due to O_NOATIME? */
+ if ((noatime & O_NOATIME) && errno != ENOENT) {
+ noatime = 0;
+ fd = open(name, O_RDONLY);
}
+
+ if (fd < 0)
+ return fd;
+
+ /* do not let child processes to hold onto the open fd */
+ git_set_cloexec(fd);
+ return fd;
}
static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
^ permalink raw reply related
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Stefan Beller @ 2016-10-28 0:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqqk2ct4bmr.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 4:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> As for the rest of the series, it should be ready for review or comments.
>
> Just a few brief comments, before reading the patches carefully.
>
> * It is somewhat surprising that [1/5] is even needed (in other
> words, I would have expected something like this to be already
> there, and my knee-jerk reaction was "Heh, how does 'git status'
> know how to show submodules that are and are not initialized
> differently without this?"
The issue with much of the existing code is that it is submodule centric,
i.e. it is written to not care about the rest.
git status for example just calls "git submodule summary" to
parse and display the submodule information additionally.
It doesn't integrate submodules and treats them "just like files".
git submodule summary then proceeds to use "submodule--helper list"
that lists submodules *only* ignoring all files.
>
> The implementation that reads from the config of the current
> repository may be OK, but I actually would have expected that a
> check would be "given a $path, check to see if $path/.git is
> there and is a valid repository". In a repository where the
> submodules originate, there may not even be submodule.$name.url
> entries there yet.
My reaction to 1/5 was that the implementation is sound,
but the design may need rethinking.
Instead of asking all these question, "Is a submodule
* initialized
* checked out (== have a working dir)
* have a .git dir (think of deleted submodules that keep the
historical git dir around)
(* have commit X)
we would want to either extend the submodule-config API
to also carry these informations just like
name/path/sha1/url/shallow clone recommendation.
Obtaining the information above is however not as cheap,
because we'd need to do extra work additionally to parsing
the .gitmodules file. So the submodule-config would need to learn
an input that will tell the submodule-config what informations should
be evaluated and which can be omitted.
>
> * It is somewhat surprising that [4/5] does not even use the
> previous ls-files to find out the paths. Also it is a bit
> disappointing to see that the way processes are spawned and
> managed does not share much with Stefan's earlier work, i.e.
> run_processes_parallel(). I was somehow hoping that it can be
> extended to support this use case, but apparently there aren't
> much to be shared.
I think there are 2 issues here:
* The API I designed runs processes in parallel and the order or
output is non-deterministic. git-grep uses threads and output is
alphabetically sorted. The order is fixable though (by e.g. adding
a flag that indicates which parallel processing output the caller
wants).
* git-grep already has its own thread pool; integrating/combining
2 worker pools doesn't sound trivial even to someone who wrote
one of them.
Maybe we could extend/rewrite the run_processes_parallel
API to not just run processes, but instead you could also provide
a function pointer that is used in a thread instead.
Then we'd have one machinery that e.g. keeps track of the
number of parallel processes/threads.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Linus Torvalds @ 2016-10-27 23:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <xmqqfunh4b63.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Would the best endgame shape for this function be to open with
> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
> but ignoring an error from it, I guess? That would be the closest
> to what we historically had, I would think.
I think that's the best model.
Note that the O_NOATIME code is very much designed to try O_NOATIME
only _once_. Because even when the kernel supports O_NOATIME, if you
have a shared object tree where you may not be the owner of all the
files, a O_NOATIME open can fail with NOPERM ("You are not allowed to
hide your accesses to this file").
This is why it uses that
static unsigned int sha1_file_open_flag = O_NOATIME;
and if the O_NOATIME open ever fails (and the no-O_NOATIME open
succeeds), it clears that flag. Exactly so that it will *not* end up
in some kind of "let's open and fail and re-open" loop. It's designed
to fail once.
Or at least that's how it used to be originally. This code has
obviously changed since that early design. Now it seems to clear it
for any non-ENOENT error. Which looks fine too.
Linus
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-27 23:36 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFxdy4maom8byH0FoBBMWx+sQB8J7uWvHOxswjiaAhSjVg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> But the basic issue still remains - I'd really prefer to have NOATIME
> stay around for all those poor misguided souls that for some reason
> don't like "relatime" or run old kernels. But whether it is with
> O_NOATIME at open time or with F_SETFL, I don't care.
Understood.
Would the best endgame shape for this function be to open with
O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
but ignoring an error from it, I guess? That would be the closest
to what we historically had, I would think.
^ permalink raw reply
* Re: feature request
From: David Lang @ 2016-10-27 23:24 UTC (permalink / raw)
To: John Rood; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CALj-rGfAyimf0nFFcDHVHUgj8PQaz6Cvoz_PQfqdhr=QJEpbRw@mail.gmail.com>
On Thu, 27 Oct 2016, John Rood wrote:
> Thanks, I think changing the default for windows is a good idea.
notepad doesn't work well with unix line endings, wordpad handles the files much
more cleanly.
David Lang
^ permalink raw reply
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Junio C Hamano @ 2016-10-27 23:26 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20161027223834.35312-1-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> As for the rest of the series, it should be ready for review or comments.
Just a few brief comments, before reading the patches carefully.
* It is somewhat surprising that [1/5] is even needed (in other
words, I would have expected something like this to be already
there, and my knee-jerk reaction was "Heh, how does 'git status'
know how to show submodules that are and are not initialized
differently without this?"
The implementation that reads from the config of the current
repository may be OK, but I actually would have expected that a
check would be "given a $path, check to see if $path/.git is
there and is a valid repository". In a repository where the
submodules originate, there may not even be submodule.$name.url
entries there yet.
* It is somewhat surprising that [4/5] does not even use the
previous ls-files to find out the paths. Also it is a bit
disappointing to see that the way processes are spawned and
managed does not share much with Stefan's earlier work, i.e.
run_processes_parallel(). I was somehow hoping that it can be
extended to support this use case, but apparently there aren't
much to be shared.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Linus Torvalds @ 2016-10-27 23:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFxTHF4BRfcrCiV1D26-be+_rPhwAV+Vq8Roz-NMpPBadg@mail.gmail.com>
On Thu, Oct 27, 2016 at 4:09 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> That said, now that I think about it, I should double-check: maybe
> open() doesn't actually set atime at all, and we *could* do NOATIME
> with SETFL after all.
Checked. Yup. O_NOATIME could easily be done with SETFL:, because as
with O_CLOEXEC, it only affects operations _after_ the open. The open
itself doesn't set the access time.
So I was full of it.
But the basic issue still remains - I'd really prefer to have NOATIME
stay around for all those poor misguided souls that for some reason
don't like "relatime" or run old kernels. But whether it is with
O_NOATIME at open time or with F_SETFL, I don't care.
Linus
^ permalink raw reply
* Re: feature request
From: John Rood @ 2016-10-27 23:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqqshrh4d91.fsf@gitster.mtv.corp.google.com>
Thanks. I wasn't aware of --no-edit, but that is indeed exactly what I
was looking for.
I think your point about encouraging users to make good use of commit
messages is good.
My concern though is that vim isn't encouraging users to leave good
messages as much as it is scaring them away from leaving messages at
all.
On Thu, Oct 27, 2016 at 5:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Rood <mr.john.rood@gmail.com> writes:
>
> [administrivia: do not top post]
>
>> What I'm really seeking is not a make-shift solution for myself, but
>> an intuitive solution for the novice user-base at large.
>
> Well, there are -m and --no-edit. Recording commits with useless
> single liner is a bad habit to get into, and change to encourage
> novice user-base at large to do so is not a good idea.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Linus Torvalds @ 2016-10-27 23:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <xmqqoa254czs.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 3:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I also thought O_NOATIME shouldn't be an effective fcntl(2) thing,
> for the reasons you stated above, when I was studying the area
> because of the discussion on these patches. I was surprised to see
> that http://man7.org/linux/man-pages/man2/fcntl.2.html contradicts
> by saying F_SETFL can take O_NOATIME.
>
> Perhaps it deserves a bugreport?
It's not a bug per se.
You can indeed change O_NOATIME with F_SETFL. And it actually does
matter, since atime is normally changed by mmap(), read() and write()
calls.
So F_SETFL O_NOATIME actually does make sense, but it doesn't cover
the atime update done by open() itself.
That said, now that I think about it, I should double-check: maybe
open() doesn't actually set atime at all, and we *could* do NOATIME
with SETFL after all.
The exact [acm]time semantics are damn subtle.
Linus
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 23:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027210753.btc7zbndhdocsbwa@sigill.intra.peff.net>
On 28/10/16 10:07, Jeff King wrote:
> I think it does
> introduce compatibility complications.
If this is not a show stopper, I am happy to knock out a short
functional spec. I'll give it some hours before I begin.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-27 22:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFwfhFqV74s_O=GucycY9U19ysiACDqX=mK4Gf=eQ0coxQ@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> Note that you can *not* do the same thing with O_NOATIME, since the
> whole point of O_NOATIME is that it changes the behavior of the open
> itself (unlike O_CLOEXEC which changes _later_ behavior, and can
> always be replaced by FD_CLOEXEC fnclt modulo races that are
> immaterial for git)
A tangent.
I also thought O_NOATIME shouldn't be an effective fcntl(2) thing,
for the reasons you stated above, when I was studying the area
because of the discussion on these patches. I was surprised to see
that http://man7.org/linux/man-pages/man2/fcntl.2.html contradicts
by saying F_SETFL can take O_NOATIME.
Perhaps it deserves a bugreport?
^ permalink raw reply
* Re: feature request
From: Junio C Hamano @ 2016-10-27 22:51 UTC (permalink / raw)
To: John Rood; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CALj-rGdTfYYn1Nnksh=WijkJxknBUeqci3i3uujH8yzcE1z_8g@mail.gmail.com>
John Rood <mr.john.rood@gmail.com> writes:
[administrivia: do not top post]
> What I'm really seeking is not a make-shift solution for myself, but
> an intuitive solution for the novice user-base at large.
Well, there are -m and --no-edit. Recording commits with useless
single liner is a bad habit to get into, and change to encourage
novice user-base at large to do so is not a good idea.
^ permalink raw reply
* Re: feature request
From: John Rood @ 2016-10-27 22:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq1sz15swv.fsf@gitster.mtv.corp.google.com>
What I'm really seeking is not a make-shift solution for myself, but
an intuitive solution for the novice user-base at large.
On Thu, Oct 27, 2016 at 5:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Rood <mr.john.rood@gmail.com> writes:
>
>> I suppose I can do git config --global core.editor notepad
>> However, this really only addresses my second concern.
>>
>> My first concern is that using a text editor at all seems like
>> overkill in many scenarios.
>
> Nobody stops you from writing a "type whatever you want; I won't let
> you edit any mistakes as I am not even a text editor; just hit
> RETURN when you are done, as you can only write a single line"
> program and set it as your GIT_EDITOR.
>
> I do not know what would happen when you need "git commit --amend",
> though.
^ permalink raw reply
* Re: feature request
From: Junio C Hamano @ 2016-10-27 22:46 UTC (permalink / raw)
To: John Rood; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CALj-rGfAyimf0nFFcDHVHUgj8PQaz6Cvoz_PQfqdhr=QJEpbRw@mail.gmail.com>
John Rood <mr.john.rood@gmail.com> writes:
> On Thu, Oct 27, 2016 at 5:30 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
>>> Users should be able to configure Git to not send them into a Vim editor.
>>>
>>> When users pull commits, and a new commit needs to be created for a
>>> merge, Git's current way of determining a commit message is to send
>>> the user into a Vim window so that they can write a message. There are
>>> 2 reasons why this might not be the ideal way to prompt for a commit
>>> message.
>>>
>>> 1. Many users are used to writing concise one-line commit messages and
>>> would not expect to save a commit message in a multi-line file. Some
>>> users will wonder why they are in a text editor or which file they are
>>> editing. Others may not, in fact, realize at all that a text editor is
>>> what they are in.
>>
>> Look at the -m option of git commit,
>>
[administrivia: do not top post]
> Thanks, I think changing the default for windows is a good idea.
>
> The -m indeed accomplishes one-line messages when you are voluntarily
> doing a commit. However, the scenario I mentioned is "When users pull
> commits, and a new commit needs to be created for the merge" In this
> situation, the user isn't issuing the "git commit" command, and so
> he/she doesn't have the opportunity to use the -m flag.
There is --no-edit there.
^ permalink raw reply
* Re: feature request
From: John Rood @ 2016-10-27 22:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79ka7BvaW2pkjeXe68yKHFq6JsH__x2cXoe6U4tRmZ0rY1A@mail.gmail.com>
Thanks, I think changing the default for windows is a good idea.
The -m indeed accomplishes one-line messages when you are voluntarily
doing a commit. However, the scenario I mentioned is "When users pull
commits, and a new commit needs to be created for the merge" In this
situation, the user isn't issuing the "git commit" command, and so
he/she doesn't have the opportunity to use the -m flag.
On Thu, Oct 27, 2016 at 5:30 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
>> Users should be able to configure Git to not send them into a Vim editor.
>>
>> When users pull commits, and a new commit needs to be created for a
>> merge, Git's current way of determining a commit message is to send
>> the user into a Vim window so that they can write a message. There are
>> 2 reasons why this might not be the ideal way to prompt for a commit
>> message.
>>
>> 1. Many users are used to writing concise one-line commit messages and
>> would not expect to save a commit message in a multi-line file. Some
>> users will wonder why they are in a text editor or which file they are
>> editing. Others may not, in fact, realize at all that a text editor is
>> what they are in.
>
> Look at the -m option of git commit,
>
> git commit -a -m "look a commit with no editor, and a precise one line message"
>
> I do not advocate this use though, as I think commit messages should be
> more wordy.
>
>>
>> 2. Many users are not familiar with Vim, and do not understand how to
>> modify, save, and exit. It is not very considerate to require a user
>> to learn Vim in order to finish a commit that they are in the middle
>> of.
>
> That is true, but vi is like the most available editor as a relict
> from ancient times;
> as you are on Windows, maybe notepad is the best on that platform.
>
> Maybe file a bug/issue at https://github.com/git-for-windows to change
> the default?
>
>>
>> The existing behavior should be optional, and there should be two new options:
>>
>> 1. Use a simple inline prompt for a commit message (in the same way
>> Git might prompt for a username).
>>
>> 2. Automatically assign names for commits in the form of "Merged x into y".
^ permalink raw reply
* [PATCH 5/5] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <20161027223834.35312-1-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 | 67 +++++++++++++++++++++++++++++++++++---
t/t7814-grep-recurse-submodules.sh | 44 ++++++++++++++++++++++++-
tree-walk.c | 17 +++++-----
4 files changed, 125 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 17aa1ba70..386a868c6 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]
[ [--[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::
+ 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 f34f16df9..bdf1b9089 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);
@@ -535,6 +537,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
{
struct child_process cp = CHILD_PROCESS_INIT;
int status, i;
+ const char *end_of_base;
struct work_item *w = opt->output_priv;
prepare_submodule_repo_env(&cp.env_array);
@@ -545,9 +548,36 @@ static int grep_submodule_launch(struct grep_opt *opt,
gs->path);
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.
+ */
+ end_of_base = strchr(gs->name, ':');
+ if (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;
@@ -672,12 +702,21 @@ 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) {
+ name_base_len = strlen(super_prefix);
+ strbuf_addstr(&name, super_prefix);
+ }
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_setlen(&name, name_base_len);
+ strbuf_addstr(&name, base->buf + tn_len);
+ match = tree_entry_interesting(&entry, &name,
+ 0, pathspec);
if (match == all_entries_not_interesting)
break;
if (match == entry_not_interesting)
@@ -689,8 +728,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;
@@ -706,12 +744,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;
}
@@ -735,6 +779,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) {
@@ -761,6 +809,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)
@@ -901,6 +955,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")),
@@ -1153,7 +1210,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 b670c70cb..3d1892dd7 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -84,6 +84,49 @@ 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_incompatible_with_recurse_submodules ()
{
test_expect_success "--recurse-submodules and $1 are incompatible" "
@@ -94,6 +137,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 828f4356b..b3f996174 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -999,10 +999,11 @@ static enum interesting do_match(const struct name_entry *entry,
return entry_interesting;
/*
- * Match all directories. We'll try to
- * match files later on.
+ * Match all directories and gitlinks. We'll
+ * try to match files later on.
*/
- if (ps->recursive && S_ISDIR(entry->mode))
+ if (ps->recursive && (S_ISDIR(entry->mode) ||
+ S_ISGITLINK(entry->mode)))
return entry_interesting;
}
@@ -1043,13 +1044,13 @@ static enum interesting do_match(const struct name_entry *entry,
strbuf_setlen(base, base_offset + baselen);
/*
- * Match all directories. We'll try to match files
- * later on.
- * max_depth is ignored but we may consider support it
- * in future, see
+ * Match all directories and gitlinks. We'll try to match files
+ * later on. max_depth is ignored but we may consider support
+ * it in future, see
* http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840
*/
- if (ps->recursive && S_ISDIR(entry->mode))
+ if (ps->recursive && (S_ISDIR(entry->mode) ||
+ S_ISGITLINK(entry->mode)))
return entry_interesting;
}
return never_interesting; /* No matches */
--
2.10.1.613.g6021889
^ permalink raw reply related
* [PATCH 4/5] grep: optionally recurse into submodules
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <20161027223834.35312-1-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 | 301 ++++++++++++++++++++++++++++++++++---
git.c | 2 +-
t/t7814-grep-recurse-submodules.sh | 99 ++++++++++++
4 files changed, 386 insertions(+), 21 deletions(-)
create mode 100755 t/t7814-grep-recurse-submodules.sh
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 0ecea6e49..17aa1ba70 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 8887b6add..f34f16df9 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,259 @@ 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.
+ */
+ if (num_threads)
+ 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->path);
+ 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))
+ exit(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) &&
+ is_submodule_checked_out(path))) {
+ warning("skiping submodule '%s%s' since it is not initialized and checked out",
+ super_prefix ? super_prefix: "",
+ 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))
- 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);
+ 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
- hit |= grep_file(opt, ce->name);
+
if (ce_stage(ce)) {
do {
nr++;
@@ -413,6 +659,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 +899,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 +1005,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 +1123,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 +1153,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 efa1059fe..a156efd85 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 000000000..b670c70cb
--- /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.10.1.613.g6021889
^ permalink raw reply related
* [PATCH 2/5] submodules: load gitmodules file from commit sha1
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <20161027223834.35312-1-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 f7ee41456..74b0c3cba 100644
--- a/cache.h
+++ b/cache.h
@@ -1681,6 +1681,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 83fdecb1b..4d78e7227 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 098085be6..8b9a2ef28 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 d05c542d2..78584ba6a 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 029b24440..f2a56689f 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 bd039ca98..9a24ac82e 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_checked_out(const char *path);
int parse_submodule_update_strategy(const char *value,
--
2.10.1.613.g6021889
^ permalink raw reply related
* [PATCH 3/5] grep: add submodules as a grep source type
From: Brandon Williams @ 2016-10-27 22:38 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <20161027223834.35312-1-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 1194d35b5..0dbdc1d00 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 5856a23e4..267534ca2 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.10.1.613.g6021889
^ 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