* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Junio C Hamano @ 2016-11-18 23:31 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Jacob Keller
In-Reply-To: <CAOLa=ZQtmQWpFMPa-SD29N7hASHAPp8SGGJsLu+AW_Kv-1LqwA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Thanks, will add it in.
OK, here is a reroll of what I sent earlier in
http://public-inbox.org/git/<xmqq7f84tqa7.fsf_-_@gitster.mtv.corp.google.com>
but rebased so that it can happen as a preparatory bugfix before
your series.
The bug dates back to the very original implementation of %(HEAD) in
7a48b83219 ("for-each-ref: introduce %(HEAD) asterisk marker",
2013-11-18) and was moved to the current location in the v2.6 days
at c95b758587 ("ref-filter: move code from 'for-each-ref'",
2015-06-14).
-- >8 --
Subject: [PATCH] for-each-ref: do not segv with %(HEAD) on an unborn branch
The code to flip between "*" and " " prefixes depending on what
branch is checked out used in --format='%(HEAD)' did not consider
that HEAD may resolve to an unborn branch and dereferenced a NULL.
This will become a lot easier to trigger as the codepath will be
used to reimplement "git branch [--list]" in the future.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
ref-filter.c | 2 +-
t/t6300-for-each-ref.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/ref-filter.c b/ref-filter.c
index bc551a752c..d7e91a78da 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1017,7 +1017,7 @@ static void populate_value(struct ref_array_item *ref)
head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
sha1, NULL);
- if (!strcmp(ref->refname, head))
+ if (head && !strcmp(ref->refname, head))
v->s = "*";
else
v->s = " ";
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 19a2823025..039509a9cb 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -553,4 +553,14 @@ test_expect_success 'Verify sort with multiple keys' '
refs/tags/bogo refs/tags/master > actual &&
test_cmp expected actual
'
+
+test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
+ test_when_finished "git checkout master" &&
+ git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
+ sed -e "s/^\* / /" actual >expect &&
+ git checkout --orphan HEAD &&
+ git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0-rc2-152-gc9ad1dc38a
^ permalink raw reply related
* [PATCH 2/2] ref-filter: add support to display trailers as part of contents
From: Jacob Keller @ 2016-11-18 23:08 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
In-Reply-To: <20161118230825.20952-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Add %(trailers) and %(contents:trailers) to display the trailers as
interpreted by trailer_info_get. Update documentation and add a test for
the new feature.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-for-each-ref.txt | 2 ++
ref-filter.c | 22 +++++++++++++++++++++-
t/t6300-for-each-ref.sh | 26 ++++++++++++++++++++++++++
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f57e69bc83e3..e5807eede787 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -165,6 +165,8 @@ of all lines of the commit message up to the first blank line. The next
line is 'contents:body', where body is all of the lines after the first
blank line. The optional GPG signature is `contents:signature`. The
first `N` lines of the message is obtained using `contents:lines=N`.
+Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
+are obtained as 'contents:trailers'.
For sorting purposes, fields with numeric values sort in numeric order
(`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index d4c2931f3aab..b6f1bb73ed37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -13,6 +13,7 @@
#include "utf8.h"
#include "git-compat-util.h"
#include "version.h"
+#include "trailer.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -40,7 +41,7 @@ static struct used_atom {
enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
struct {
- enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
+ enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
unsigned int nlines;
} contents;
enum { O_FULL, O_SHORT } objectname;
@@ -85,6 +86,13 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
atom->u.contents.option = C_SUB;
}
+static void trailers_atom_parser(struct used_atom *atom, const char *arg)
+{
+ if (arg)
+ die(_("%%(trailers) does not take arguments"));
+ atom->u.contents.option = C_TRAILERS;
+}
+
static void contents_atom_parser(struct used_atom *atom, const char *arg)
{
if (!arg)
@@ -95,6 +103,8 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
+ else if (!strcmp(arg, "trailers"))
+ atom->u.contents.option = C_TRAILERS;
else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
@@ -194,6 +204,7 @@ static struct {
{ "creatordate", FIELD_TIME },
{ "subject", FIELD_STR, subject_atom_parser },
{ "body", FIELD_STR, body_atom_parser },
+ { "trailers", FIELD_STR, trailers_atom_parser },
{ "contents", FIELD_STR, contents_atom_parser },
{ "upstream", FIELD_STR, remote_ref_atom_parser },
{ "push", FIELD_STR, remote_ref_atom_parser },
@@ -785,6 +796,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
+ strcmp(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -808,6 +820,14 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
/* Size is the length of the message after removing the signature */
append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);
v->s = strbuf_detach(&s, NULL);
+ } else if (atom->u.contents.option == C_TRAILERS) {
+ struct trailer_info info;
+
+ /* Search for trailer info */
+ trailer_info_get(&info, subpos);
+ v->s = xmemdupz(info.trailer_start,
+ info.trailer_end - info.trailer_start);
+ trailer_info_release(&info);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 19a2823025e7..eb4bac0fe477 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -553,4 +553,30 @@ test_expect_success 'Verify sort with multiple keys' '
refs/tags/bogo refs/tags/master > actual &&
test_cmp expected actual
'
+
+cat >trailers <<EOF
+Reviewed-by: A U Thor <author@example.com>
+Signed-off-by: A U Thor <author@example.com>
+EOF
+
+test_expect_success 'basic atom: head contents:trailers' '
+ echo "Some contents" > two &&
+ git add two &&
+ git commit -F - <<-EOF &&
+ trailers: this commit message has trailers
+
+ Some message contents
+
+ $(cat trailers)
+ EOF
+ git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
+ sanitize_pgp <actual >actual.clean &&
+ # git for-each-ref ends with a blank line
+ cat >expect <<-EOF &&
+ $(cat trailers)
+
+ EOF
+ test_cmp expect actual.clean
+'
+
test_done
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* [PATCH 0/2] add format specifiers to display trailers
From: Jacob Keller @ 2016-11-18 23:08 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
This is based off of jt/use-trailer-api-in-commands so that we can make
use of the public trailer API that will parse a string for trailers.
I use trailers as a way to store extra commit metadata, and would like a
convenient way to obtain the trailers of a commit message easily. This
adds format specifiers to both the ref-filter API and the pretty
formats. I am not a fan of %bT but %t and %T were already taken. I don't
really know if it's ok to use %bT, since I think we used to allow "%bT"
format, though i don't think this is likely used much in practice.
I am open to suggestions for the pretty format specifier.
Additionally, I am somewhat not a fan of the way that if you have a
series of trailers which are trailer format, but not recognized, such
as the following:
<text>
My-tag: my value
My-other-tag: my other value
[non-trailer line]
My-tag: my third value
---
Git interpret-trailers will not recognize this as a trailer block
because it doesn't have any standard git tags within it. Would it be ok
to augment the trailer interpretation to say that if we have over 75%
trailers in the block that we accept it even if it doesn't have any real
recognized tags?
I say this because I regularly use extra tags in my git projects to
represent change metadata, and it would be nice if the tag block could
be recognized even if it has 1-2 lines of non-trailer formatting in
it...
Thoughts?
Jacob Keller (2):
pretty: add %bT format for displaying trailers of a commit message
ref-filter: add support to display trailers as part of contents
Documentation/git-for-each-ref.txt | 2 ++
Documentation/pretty-formats.txt | 1 +
pretty.c | 18 ++++++++++++++++++
ref-filter.c | 22 +++++++++++++++++++++-
t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 26 ++++++++++++++++++++++++++
6 files changed, 94 insertions(+), 1 deletion(-)
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply
* [PATCH 1/2] pretty: add %bT format for displaying trailers of a commit message
From: Jacob Keller @ 2016-11-18 23:08 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
In-Reply-To: <20161118230825.20952-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Recent patches have expanded on the trailers.c code and we have the
builtin commant git-interpret-trailers which can be used to add or
modify trailer lines. However, there is no easy way to simply display
the trailers of a commit message. Add support for %bT format modifier
which will use the trailer_info_get() calls to read trailers in an
identical way as git interpret-trailers does.
Add documentation and tests for the same.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/pretty-formats.txt | 1 +
pretty.c | 18 ++++++++++++++++++
t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
3 files changed, 45 insertions(+)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3bcee2ddb124..9ee68a4cb64a 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -138,6 +138,7 @@ The placeholders are:
- '%s': subject
- '%f': sanitized subject line, suitable for a filename
- '%b': body
+- '%bT': trailers of body as interpreted by linkgit:git-interpret-trailers[1]
- '%B': raw body (unwrapped subject and body)
ifndef::git-rev-list[]
- '%N': commit notes
diff --git a/pretty.c b/pretty.c
index 37b2c3b1f995..ea8764334865 100644
--- a/pretty.c
+++ b/pretty.c
@@ -10,6 +10,7 @@
#include "color.h"
#include "reflog-walk.h"
#include "gpg-interface.h"
+#include "trailer.h"
static char *user_format;
static struct cmt_fmt_map {
@@ -889,6 +890,16 @@ const char *format_subject(struct strbuf *sb, const char *msg,
return msg;
}
+static void format_trailers(struct strbuf *sb, const char *msg)
+{
+ struct trailer_info info;
+
+ trailer_info_get(&info, msg);
+ strbuf_add(sb, info.trailer_start,
+ info.trailer_end - info.trailer_start);
+ trailer_info_release(&info);
+}
+
static void parse_commit_message(struct format_commit_context *c)
{
const char *msg = c->message + c->message_off;
@@ -1289,6 +1300,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
format_sanitized_subject(sb, msg + c->subject_off);
return 1;
case 'b': /* body */
+ switch (placeholder[1]) {
+ case 'T':
+ format_trailers(sb, msg + c->subject_off);
+ return 2;
+ default:
+ break;
+ }
strbuf_addstr(sb, msg + c->body_off);
return 1;
}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index f5435fd250ba..7a35941ddcbd 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -535,4 +535,30 @@ test_expect_success 'clean log decoration' '
test_cmp expected actual1
'
+cat >trailers <<EOF
+Signed-off-by: A U Thor <author@example.com>
+Acked-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Signed-off-by: A U Thor <author@example.com>
+EOF
+
+test_expect_success 'pretty format %bT shows trailers' '
+ echo "Some contents" >trailerfile &&
+ git add trailerfile &&
+ git commit -F - <<-EOF &&
+ trailers: this commit message has trailers
+
+ This commit is a test commit with trailers at the end. We parse this
+ message and display the trailers using %bT
+
+ $(cat trailers)
+ EOF
+ git log --no-walk --pretty="%bT" >actual &&
+ cat >expect <<-EOF &&
+ $(cat trailers)
+
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* Re: [PATCH v4 4/6] grep: optionally recurse into submodules
From: Brandon Williams @ 2016-11-18 22:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqqlgwgl9ke.fsf@gitster.mtv.corp.google.com>
On 11/18, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > +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);
>
> We used to reject anything other than S_ISREG() upfront in the loop,
> and then either did grep_sha1() from the cache or from grep_file()
> from the working tree.
>
> Now, the guard upfront is removed, and we do the same in the first
> part of this if/elseif. The elseif part deals with a submodule that
> could match the pathspec.
>
> Don't we need a final else clause that would skip the remainder of
> this loop? What would happen to a S_ISREG() path that does *NOT*
> match the given pathspec? We used to just "continue", but it seems
> to me that such a path will fall through the above if/elseif in the
> new code. Would that be a problem?
It may be (Though I didn't see any issues when running tests). It would
be easy enough to add an 'else continue;' at the end though.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 3/6] grep: add submodules as a grep source type
From: Brandon Williams @ 2016-11-18 22:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqq60nkmpud.fsf@gitster.mtv.corp.google.com>
On 11/18, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > 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;
>
> Hmph, interesting. We have avoided ending enum definition with a
> comma, because it is only valid in more recent C than what we aim to
> support. This patch is not introducing a new problem, but just
> doing the same thing that would have broken older compilers as the
> existing code. Perhaps those older compilers have died out?
Perhaps it is time to move to a new C standard! :P
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-18 22:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqqh974l9bz.fsf@gitster.mtv.corp.google.com>
On 11/18, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > @@ -671,12 +707,29 @@ 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_setlen(&name, name_base_len);
> > + strbuf_addstr(&name, base->buf + tn_len);
> > +
> > + if (recurse_submodules && S_ISGITLINK(entry.mode)) {
> > + strbuf_addstr(&name, entry.path);
> > + match = submodule_path_match(pathspec, name.buf,
> > + NULL);
>
> The vocabulary from submodule_path_match() returns is the same as
> that of do_match_pathspec() and match_pathspec_item() which is
> MATCHED_{EXACTLY,FNMATCH,RECURSIVELY}, which is different from the
> vocabulary of the variable "match" which is "enum interesting" that
> is used by the tree-walk infrastructure.
>
> I doubt they are compatible to be usable like this. Am I missing
> something?
I think i initially must have thought it would work out, but looking
back at this I can clearly see that they aren't 100% compatible...
It slightly feels odd to me that we have so many different means for
checking pathspecs, all of which pretty much duplicate some of the
functionality of the other. Is there any reason there are these two
different code paths? Do we want them to remain separate or have them
be unified at some point?
Also, in order to use the tree_entry_interesting code it looks like I'll
either have to pipe through a flag saying 'yes i want to match against
submodules' like I did for the other pathspec codepath. Either that or
add functionality to perform wildmatching against partial matches (ie
directories and submodules) since currently the tree_entry_interesting
code path just punts and says 'well say it matches for now and check
again later' whenever it runs into a directory (I can't really make it
do that for submodules without a flag of somesort as tests could break).
Or maybe both?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output
From: Jakub Narębski @ 2016-11-18 22:46 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Jacob Keller
In-Reply-To: <20161108201211.25213-15-Karthik.188@gmail.com>
W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
> the atom %(upstream:track) can be translated if needed. This is needed
> as we port branch.c to use ref-filter's printing API's.
>
> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> ref-filter.c | 28 ++++++++++++++++++++++++----
> ref-filter.h | 2 ++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index b47b900..944671a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -15,6 +15,26 @@
> #include "version.h"
> #include "wt-status.h"
>
> +static struct ref_msg {
> + const char *gone;
> + const char *ahead;
> + const char *behind;
> + const char *ahead_behind;
> +} msgs = {
> + "gone",
> + "ahead %d",
> + "behind %d",
> + "ahead %d, behind %d"
> +};
> +
> +void setup_ref_filter_porcelain_msg(void)
> +{
> + msgs.gone = _("gone");
> + msgs.ahead = _("ahead %d");
> + msgs.behind = _("behind %d");
> + msgs.ahead_behind = _("ahead %d, behind %d");
> +}
Do I understand it correctly that this mechanism is here to avoid
repeated calls into gettext, as those messages would get repeated
over and over; otherwise one would use foo = N_("...") and _(foo),
isn't it?
I wonder if there is some way to avoid duplication here, but I don't
see anything easy and safe (e.g. against running setup_*() twice).
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v4 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-18 22:19 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479499135-64269-6-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> @@ -671,12 +707,29 @@ 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_setlen(&name, name_base_len);
> + strbuf_addstr(&name, base->buf + tn_len);
> +
> + if (recurse_submodules && S_ISGITLINK(entry.mode)) {
> + strbuf_addstr(&name, entry.path);
> + match = submodule_path_match(pathspec, name.buf,
> + NULL);
The vocabulary from submodule_path_match() returns is the same as
that of do_match_pathspec() and match_pathspec_item() which is
MATCHED_{EXACTLY,FNMATCH,RECURSIVELY}, which is different from the
vocabulary of the variable "match" which is "enum interesting" that
is used by the tree-walk infrastructure.
I doubt they are compatible to be usable like this. Am I missing
something?
> + } else {
> + match = tree_entry_interesting(&entry, &name,
> + 0, pathspec);
> + }
> +
> if (match == all_entries_not_interesting)
> break;
> if (match == entry_not_interesting)
^ permalink raw reply
* Re: [PATCH v4 4/6] grep: optionally recurse into submodules
From: Junio C Hamano @ 2016-11-18 22:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479499135-64269-5-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +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);
We used to reject anything other than S_ISREG() upfront in the loop,
and then either did grep_sha1() from the cache or from grep_file()
from the working tree.
Now, the guard upfront is removed, and we do the same in the first
part of this if/elseif. The elseif part deals with a submodule that
could match the pathspec.
Don't we need a final else clause that would skip the remainder of
this loop? What would happen to a S_ISREG() path that does *NOT*
match the given pathspec? We used to just "continue", but it seems
to me that such a path will fall through the above if/elseif in the
new code. Would that be a problem?
^ permalink raw reply
* Re: [PATCH v4 4/6] grep: optionally recurse into submodules
From: Junio C Hamano @ 2016-11-18 22:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479499135-64269-5-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> @@ -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);
> + }
The above two hunks both assume that the super_prefix option is
usable only from the top-level (i.e. opt->prefix_length == 0) and
also "--no-full-name" (which is the default) cannot be used. The
only invoker that runs "grep" with "--super-prefix" is the "grep"
that runs in the superproject, and it will only run us from the
top-level of the working tree, so the former assumption is OK.
It is a bit unclear to me how the "relative" and "--recurse-submodules"
would interact with each other, though.
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Jakub Narębski @ 2016-11-18 21:49 UTC (permalink / raw)
To: Junio C Hamano, Jacob Keller; +Cc: Karthik Nayak, Git mailing list
In-Reply-To: <xmqq4m38vdw4.fsf@gitster.mtv.corp.google.com>
W dniu 15.11.2016 o 18:42, Junio C Hamano pisze:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> dirname makes sense. What about implementing a reverse variant of
>> strip, which you could perform stripping of right-most components and
>> instead of stripping by a number, strip "to" a number, ie: keep the
>> left N most components, and then you could use something like
>> ...
>> I think that would be more general purpose than basename, and less confusing?
>
> I think you are going in the right direction. I had a similar
> thought but built around a different axis. I.e. if strip=1 strips
> one from the left, perhaps we want to have rstrip=1 that strips one
> from the right, and also strip=-1 to mean strip everything except
> one from the left and so on?. I think this and your keep (and
> perhaps you'll have rkeep for completeness) have the same expressive
> power. I do not offhand have a preference one over the other.
>
> Somehow it sounds a bit strange to me to treat 'remotes' as the same
> class of token as 'heads' and 'tags' (I'd expect 'heads' and
> 'remotes/origin' would be at the same level in end-user's mind), but
> that is probably an unrelated tangent. The reason this series wants
> to introduce :base must be to emulate an existing feature, so that
> existing feature is a concrete counter-example that argues against
> my "it sounds a bit strange" reaction.
If it is to implement the feature where we select if to display only
local branches (refs/heads/**), only remote-tracking branches
(refs/remotes/**/**), or only tags (refs/tags/**), then perhaps
':category' or ':type' would make sense?
As in '%(refname:category)', e.g.
%(if:equals=heads)%(refname:category)%(then)...%(end)
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v4 4/6] grep: optionally recurse into submodules
From: Junio C Hamano @ 2016-11-18 21:48 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479499135-64269-5-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +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");
> +...
> +
> + /* Add Pathspecs */
> + argv_array_push(&submodule_options, "--");
> + for (i = 0; i < pathspec->nr; i++)
> + argv_array_push(&submodule_options,
> + pathspec->items[i].original);
> +}
When I do
$ git grep --recurse-submodules pattern submodules/ lib/
where I have bunch of submodules in "submodules/" directory in the
top-level project, the top-level grep would try to find the pattern
in its own files in its "lib/" directory and then invoke sub-greps
in the submodule/a, submodule/b, etc. working trees.
This passes the "submodules/" and "lib/" pathspec down to these
sub-greps. These sub-greps in turn learn via --super-prefix where
they are in the super-project's context (e.g. "submodules/a/") to
adjust the given pathspec patterns, so everything cancels out
(e.g. they know "lib/" is totally outside of their area and their
files do not match with the pathspec element "lib/" at all).
Looking good.
^ permalink raw reply
* Re: [PATCH v4 3/6] grep: add submodules as a grep source type
From: Junio C Hamano @ 2016-11-18 21:37 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479499135-64269-4-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> 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;
Hmph, interesting. We have avoided ending enum definition with a
comma, because it is only valid in more recent C than what we aim to
support. This patch is not introducing a new problem, but just
doing the same thing that would have broken older compilers as the
existing code. Perhaps those older compilers have died out?
^ permalink raw reply
* Re: [PATCH v7 10/17] ref-filter: introduce refname_atom_parser_internal()
From: Jakub Narębski @ 2016-11-18 21:36 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Jacob Keller
In-Reply-To: <20161108201211.25213-11-Karthik.188@gmail.com>
W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Since there are multiple atoms which print refs ('%(refname)',
> '%(symref)', '%(push)', '%upstream'), it makes sense to have a common
Minor typo; it should be: "%(upstream)"
> ground for parsing them. This would allow us to share implementations of
> the atom modifiers between these atoms.
>
> Introduce refname_atom_parser_internal() to act as a common parsing
> function for ref printing atoms. This would eventually be used to
> introduce refname_atom_parser() and symref_atom_parser() and also be
> internally used in remote_ref_atom_parser().
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
[...]
> +static void refname_atom_parser_internal(struct refname_atom *atom,
> + const char *arg, const char *name)
> +{
> + if (!arg)
> + atom->option = R_NORMAL;
> + else if (!strcmp(arg, "short"))
> + atom->option = R_SHORT;
> + else if (skip_prefix(arg, "strip=", &arg)) {
> + atom->option = R_STRIP;
> + if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
> + die(_("positive value expected refname:strip=%s"), arg);
> + } else
^^^^^^
It looks like you have spurious tab here.
> + die(_("unrecognized %%(%s) argument: %s"), name, arg);
> +}
> +
> static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> {
> struct string_list params = STRING_LIST_INIT_DUP;
>
^ permalink raw reply
* Re: [PATCH v7 09/17] ref-filter: make "%(symref)" atom work with the ':short' modifier
From: Jakub Narębski @ 2016-11-18 21:34 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Jacob Keller
In-Reply-To: <20161108201211.25213-10-Karthik.188@gmail.com>
W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
[...]
> Add tests for %(symref) and %(symref:short) while we're here.
That's nice.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
[...]
> +test_expect_success 'Add symbolic ref for the following tests' '
> + git symbolic-ref refs/heads/sym refs/heads/master
> +'
> +
> +cat >expected <<EOF
> +refs/heads/master
> +EOF
This should be inside the relevant test, not outside. In other
patches in this series you are putting setup together with the
rest of test, by using "cat >expected <<-\EOF".
> +
> +test_expect_success 'Verify usage of %(symref) atom' '
> + git for-each-ref --format="%(symref)" refs/heads/sym > actual &&
This should be spelled " >actual", rather than " > actual"; there
should be no space between redirection and file name.
> + test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +heads/master
> +EOF
> +
> +test_expect_success 'Verify usage of %(symref:short) atom' '
> + git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
> + test_cmp expected actual
> +'
Same here.
> +
> test_done
>
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v3] remote-curl: don't hang when a server dies before any output
From: Junio C Hamano @ 2016-11-18 21:05 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce, peff
In-Reply-To: <1479501049-15458-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> In the event that a HTTP server closes the connection after giving a
> 200 but before giving any packets, we don't want to hang forever
> waiting for a response that will never come. Instead, we should die
> immediately.
>
> One case where this happens is when attempting to fetch a dangling
> object by SHA. In this case, the server dies before sending any data.
> Prior to this patch, fetch-pack would wait for data from the server,
> and remote-curl would wait for fetch-pack, causing a deadlock.
> ...
> Still to do: it would be good to give a better error message
> than "fatal: The remote end hung up unexpectedly".
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Thanks.
^ permalink raw reply
* [PATCH v3] remote-curl: don't hang when a server dies before any output
From: David Turner @ 2016-11-18 20:30 UTC (permalink / raw)
To: git, spearce, peff; +Cc: David Turner, Junio C Hamano
In the event that a HTTP server closes the connection after giving a
200 but before giving any packets, we don't want to hang forever
waiting for a response that will never come. Instead, we should die
immediately.
One case where this happens is when attempting to fetch a dangling
object by SHA. In this case, the server dies before sending any data.
Prior to this patch, fetch-pack would wait for data from the server,
and remote-curl would wait for fetch-pack, causing a deadlock.
Despite this patch, there is other possible malformed input that could
cause the same deadlock (e.g. a half-finished pktline, or a pktline but
no trailing flush). There are a few possible solutions to this:
1. Allowing remote-curl to tell fetch-pack about the EOF (so that
fetch-pack could know that no more data is coming until it says
something else). This is tricky because an out-of-band signal would
be required, or the http response would have to be re-framed inside
another layer of pkt-line or something.
2. Make remote-curl understand some of the protocol. It turns out
that in addition to understanding pkt-line, it would need to watch for
ack/nak. This is somewhat fragile, as information about the protocol
would end up in two places. Also, pkt-lines which are already at the
length limit would need special handling.
Both of these solutions would require a fair amount of work, whereas
this hack is easy and solves at least some of the problem.
Still to do: it would be good to give a better error message
than "fatal: The remote end hung up unexpectedly".
Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
remote-curl.c | 8 ++++++++
t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f..ee44236 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -400,6 +400,7 @@ struct rpc_state {
size_t pos;
int in;
int out;
+ int any_written;
struct strbuf result;
unsigned gzip_request : 1;
unsigned initial_buffer : 1;
@@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
{
size_t size = eltsize * nmemb;
struct rpc_state *rpc = buffer_;
+ if (size)
+ rpc->any_written = 1;
write_or_die(rpc->in, ptr, size);
return size;
}
@@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+
+ rpc->any_written = 0;
err = run_slot(slot, NULL);
if (err == HTTP_REAUTH && !large_request) {
credential_fill(&http_auth);
@@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
if (err != HTTP_OK)
err = -1;
+ if (!rpc->any_written)
+ err = -1;
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b27..43665ab 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
test_line_count = 2 posts
'
+test_expect_success 'test allowreachablesha1inwant' '
+ test_when_finished "rm -rf test_reachable.git" &&
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ git -C test_reachable.git fetch origin "$master_sha"
+'
+
+test_expect_success 'test allowreachablesha1inwant with unreachable' '
+ test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
+
+ #create unreachable sha
+ echo content >file2 &&
+ git add file2 &&
+ git commit -m two &&
+ git push public HEAD:refs/heads/doomed &&
+ git push public :refs/heads/doomed &&
+
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+'
+
test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
(
cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: [PATCH v4 0/6] recursively grep across submodules
From: Stefan Beller @ 2016-11-18 20:10 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jonathan Tan, Junio C Hamano
In-Reply-To: <1479499135-64269-1-git-send-email-bmwill@google.com>
On Fri, Nov 18, 2016 at 11:58 AM, Brandon Williams <bmwill@google.com> wrote:
> This revision of this series should address all of the problems brought up with
> v3.
>
> * indent output example in patch 5/6.
> * fix ':' in submodule names and add a test to verify.
> * cleanup some comments.
> * fixed tests to test the case where a submodule isn't at the root of a
> repository.
> * always pass --threads=%d in order to limit threads to child proccess.
>
>
> -- interdiff based on 'bw/grep-recurse-submodules'
Thanks for interdiff!
I only skimmed the patches, but rather reviewed this interdiff in detail.
The series looks good to me, no nits!
Thanks,
Stefan
^ permalink raw reply
* Re: gitweb html validation
From: Ralf Thielow @ 2016-11-18 20:06 UTC (permalink / raw)
To: Raphaël Gertz, Junio C Hamano, jnareb; +Cc: git
In-Reply-To: <20161115182632.GA17539@gmail.com>
2016-11-15 19:26 GMT+01:00 Ralf Thielow <ralf.thielow@gmail.com>:
Finally I've found the time to actually try this out and there
are some problems with it.
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7cf68f07b..33d7c154f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5531,8 +5531,8 @@ sub git_project_search_form {
> $limit = " in '$project_filter/'";
> }
>
> - print "<div class=\"projsearch\">\n";
> print $cgi->start_form(-method => 'get', -action => $my_uri) .
> + "<div class=\"projsearch\">\n" .
> $cgi->hidden(-name => 'a', -value => 'project_list') . "\n";
> print $cgi->hidden(-name => 'pf', -value => $project_filter). "\n"
> if (defined $project_filter);
> @@ -5544,11 +5544,11 @@ sub git_project_search_form {
> -checked => $search_use_regexp) .
> "</span>\n" .
> $cgi->submit(-name => 'btnS', -value => 'Search') .
> - $cgi->end_form() . "\n" .
> $cgi->a({-href => href(project => undef, searchtext => undef,
> project_filter => $project_filter)},
> esc_html("List all projects$limit")) . "<br />\n";
> - print "</div>\n";
> + print "</div>\n" .
> + $cgi->end_form() . "\n";
> }
>
The anchor is now inside the form-tag, which means there is no
visual line-break anymore that comes automatically after </form>
as the form-tag is a block level element. Could be solved by adding a
"<br />", which is not very nice, but OK.
> # entry for given @keys needs filling if at least one of keys in list
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 321260103..507740b6a 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -539,7 +539,7 @@ div.projsearch {
> margin: 20px 0px;
> }
>
> -div.projsearch form {
> +form div.projsearch {
> margin-bottom: 2px;
> }
>
This is wrong as it overwrites the setting above, 20px at the bottom
went to 2px.
The problem is how to apply the 2px now. Before this, we had the
<form>, a block element, which we can give the 2px margin at the
bottom. Now this element is gone and we have a set of inline
elements where we use "<br />" to emulate the line break. There
are two css rules which can solve this, but I'm not really happy with
both of them.
1) As we know we need the two pixel below an input element, we
can say
div.projsearch input {
margin-bottom: 2px;
}
2) Make the a-Tag inside div.projsearch being displayed as a block
element to make a margin-top setting working. This has the benefit
that we don't care about the element above.
div.projsearch a {
display: inline-block;
margin-top: 2px;
}
If I have to choose I'd prefer the second one.
So I can't think of a way to solve this nicely with this change.
^ permalink raw reply
* Re: [PATCH v2] remote-curl: don't hang when a server dies before any output
From: Junio C Hamano @ 2016-11-18 20:04 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, spearce
In-Reply-To: <1479491919-12592-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> In the event that a HTTP server closes the connection after giving a
> 200 but before giving any packets, we don't want to hang forever
> waiting for a response that will never come. Instead, we should die
> immediately.
>
> One case where this happens is when attempting to fetch a dangling
> object by SHA.
>
> Prior to this patch, fetch-pack would wait for more data from the
> server, and remote-curl would wait for fetch-pack, causing a deadlock.
>
> Despite this patch, there is other possible malformed input that could
> cause the same deadlock (e.g. a half-finished pktline, or a pktline but
> no trailing flush). There are a few possible solutions to this:
>
> 1. Allowing remote-curl to tell fetch-pack about the EOF (so that
> fetch-pack could know that no more data is coming until it says
> something else). This is tricky because an out-of-band signal would
> be required, or the http response would have to be re-framed inside
> another layer of pkt-line or something.
>
> 2. Make remote-curl understand some of the protocol. It turns out
> that in addition to understanding pkt-line, it would need to watch for
> ack/nak. This is somewhat fragile, as information about the protocol
> would end up in two places. Also, pkt-lines which are already at the
> length limit would need special handling.
>
> Both of these solutions would require a fair amount of work, whereas
> this hack is easy and solves at least some of the problem.
>
> Still to do: it would be good to give a better error message
> than "fatal: The remote end hung up unexpectedly".
It seems to me that there is some gap/leap in the description
between the second and third paragraph above (i.e. the client asks
for an object, the server tries to see if the object is reachable
from the refs, and then what? who waits for more data without
asking?), but other than that, including the future direction, the
proposed log message is very nicely described.
Thanks, will queue.
^ permalink raw reply
* [PATCH v4 4/6] grep: optionally recurse into submodules
From: Brandon Williams @ 2016-11-18 19:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, jonathantanmy, gitster
In-Reply-To: <1479499135-64269-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, 385 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 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..cfafa15 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,258 @@ 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))
- 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 +658,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 +898,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 +1004,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 +1122,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 +1152,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 v4 6/6] grep: search history of moved submodules
From: Brandon Williams @ 2016-11-18 19:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, jonathantanmy, gitster
In-Reply-To: <1479499135-64269-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 9b795ee..747b0c3 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 d1fd7ed..7d66716 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -158,6 +158,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 v4 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-18 19:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, jonathantanmy, gitster
In-Reply-To: <1479499135-64269-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 | 83 +++++++++++++++++++++++++++++++++++---
t/t7814-grep-recurse-submodules.sh | 75 +++++++++++++++++++++++++++++++++-
3 files changed, 162 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 cfafa15..9b795ee 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;
@@ -671,12 +707,29 @@ 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_setlen(&name, name_base_len);
+ strbuf_addstr(&name, base->buf + tn_len);
+
+ if (recurse_submodules && S_ISGITLINK(entry.mode)) {
+ strbuf_addstr(&name, entry.path);
+ match = submodule_path_match(pathspec, name.buf,
+ NULL);
+ } else {
+ match = tree_entry_interesting(&entry, &name,
+ 0, pathspec);
+ }
+
if (match == all_entries_not_interesting)
break;
if (match == entry_not_interesting)
@@ -688,8 +741,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;
@@ -705,12 +757,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;
}
@@ -734,6 +792,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) {
@@ -760,6 +822,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)
@@ -900,6 +968,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")),
@@ -1152,7 +1223,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..d1fd7ed 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -84,6 +84,80 @@ 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 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 +168,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
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 2/6] submodules: load gitmodules file from commit sha1
From: Brandon Williams @ 2016-11-18 19:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, jonathantanmy, gitster
In-Reply-To: <1479499135-64269-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 f5107f0..062e58b 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
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