* Re: [PATCH 08/16] update submodules: add depopulate_submodule
From: Stefan Beller @ 2016-11-18 0:16 UTC (permalink / raw)
To: Brandon Williams
Cc: git@vger.kernel.org, Junio C Hamano, Jonathan Nieder, Martin Fick,
David Turner
In-Reply-To: <CAGZ79ka0-JFvogHRoTA4ioMK86zD=zkgEfBb-gpU8tbOjwEoFA@mail.gmail.com>
On Thu, Nov 17, 2016 at 2:42 PM, Stefan Beller <sbeller@google.com> wrote:
>
> I think I'll just write this functionality in C and optionally expose
> it via the submodule--helper,
> such that the user facing git-submodule.sh only has to call that helper.
I think it will roughly look like this:
(white space mangled)
commit e72ef244c667920c874247aa32aa55845500aac8
Author: Stefan Beller <sbeller@google.com>
Date: Thu Nov 17 16:14:46 2016 -0800
submodule--helper: add intern-git-dir function
When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.
Add functionality to migrate the git directory to be embedded
into the superprojects git directory.
Signed-off-by: Stefan Beller <sbeller@google.com>
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..4f31100 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,21 @@ static int resolve_remote_submodule_branch(int
argc, const char **argv,
return 0;
}
+static int intern_git_dir(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ struct pathspec pathspec;
+ struct module_list list = MODULE_LIST_INIT;
+
+ if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+ return 1;
+
+ for (i = 0; i < list.nr; i++)
+ migrate_submodule_gitdir(list.entries[i]->name);
+
+ return 0;
+}
+
struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
@@ -1090,7 +1105,8 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url", resolve_relative_url},
{"resolve-relative-url-test", resolve_relative_url_test},
{"init", module_init},
- {"remote-branch", resolve_remote_submodule_branch}
+ {"remote-branch", resolve_remote_submodule_branch},
+ {"intern-git-dir", intern_git_dir}
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/submodule.c b/submodule.c
index 45b9060..e513bba 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1335,3 +1335,42 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
}
+
+/*
+ * Migrate the given submodule (and all its submodules recursively) from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void migrate_submodule_gitdir(const char *path)
+{
+ char *old_git_dir;
+ const char *new_git_dir;
+ const struct submodule *sub;
+
+ struct child_process cp = CHILD_PROCESS_INIT;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path;
+ argv_array_pushl(&cp.args, "submodule", "foreach", "--recursive",
+ "git", "submodule--helper" "intern-git-dir", NULL);
+
+ if (run_command(&cp))
+ die(_("Could not migrate git directory in submodule '%s'"),
+ path);
+
+ old_git_dir = xstrfmt("%s/.git", path);
+ if (read_gitfile(old_git_dir))
+ /* If it is an actual gitfile, it doesn't need migration. */
+ goto out;
+
+ sub = submodule_from_path(null_sha1, path);
+ new_git_dir = git_common_path("modules/%s", sub->name);
+
+ if (rename(old_git_dir, new_git_dir) < 0)
+ die_errno(_("Could not migrate git directory from %s to %s"),
+ old_git_dir, new_git_dir);
+
+ connect_work_tree_and_git_dir(path, new_git_dir);
+out:
+ free(old_git_dir);
+}
diff --git a/submodule.h b/submodule.h
index aac202c..143ec18 100644
--- a/submodule.h
+++ b/submodule.h
@@ -90,5 +90,6 @@ extern int parallel_submodules(void);
* retaining any config in the environment.
*/
extern void prepare_submodule_repo_env(struct argv_array *out);
+extern void migrate_submodule_gitdir(const char *path);
#endif
^ permalink raw reply related
* Re: [PATCH 09/16] update submodules: add scheduling to update submodules
From: Stefan Beller @ 2016-11-18 0:28 UTC (permalink / raw)
To: Brandon Williams
Cc: git@vger.kernel.org, Junio C Hamano, Jonathan Nieder, Martin Fick,
David Turner
In-Reply-To: <20161116000236.GF66382@google.com>
On Tue, Nov 15, 2016 at 4:02 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/15, Stefan Beller wrote:
>> +
>> + child_process_clear(&cp);
>> + return 0;
>> +}
>
> If run command is successful then it handles the clearing of the child
> process struct, correct? Is there a negative to having all the explicit
> clears when the child was successful?
void child_process_clear(struct child_process *child)
{
argv_array_clear(&child->args);
argv_array_clear(&child->env_array);
}
I don't think so, as clearing empty arg arrays is a no op.
>> +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}
>
> I may not know enough about these types of initializors but that Init
> macro only has 2 entries while there are three entries in the struct
> itself.
Filled up to 3 to be explicit.
>
>> +
>> +int scheduled_submodules_nr, scheduled_submodules_alloc;
>
> Should these globals be static since they should be scoped to only this
> file?
Of course, done.
>
> nit: organization wise it makes more sense to me to have the
> 'update_submodule' helper function be located more closely to the
> 'update_submodules' function.
>
done
David wrote:
> In fact, only the first NULL is necessary; unspecified initializer entries in C default to zero.
as said above, I explicitly init all of them now.
^ permalink raw reply
* Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick
From: Junio C Hamano @ 2016-11-18 4:47 UTC (permalink / raw)
To: David Turner; +Cc: git, sbeller
In-Reply-To: <1478543491-6286-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> index 470f334..be074a1 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -575,13 +575,13 @@ test_expect_success 'merge removes empty directories' '
> test_must_fail test -d d
> '
>
> -test_expect_failure 'merge-recursive simple w/submodule' '
> +test_expect_success 'merge-recursive simple w/submodule' '
>
> git checkout submod &&
> git merge remove
> '
>
> -test_expect_failure 'merge-recursive simple w/submodule result' '
> +test_expect_sucess 'merge-recursive simple w/submodule result' '
Here is a typo. I wonder if we want to do "set -e" at the end of
test-lib.sh to catch a breakage like this. I only caught it by
being lucky (I was staring "make test" output as it flew by).
I've already amended the copy I have, but in case you are going to
reroll in the future, please do not forget to update your copy.
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-18 7:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqqmvgynee4.fsf@gitster.mtv.corp.google.com>
Hey,
On Fri, Nov 18, 2016 at 12:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jacob Keller <jacob.keller@gmail.com> writes:
>>> ...
>>> 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?
>> ...
>
>> If we do implement strip with negative numbers, it definitely
>> would be neat, but to get the desired feature which I've mentioned
>> below, we'd need to call strip twice, i.e
>> to get remotes from /refs/foo/abc/xyz we'd need to do
>> strip=1,strip=-1, which could be
>> done but ...
>
> ... would be unnecessary if this is the only use case:
>
>> strbuf_addf(&fmt,
>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>> local.buf, remote.buf);
>
> You can "strip to leave only 2 components" and compare the result
> with refs/remotes instead, no?
>
Of course, my only objective was that someone would find it useful to
have these two additional
atoms. So if you think it's unnecessary we could drop it entirely :D
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Jacob Keller @ 2016-11-18 8:19 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, Git mailing list
In-Reply-To: <CAOLa=ZTVTZ+1dXpcp=kdoGbT1Feq=vOfFpNpBiZepajMucraPQ@mail.gmail.com>
On Thu, Nov 17, 2016 at 11:33 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Hey,
>
> On Fri, Nov 18, 2016 at 12:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Jacob Keller <jacob.keller@gmail.com> writes:
>>>> ...
>>>> 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?
>>> ...
>>
>>> If we do implement strip with negative numbers, it definitely
>>> would be neat, but to get the desired feature which I've mentioned
>>> below, we'd need to call strip twice, i.e
>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>> strip=1,strip=-1, which could be
>>> done but ...
>>
>> ... would be unnecessary if this is the only use case:
>>
>>> strbuf_addf(&fmt,
>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>> local.buf, remote.buf);
>>
>> You can "strip to leave only 2 components" and compare the result
>> with refs/remotes instead, no?
>>
>
> Of course, my only objective was that someone would find it useful to
> have these two additional
> atoms. So if you think it's unnecessary we could drop it entirely :D
>
> --
> Regards,
> Karthik Nayak
I think having strip and rstrip make sense, (along with support for
negative numbers) I don't think we need to make them work together
unless someone is interested, since we can use strip=-2 to get the
behavior we need today.
Thanks,
Jake
^ permalink raw reply
* Staging chunks can get confused
From: Daurnimator @ 2016-11-18 11:29 UTC (permalink / raw)
To: Git Mailing List
Tonight I was staging changes with `git add -p` and noticed they got
applied at the *wrong* location.
My guess is that when you stage a hunk it doesn't do a line number
fix-up for earlier unstaged hunks in the file.
Screencast: https://asciinema.org/a/7y9qhr0837a7t96m8w14mupnk
Alternatively, an example follows:
$ git add -p
diff --git a/unistring/unistring.c b/unistring/unistring.c
index 62bbc8a..45ced5d 100644
--- a/unistring/unistring.c
+++ b/unistring/unistring.c
@@ -17,6 +17,27 @@ static const char *const uninormnames[] = {"NFD",
"NFC", "NFKD", "NFKC", NULL};
#define lunistring_optuninorm(L, arg)
(lua_isnoneornil(L,(arg))?NULL:lunistring_checkuninorm((L), (arg)))
+const uint8_t * u8_check (const uint8_t *s, size_t n);
+int u8_mblen (const uint8_t *s, size_t n);
+int u8_mbtouc_unsafe (ucs4_t *puc, const uint8_t *s, size_t n);
+int u8_mbtouc (ucs4_t *puc, const uint8_t *s, size_t n);
+int u8_mbtoucr (ucs4_t *puc, const uint8_t *s, size_t n);
+int u8_uctomb (uint8_t *s, ucs4_t uc, int n);
+size_t u8_mbsnlen (const uint8_t *s, size_t n);
+
+int u8_strmblen (const uint8_t *s);
+int u8_strmbtouc (ucs4_t *puc, const uint8_t *s);
+const uint8_t * u8_next (ucs4_t *puc, const uint8_t *s);
+const uint8_t * u8_prev (ucs4_t *puc, const uint8_t *s, const uint8_t *start);
+
+uint8_t * u8_conv_from_encoding (const char *fromcode, enum
iconv_ilseq_handler handler, const char *src, size_t srclen, size_t
*offsets, uint8_t *resultbuf, size_t *lengthp);
+char * u8_conv_to_encoding (const char *tocode, enum
iconv_ilseq_handler handler, const uint8_t *src, size_t srclen, size_t
*offsets, char *resultbuf, size_t *lengthp);
+uint8_t * u8_strconv_from_encoding (const char *string, const char
*fromcode, enum iconv_ilseq_handler handler);
+char * u8_strconv_to_encoding (const uint8_t *string, const char
*tocode, enum iconv_ilseq_handler handler);
+uint8_t * u8_strconv_from_locale (const char *string);
+char * u8_strconv_to_locale (const uint8_t *string);
+
+
static int lunistring_width(lua_State *L) {
size_t n;
const uint8_t *s = (const uint8_t*)luaL_checklstring(L, 1, &n);
Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? n
@@ -27,6 +48,17 @@ static int lunistring_width(lua_State *L) {
}
+int u8_strwidth (const uint8_t *s, const char *encoding);
+void u8_wordbreaks (const uint8_t *s, size_t n, char *p);
+void u8_possible_linebreaks (const uint8_t *s, size_t n, const char
*encoding, char *p);
+int u8_width_linebreaks (const uint8_t *s, size_t n, int width, int
start_column, int at_end_columns, const char *override, const char
*encoding, char *p);
+
+
+int uc_decomposition (ucs4_t uc, int *decomp_tag, ucs4_t *decomposition);
+int uc_canonical_decomposition (ucs4_t uc, ucs4_t *decomposition);
+ucs4_t uc_composition (ucs4_t uc1, ucs4_t uc2);
+
+
static int lunistring_normalize(lua_State *L) {
uninorm_t nf = lunistring_optuninorm(L, 1);
size_t n;
Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n
@@ -54,6 +86,9 @@ static int lunistring_normalize(lua_State *L) {
}
+int u8_normcmp (const uint8_t *s1, size_t n1, const uint8_t *s2,
size_t n2, uninorm_t nf, int *resultp);
+
+
static int lunistring_normxfrm(lua_State *L) {
size_t n;
const uint8_t *s = (const uint8_t*)luaL_checklstring(L, 1, &n);
Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n
@@ -83,6 +118,9 @@ static int lunistring_normxfrm(lua_State *L) {
}
+int u8_normcoll (const uint8_t *s1, size_t n1, const uint8_t *s2,
size_t n2, uninorm_t nf, int *resultp);
+
+
static int lunistring_uc_locale_language(lua_State *L) {
lua_pushstring(L, uc_locale_language());
return 1;
Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n
@@ -173,6 +211,15 @@ static int lunistring_totitle(lua_State *L) {
}
+casing_prefix_context_t u8_casing_prefix_context (const uint8_t *s, size_t n);
+casing_prefix_context_t u8_casing_prefixes_context (const uint8_t *s,
size_t n, casing_prefix_context_t a_context);
+casing_suffix_context_t u8_casing_suffix_context (const uint8_t *s, size_t n);
+casing_suffix_context_t u8_casing_suffixes_context (const uint8_t *s,
size_t n, casing_suffix_context_t a_context);
+uint8_t * u8_ct_toupper (const uint8_t *s, size_t n,
casing_prefix_context_t prefix_context, casing_suffix_context_t
suffix_context, const char *iso639_language, uninorm_t nf, uint8_t
*resultbuf, size_t *lengthp);
+uint8_t * u8_ct_tolower (const uint8_t *s, size_t n,
casing_prefix_context_t prefix_context, casing_suffix_context_t
suffix_context, const char *iso639_language, uninorm_t nf, uint8_t
*resultbuf, size_t *lengthp);
+uint8_t * u8_ct_totitle (const uint8_t *s, size_t n,
casing_prefix_context_t prefix_context, casing_suffix_context_t
suffix_context, const char *iso639_language, uninorm_t nf, uint8_t
*resultbuf, size_t *lengthp);
+
+
static int lunistring_casefold(lua_State *L) {
size_t n;
const uint8_t *s = (const uint8_t*)luaL_checklstring(L, 1, &n);
Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n
@@ -201,6 +248,10 @@ static int lunistring_casefold(lua_State *L) {
}
+uint8_t * u8_ct_casefold (const uint8_t *s, size_t n,
casing_prefix_context_t prefix_context, casing_suffix_context_t
suffix_context, const char *iso639_language, uninorm_t nf, uint8_t
*resultbuf, size_t *lengthp);
+int u8_casecmp (const uint8_t *s1, size_t n1, const uint8_t *s2,
size_t n2, const char *iso639_language, uninorm_t nf, int *resultp);
+
+
static int lunistring_casexfrm(lua_State *L) {
size_t n;
const uint8_t *s = (const uint8_t*)luaL_checklstring(L, 1, &n);
Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n
@@ -230,6 +281,9 @@ static int lunistring_casexfrm(lua_State *L) {
}
+int u8_casecoll (const uint8_t *s1, size_t n1, const uint8_t *s2,
size_t n2, const char *iso639_language, uninorm_t nf, int *resultp);
+
+
static int lunistring_casecoll(lua_State *L) {
size_t n1, n2;
const uint8_t *s1 = (const uint8_t*)luaL_checklstring(L, 1, &n1);
Stage this hunk [y,n,q,a,d,/,K,j,J,g,e,?]? n
@@ -288,7 +342,7 @@ static int lunistring_is_titlecase(lua_State *L) {
return luaL_fileresult(L, 0, NULL);
}
- lua_pushboolean(L, resultp);
+ lua_pushinteger(L, resultp);
return 1;
}
Stage this hunk [y,n,q,a,d,/,K,g,e,?]? y
$ git diff --cached
diff --git a/unistring/unistring.c b/unistring/unistring.c
index 62bbc8a..ea85d65 100644
--- a/unistring/unistring.c
+++ b/unistring/unistring.c
@@ -318,7 +318,7 @@ static int lunistring_is_cased(lua_State *L) {
return luaL_fileresult(L, 0, NULL);
}
- lua_pushboolean(L, resultp);
+ lua_pushinteger(L, resultp);
return 1;
}
^ permalink raw reply related
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Christian Couder @ 2016-11-18 14:34 UTC (permalink / raw)
To: Duy Nguyen
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Christian Couder
In-Reply-To: <CACsJy8BZNfESmFv=V89Cq-b+aMJWLH=qhXHNE8inZZRjvXB33Q@mail.gmail.com>
On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> (sorry I got sick in the last few weeks and could not respond to this earlier)
(Yeah, I have also been sick during the last few weeks.)
> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Le 6 nov. 2016 09:16, "Junio C Hamano" <gitster@pobox.com> a écrit :
>>>
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>> > I think it is easier for user to be able to just set core.splitIndex
>>> > to true to enable split-index.
>>>
>>> You can have that exact benefit by making core.splitIndex to
>>> bool-or-more. If your default is 20%, take 'true' as if the user
>>> specified 20% and take 'false' as if the user specified 100% (or is
>>> it 0%? I do not care about the details but you get the point).
>
>> Then if we ever add 'auto' and the user wants for example 10% instead of the
>> default 20%, we will have to make it accept things like "auto,10".
(Sorry for writing the above on my phone which added HTML, so that it
didn't reach the list.)
> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
> do the right thing, just re-split the index when it makes sense", "no"
> is disabled of course. If the user wants to be specific, just write
> "10" or some other percentage.(and either 0 or 100 would mean enable
> split-index but do not re-split automatically, let _me_ do it when I
> want it)
The meaning of a future "auto" option for "core.splitIndex" could be
"use the split-index feature only if the number of entries in whole
index is greater than 10000 (by default)".
If there is no difference between "true" and "auto" then, when users
who have "core.splitIndex=true" will migrate to the git version that
adds the "auto" feature, their repos with under 10000 entires will not
use the split-index feature anymore. These users may then be annoyed
that the behavior has been switched under them, and that the
split-index feature is not always used despite having
"core.splitIndex=true" in their config.
^ permalink raw reply
* RE: merge --no-ff is NOT mentioned in help
From: Vanderhoof, Tzadik @ 2016-11-18 14:51 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Mike Rappazzo, git@vger.kernel.org
In-Reply-To: <20161117222142.mca6lmhj5mvl4gbp@sigill.intra.peff.net>
> On Thu, Nov 17, 2016 at 09:10:22AM -0800, Junio C Hamano wrote:
>
> > People interested may want to try the attached single-liner patch to
> > see how the output from _ALL_ commands that use parse-options API
> > looks when given "-h". It could be that the result may not be too
> > bad.
>
> The output is less ugly than I expected, but still a bit cluttered IMHO.
> I was surprised that the column-adjustment did not need tweaked, but the code correctly increments "pos" from the return value of fprintf, which just works.
>
> Looking at the output for --ff, though:
>
> --[no-]ff allow fast-forward (default)
>
> I do not think it's improving the situation nearly as much as if we made the primary option "--no-ff" with a NONEG flga, and then added back in a HIDDEN "--ff". I thought we had done that in other cases, but I can't seem to find any. But it would make "--no-ff" the primary form, which makes sense, as "--ff" is already the default.
>
> Another option would be to teach parse-options to somehow treat the negated form as primary in the help text. That's a bit more code, but might be usable in other places.
>
> -Peff
>
What about leaving the help as is, but adding a sentence at the end (or beginning?) like: "The following options may be negated by adding 'no-' after the double dashes"?
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-18 17:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Turner, git, spearce
In-Reply-To: <xmqqzil0tza6.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 15, 2016 at 09:42:57AM -0800, Junio C Hamano wrote:
> >> Hmph. I think I tried David's original under GIT_TEST_LONG and saw
> >> it got stuck; could be the same issue, I guess.
> >
> > It works OK here. I think it is just that the test is really slow (by
> > design).
>
> Yeah, I think what I recalled was my old attempt to run the
> follow-up "any SHA-1" patch without this one.
Right, that makes sense.
So I don't feel like we have a good patch for the general case yet, and
I'm probably not going to get around to implementing it anytime soon. So
I'd suggest taking David's original patch (to punt when the response is
empty) in the meantime.
It doesn't fix all cases, but if fixes _a_ case, and probably one of the
most likely one in practice. I don't think it can cause any regressions.
It's a "snooping" solution like mine, but it makes many fewer
assumptions about the protocol. We know that an empty response cannot
possibly advance fetch-pack further because we won't have sent it any
bytes. :)
I do think the commit message could be improved based on the discussion
here, though (at the very least to describe the nature of the deadlock,
and that we are choosing only one of the possible solutions, and why).
-Peff
^ permalink raw reply
* RE: [PATCH] remote-curl: don't hang when a server dies before any output
From: David Turner @ 2016-11-18 17:04 UTC (permalink / raw)
To: 'Jeff King', Junio C Hamano
Cc: git@vger.kernel.org, spearce@spearce.org
In-Reply-To: <20161118170147.g7nbkxpyihwkk6fw@sigill.intra.peff.net>
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Friday, November 18, 2016 12:02 PM
> To: Junio C Hamano
> Cc: David Turner; git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
>
> On Tue, Nov 15, 2016 at 09:42:57AM -0800, Junio C Hamano wrote:
>
> > >> Hmph. I think I tried David's original under GIT_TEST_LONG and saw
> > >> it got stuck; could be the same issue, I guess.
> > >
> > > It works OK here. I think it is just that the test is really slow
> > > (by design).
> >
> > Yeah, I think what I recalled was my old attempt to run the follow-up
> > "any SHA-1" patch without this one.
>
> Right, that makes sense.
>
> So I don't feel like we have a good patch for the general case yet, and
> I'm probably not going to get around to implementing it anytime soon.
I'm confused -- it sounds like your patch actually does work (that is, that Junio's failure was not caused by your patch but by the absence of our patches). And your patch handles more cases than mine. So we should probably use it instead of mine.
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-18 17:08 UTC (permalink / raw)
To: David Turner; +Cc: Junio C Hamano, git@vger.kernel.org, spearce@spearce.org
In-Reply-To: <764305554fa74779ad5fb956aa2b658a@EXMBNJE7.ad.twosigma.com>
On Fri, Nov 18, 2016 at 05:04:59PM +0000, David Turner wrote:
> > So I don't feel like we have a good patch for the general case yet,
> > and I'm probably not going to get around to implementing it anytime
> > soon.
>
> I'm confused -- it sounds like your patch actually does work (that is,
> that Junio's failure was not caused by your patch but by the absence
> of our patches). And your patch handles more cases than mine. So we
> should probably use it instead of mine.
No, mine passes the vanilla test suite, but fails with GIT_TEST_LONG.
If the want/have negotiation takes multiple rounds, the intermediate
rounds don't end on a flush packet, and my patch causes remote-curl to
complain that the response was truncated.
I think you could fix it by teaching remote-curl that the final packet
must be a flush _or_ contain an ACK/NAK, but I didn't try it. That's
getting a bit invasive and brittle.
-Peff
^ permalink raw reply
* Re: [PATCH 08/16] update submodules: add depopulate_submodule
From: Brandon Williams @ 2016-11-18 17:46 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Junio C Hamano, Jonathan Nieder, Martin Fick,
David Turner
In-Reply-To: <CAGZ79kYE1JooyKMDsEM5=6OWxbCOL3q2=Et3nL7mMcayxtLZxA@mail.gmail.com>
On 11/17, Stefan Beller wrote:
> On Thu, Nov 17, 2016 at 2:42 PM, Stefan Beller <sbeller@google.com> wrote:
> >
> > I think I'll just write this functionality in C and optionally expose
> > it via the submodule--helper,
> > such that the user facing git-submodule.sh only has to call that helper.
>
> I think it will roughly look like this:
> (white space mangled)
>
>
> commit e72ef244c667920c874247aa32aa55845500aac8
> Author: Stefan Beller <sbeller@google.com>
> Date: Thu Nov 17 16:14:46 2016 -0800
>
> submodule--helper: add intern-git-dir function
>
> When a submodule has its git dir inside the working dir, the submodule
> support for checkout that we plan to add in a later patch will fail.
>
> Add functionality to migrate the git directory to be embedded
> into the superprojects git directory.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4beeda5..4f31100 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1076,6 +1076,21 @@ static int resolve_remote_submodule_branch(int
> argc, const char **argv,
> return 0;
> }
>
> +static int intern_git_dir(int argc, const char **argv, const char *prefix)
> +{
> + int i;
> + struct pathspec pathspec;
> + struct module_list list = MODULE_LIST_INIT;
> +
> + if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> + return 1;
> +
> + for (i = 0; i < list.nr; i++)
> + migrate_submodule_gitdir(list.entries[i]->name);
> +
> + return 0;
> +}
> +
> struct cmd_struct {
> const char *cmd;
> int (*fn)(int, const char **, const char *);
> @@ -1090,7 +1105,8 @@ static struct cmd_struct commands[] = {
> {"resolve-relative-url", resolve_relative_url},
> {"resolve-relative-url-test", resolve_relative_url_test},
> {"init", module_init},
> - {"remote-branch", resolve_remote_submodule_branch}
> + {"remote-branch", resolve_remote_submodule_branch},
> + {"intern-git-dir", intern_git_dir}
> };
>
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/submodule.c b/submodule.c
> index 45b9060..e513bba 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1335,3 +1335,42 @@ void prepare_submodule_repo_env(struct argv_array *out)
> }
> argv_array_push(out, "GIT_DIR=.git");
> }
> +
> +/*
> + * Migrate the given submodule (and all its submodules recursively) from
> + * having its git directory within the working tree to the git dir nested
> + * in its superprojects git dir under modules/.
> + */
> +void migrate_submodule_gitdir(const char *path)
> +{
> + char *old_git_dir;
> + const char *new_git_dir;
> + const struct submodule *sub;
> +
> + struct child_process cp = CHILD_PROCESS_INIT;
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + argv_array_pushl(&cp.args, "submodule", "foreach", "--recursive",
> + "git", "submodule--helper" "intern-git-dir", NULL);
> +
> + if (run_command(&cp))
> + die(_("Could not migrate git directory in submodule '%s'"),
> + path);
> +
> + old_git_dir = xstrfmt("%s/.git", path);
> + if (read_gitfile(old_git_dir))
> + /* If it is an actual gitfile, it doesn't need migration. */
> + goto out;
> +
> + sub = submodule_from_path(null_sha1, path);
This should probably be checked to see if sub not NULL before
dereferencing it right?
> + new_git_dir = git_common_path("modules/%s", sub->name);
> +
> + if (rename(old_git_dir, new_git_dir) < 0)
> + die_errno(_("Could not migrate git directory from %s to %s"),
> + old_git_dir, new_git_dir);
> +
> + connect_work_tree_and_git_dir(path, new_git_dir);
> +out:
> + free(old_git_dir);
> +}
> diff --git a/submodule.h b/submodule.h
> index aac202c..143ec18 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -90,5 +90,6 @@ extern int parallel_submodules(void);
> * retaining any config in the environment.
> */
> extern void prepare_submodule_repo_env(struct argv_array *out);
> +extern void migrate_submodule_gitdir(const char *path);
>
> #endif
--
Brandon Williams
^ permalink raw reply
* RE: [PATCH] remote-curl: don't hang when a server dies before any output
From: David Turner @ 2016-11-18 17:48 UTC (permalink / raw)
To: 'Jeff King'
Cc: Junio C Hamano, git@vger.kernel.org, spearce@spearce.org
In-Reply-To: <20161118170855.22p3vhcocdxgmpy4@sigill.intra.peff.net>
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Friday, November 18, 2016 12:09 PM
> To: David Turner
> Cc: Junio C Hamano; git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
>
> On Fri, Nov 18, 2016 at 05:04:59PM +0000, David Turner wrote:
>
> > > So I don't feel like we have a good patch for the general case yet,
> > > and I'm probably not going to get around to implementing it anytime
> > > soon.
> >
> > I'm confused -- it sounds like your patch actually does work (that is,
> > that Junio's failure was not caused by your patch but by the absence
> > of our patches). And your patch handles more cases than mine. So we
> > should probably use it instead of mine.
>
> No, mine passes the vanilla test suite, but fails with GIT_TEST_LONG.
> If the want/have negotiation takes multiple rounds, the intermediate
> rounds don't end on a flush packet, and my patch causes remote-curl to
> complain that the response was truncated.
>
> I think you could fix it by teaching remote-curl that the final packet
> must be a flush _or_ contain an ACK/NAK, but I didn't try it. That's
> getting a bit invasive and brittle.
OK, I'll re-roll mine with a better message.
^ permalink raw reply
* [PATCH v2] remote-curl: don't hang when a server dies before any output
From: David Turner @ 2016-11-18 17:58 UTC (permalink / raw)
To: git, peff, spearce; +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.
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".
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 v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-18 18:18 UTC (permalink / raw)
To: Jacob Keller; +Cc: Karthik Nayak, Git mailing list
In-Reply-To: <CA+P7+xo847HRkm1Kur0mdB15OgwVMOckuK5fXNMkPxDGkK1XGA@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>> strip=1,strip=-1, which could be
>>>> done but ...
>>>
>>> ... would be unnecessary if this is the only use case:
>>>
>>>> strbuf_addf(&fmt,
>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>> local.buf, remote.buf);
>>>
>>> You can "strip to leave only 2 components" and compare the result
>>> with refs/remotes instead, no?
>>>
>>
>> Of course, my only objective was that someone would find it useful to
>> have these two additional
>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>
>> --
>> Regards,
>> Karthik Nayak
>
> I think having strip and rstrip make sense, (along with support for
> negative numbers) I don't think we need to make them work together
> unless someone is interested, since we can use strip=-2 to get the
> behavior we need today.
I am OK with multiple strips Karthik suggests, e.g.
%(refname:strip=1,rstrip=-1)
if it is cleanly implemented.
I have a bit of trouble with these names, though. If we call one
strip and the other rstrip, to only those who know about rstrip it
would be clear that strip is about stripping from the left. Perhaps
we should call it lstrip for symmetry and ease-of-remembering?
refs/heads/master:lstrip=-1 => master (strip all but one level
from the left)
refs/heads/master:rstrip=-2 => refs/heads (strip all but two
levels from the right)
refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
from the left and then strip all but one level from the right)
I dunno.
^ permalink raw reply
* Re: [PATCH 08/16] update submodules: add depopulate_submodule
From: Stefan Beller @ 2016-11-18 18:25 UTC (permalink / raw)
To: Brandon Williams
Cc: git@vger.kernel.org, Junio C Hamano, Jonathan Nieder, Martin Fick,
David Turner
In-Reply-To: <20161118174607.GP66382@google.com>
On Fri, Nov 18, 2016 at 9:46 AM, Brandon Williams <bmwill@google.com> wrote:
>> +
>> + sub = submodule_from_path(null_sha1, path);
>
> This should probably be checked to see if sub not NULL before
> dereferencing it right?
yeah, will fix.
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Junio C Hamano @ 2016-11-18 18:28 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, spearce
In-Reply-To: <20161118170147.g7nbkxpyihwkk6fw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So I don't feel like we have a good patch for the general case yet, and
> I'm probably not going to get around to implementing it anytime soon. So
> I'd suggest taking David's original patch (to punt when the response is
> empty) in the meantime.
Yup, we are on the same page; the above matches what I wrote in the
draft of the next issue of What's cooking report last night.
> I do think the commit message could be improved based on the discussion
> here, though (at the very least to describe the nature of the deadlock,
> and that we are choosing only one of the possible solutions, and why).
Thanks. That sounds sensible.
^ permalink raw reply
* Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents
From: Stefan Beller @ 2016-11-18 19:25 UTC (permalink / raw)
To: Heiko Voigt
Cc: git@vger.kernel.org, Brandon Williams, Junio C Hamano,
Jonathan Nieder, Martin Fick, David Turner
In-Reply-To: <20161117133538.GF39230@book.hvoigt.net>
On Thu, Nov 17, 2016 at 5:35 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Tue, Nov 15, 2016 at 03:06:46PM -0800, Stefan Beller wrote:
>> Extend rmdir_or_warn() to remove the directories of those submodules which
>> are scheduled for removal. Also teach verify_clean_submodule() to check
>> that a submodule configured to be removed is not modified before scheduling
>> it for removal.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
> [...]
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -2,6 +2,7 @@
>> * Various trivial helper wrappers around standard functions
>> */
>> #include "cache.h"
>> +#include "submodule.h"
>>
>> static void do_nothing(size_t size)
>> {
>> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)
>>
>> int rmdir_or_warn(const char *file)
>> {
>> + if (submodule_is_interesting(file, null_sha1)
>> + && depopulate_submodule(file))
>> + return -1;
>
> How about untracked files inside submodules? Should we not care about
> them? With this code the entire directory will be removed. In general
> git would not remove a directory with untracked files in it even though
> it is removed in the database. I wonder if we should imitate that here
> and just remove files that are tracked by git so that users do not loose
> files that might be precious to them.
Well from the superprojects perspective a submodule looks like a file,
so if the checkout involved removing a file it had to either be clean or the
checkout failed. So I though we'd go with a similar way here?
We need to stop when untracked files are present, I am not sure if we need
to care if the files are ignored, though.
In this version of the series, the test added in patch 14, testing for
treating untracked files properly is a failing test. So I agree that we should
not just delete them; I'll fix that in the next version of the series.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Jakub Narębski @ 2016-11-18 19:58 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Jacob Keller
In-Reply-To: <20161108201211.25213-4-Karthik.188@gmail.com>
W dniu 08.11.2016 o 21:11, Karthik Nayak pisze:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Implement %(if:equals=<string>) wherein the if condition is only
> satisfied if the value obtained between the %(if:...) and %(then) atom
> is the same as the given '<string>'.
>
> Similarly, implement (if:notequals=<string>) wherein the if condition
> is only satisfied if the value obtained between the %(if:...) and
> %(then) atom is differnt from the given '<string>'.
^^^^^^^^
s/differnt/different/ <-- typo
>
> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.
Nb. the syntax reminds me a bit of RPM SPEC language.
>
> Add tests and Documentation for the same.
>
> 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>
> ---
> Documentation/git-for-each-ref.txt | 3 +++
> ref-filter.c | 43 +++++++++++++++++++++++++++++++++-----
> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++
> 3 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fed8126..b7b8560 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -155,6 +155,9 @@ if::
> evaluating the string before %(then), this is useful when we
> use the %(HEAD) atom which prints either "*" or " " and we
> want to apply the 'if' condition only on the 'HEAD' ref.
So %(if) is actually %(if:notempty) ? Just kidding.
> + Append ":equals=<string>" or ":notequals=<string>" to compare
> + the value between the %(if:...) and %(then) atoms with the
> + given string.
>
> In addition to the above, for commit and tag objects, the header
> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 8392303..44481c3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -22,6 +22,8 @@ struct align {
> };
>
> struct if_then_else {
> + const char *if_equals,
> + *not_equals;
I guess using anonymous structs from C11 here...
> unsigned int then_atom_seen : 1,
> else_atom_seen : 1,
> condition_satisfied : 1;
> @@ -49,6 +51,10 @@ static struct used_atom {
> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
> unsigned int nlines;
> } contents;
> + struct {
> + const char *if_equals,
> + *not_equals;
> + } if_then_else;
...to avoid code duplication there is rather out of question?
> enum { O_FULL, O_SHORT } objectname;
> } u;
> } *used_atom;
> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
> string_list_clear(¶ms, 0);
> }
>
> +static void if_atom_parser(struct used_atom *atom, const char *arg)
> +{
> + if (!arg)
> + return;
> + else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
> + ;
> + else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
> + ;
Those ';' should be perfectly aligned, isn't it?
[...]
> +test_expect_success 'check %(if:equals=<string>)' '
> + git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + Found master
> + Not master
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if:notequals=<string>)' '
> + git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
> + cat >expect <<-\EOF &&
> + Found master
> + Not master
> + EOF
> + test_cmp expect actual
> +'
Nice!
--
Jakub Narębski
^ permalink raw reply
* [PATCH v4 0/6] recursively grep across submodules
From: Brandon Williams @ 2016-11-18 19:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, jonathantanmy, gitster
In-Reply-To: <1478908273-190166-1-git-send-email-bmwill@google.com>
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.
Brandon Williams (6):
submodules: add helper functions to determine presence of submodules
submodules: load gitmodules file from commit sha1
grep: add submodules as a grep source type
grep: optionally recurse into submodules
grep: enable recurse-submodules to work on <tree> objects
grep: search history of moved submodules
Documentation/git-grep.txt | 14 ++
builtin/grep.c | 393 ++++++++++++++++++++++++++++++++++---
cache.h | 2 +
config.c | 8 +-
git.c | 2 +-
grep.c | 16 +-
grep.h | 1 +
submodule-config.c | 6 +-
submodule-config.h | 3 +
submodule.c | 50 +++++
submodule.h | 3 +
t/t7814-grep-recurse-submodules.sh | 213 ++++++++++++++++++++
12 files changed, 679 insertions(+), 32 deletions(-)
create mode 100755 t/t7814-grep-recurse-submodules.sh
-- interdiff based on 'bw/grep-recurse-submodules'
diff --git a/builtin/grep.c b/builtin/grep.c
index 1cd2be9..747b0c3 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -518,9 +518,8 @@ static void compile_submodule_options(const struct grep_opt *opt,
* 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);
+ argv_array_pushf(&submodule_options, "--threads=%d",
+ (num_threads + 1) / 2);
/* Add Pathspecs */
argv_array_push(&submodule_options, "--");
@@ -542,7 +541,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
struct work_item *w = opt->output_priv;
end_of_base = strchr(gs->name, ':');
- if (end_of_base)
+ if (gs->identifier && end_of_base)
name = end_of_base + 1;
else
name = gs->name;
@@ -558,13 +557,13 @@ static int grep_submodule_launch(struct grep_opt *opt,
/*
* 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
+ * 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 (end_of_base)
+ if (gs->identifier && end_of_base)
argv_array_pushf(&cp.args, "--parent-basename=%.*s",
(int) (end_of_base - gs->name),
gs->name);
@@ -572,7 +571,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
/* Add options */
for (i = 0; i < submodule_options.argc; i++) {
/*
- * If there is a <tree> identifier for the submodule, add the
+ * 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
@@ -615,17 +614,20 @@ static int grep_submodule_launch(struct grep_opt *opt,
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_populated(path))) {
+ if (!is_submodule_initialized(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) {
- path = git_path("modules/%s",
- submodule_from_path(null_sha1, path)->name);
+ 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)))
+ if(!(is_directory(path) && is_git_directory(path)))
return 0;
} else {
return 0;
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index ee173ad..7d66716 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -60,7 +60,7 @@ test_expect_success 'grep and nested submodules' '
submodule/sub/a:foobar
EOF
- git grep -e "bar" --recurse-submodules > actual &&
+ git grep -e "bar" --recurse-submodules >actual &&
test_cmp expect actual
'
@@ -71,7 +71,7 @@ test_expect_success 'grep and multiple patterns' '
submodule/sub/a:foobar
EOF
- git grep -e "bar" --and -e "foo" --recurse-submodules > actual &&
+ git grep -e "bar" --and -e "foo" --recurse-submodules >actual &&
test_cmp expect actual
'
@@ -80,7 +80,7 @@ test_expect_success 'grep and multiple patterns' '
b/b:bar
EOF
- git grep -e "bar" --and --not -e "foo" --recurse-submodules > actual &&
+ git grep -e "bar" --and --not -e "foo" --recurse-submodules >actual &&
test_cmp expect actual
'
@@ -92,7 +92,7 @@ test_expect_success 'basic grep tree' '
HEAD:submodule/sub/a:foobar
EOF
- git grep -e "bar" --recurse-submodules HEAD > actual &&
+ git grep -e "bar" --recurse-submodules HEAD >actual &&
test_cmp expect actual
'
@@ -103,7 +103,7 @@ test_expect_success 'grep tree HEAD^' '
HEAD^:submodule/a:foobar
EOF
- git grep -e "bar" --recurse-submodules HEAD^ > actual &&
+ git grep -e "bar" --recurse-submodules HEAD^ >actual &&
test_cmp expect actual
'
@@ -113,7 +113,7 @@ test_expect_success 'grep tree HEAD^^' '
HEAD^^:b/b:bar
EOF
- git grep -e "bar" --recurse-submodules HEAD^^ > actual &&
+ git grep -e "bar" --recurse-submodules HEAD^^ >actual &&
test_cmp expect actual
'
@@ -123,49 +123,80 @@ test_expect_success 'grep tree and pathspecs' '
HEAD:submodule/sub/a:foobar
EOF
- git grep -e "bar" --recurse-submodules HEAD -- submodule > actual &&
+ 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_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 &&
+ git -C parent submodule add ../sub dir/sub &&
git -C parent commit -m "add submodule" &&
cat >expect <<-\EOF &&
+ dir/sub/file:foobar
file:foobar
- sub/file:foobar
EOF
- git -C parent grep -e "foobar" --recurse-submodules > actual &&
+ git -C parent grep -e "foobar" --recurse-submodules >actual &&
test_cmp expect actual &&
- git -C parent mv sub sub-moved &&
+ 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 &&
+ git -C parent grep -e "foobar" --recurse-submodules >actual &&
test_cmp expect actual &&
cat >expect <<-\EOF &&
+ HEAD^:dir/sub/file:foobar
HEAD^:file:foobar
- HEAD^:sub/file:foobar
EOF
- git -C parent grep -e "foobar" --recurse-submodules HEAD^ > actual &&
- test_cmp expect actual &&
-
- rm -rf parent sub
+ git -C parent grep -e "foobar" --recurse-submodules HEAD^ >actual &&
+ test_cmp expect actual
'
test_incompatible_with_recurse_submodules ()
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 1/6] submodules: add helper functions to determine presence of 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>
Add two helper functions to submodules.c.
`is_submodule_initialized()` checks if a submodule has been initialized
at a given path and `is_submodule_populated()` check if a submodule
has been checked out at a given path.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
submodule.c | 38 ++++++++++++++++++++++++++++++++++++++
submodule.h | 2 ++
2 files changed, 40 insertions(+)
diff --git a/submodule.c b/submodule.c
index 6f7d883..f5107f0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,6 +198,44 @@ void gitmodules_config(void)
}
}
+/*
+ * Determine if a submodule has been initialized at a given 'path'
+ */
+int is_submodule_initialized(const char *path)
+{
+ int ret = 0;
+ const struct submodule *module = NULL;
+
+ module = submodule_from_path(null_sha1, path);
+
+ if (module) {
+ char *key = xstrfmt("submodule.%s.url", module->name);
+ char *value = NULL;
+
+ ret = !git_config_get_string(key, &value);
+
+ free(value);
+ free(key);
+ }
+
+ return ret;
+}
+
+/*
+ * Determine if a submodule has been populated at a given 'path'
+ */
+int is_submodule_populated(const char *path)
+{
+ int ret = 0;
+ char *gitdir = xstrfmt("%s/.git", path);
+
+ if (resolve_gitdir(gitdir))
+ ret = 1;
+
+ free(gitdir);
+ return ret;
+}
+
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
{
diff --git a/submodule.h b/submodule.h
index d9e197a..6ec5f2f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+extern int is_submodule_initialized(const char *path);
+extern int is_submodule_populated(const char *path);
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 3/6] grep: add submodules as a grep source type
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>
Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
type in the various switch statements in grep.c.
When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
identifier can either be NULL (to indicate that the working tree will be
used) or a SHA1 (the REV of the submodule to be grep'd). If the
identifier is a SHA1 then we want to fall through to the
`GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
grep.c | 16 +++++++++++++++-
grep.h | 1 +
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/grep.c b/grep.c
index 1194d35..0dbdc1d 100644
--- a/grep.c
+++ b/grep.c
@@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
case GREP_SOURCE_FILE:
gs->identifier = xstrdup(identifier);
break;
+ case GREP_SOURCE_SUBMODULE:
+ if (!identifier) {
+ gs->identifier = NULL;
+ break;
+ }
+ /*
+ * FALL THROUGH
+ * If the identifier is non-NULL (in the submodule case) it
+ * will be a SHA1 that needs to be copied.
+ */
case GREP_SOURCE_SHA1:
gs->identifier = xmalloc(20);
hashcpy(gs->identifier, identifier);
break;
case GREP_SOURCE_BUF:
gs->identifier = NULL;
+ break;
}
}
@@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
switch (gs->type) {
case GREP_SOURCE_FILE:
case GREP_SOURCE_SHA1:
+ case GREP_SOURCE_SUBMODULE:
free(gs->buf);
gs->buf = NULL;
gs->size = 0;
@@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
return grep_source_load_sha1(gs);
case GREP_SOURCE_BUF:
return gs->buf ? 0 : -1;
+ case GREP_SOURCE_SUBMODULE:
+ break;
}
- die("BUG: invalid grep_source type");
+ die("BUG: invalid grep_source type to load");
}
void grep_source_load_driver(struct grep_source *gs)
diff --git a/grep.h b/grep.h
index 5856a23..267534c 100644
--- a/grep.h
+++ b/grep.h
@@ -161,6 +161,7 @@ struct grep_source {
GREP_SOURCE_SHA1,
GREP_SOURCE_FILE,
GREP_SOURCE_BUF,
+ GREP_SOURCE_SUBMODULE,
} type;
void *identifier;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 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
* [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 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
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