* [PATCHv2 1/3] submodule config: inline config_from_{name, path}
From: Stefan Beller @ 2016-11-21 23:27 UTC (permalink / raw)
To: git, gitster; +Cc: bmwill, jacob.keller, Stefan Beller
In-Reply-To: <20161121232709.8906-1-sbeller@google.com>
There is no other user of config_from_{name, path}, such that there is no
reason for the existence of these one liner functions. Just inline these
to increase readability.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
submodule-config.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..15ffab6af4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,
return submodule;
}
-static const struct submodule *config_from_path(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *path)
-{
- return config_from(cache, commit_sha1, path, lookup_path);
-}
-
-static const struct submodule *config_from_name(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *name)
-{
- return config_from(cache, commit_sha1, name, lookup_name);
-}
-
static void ensure_cache_init(void)
{
if (is_cache_init)
@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
const char *name)
{
ensure_cache_init();
- return config_from_name(&the_submodule_cache, commit_sha1, name);
+ return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
}
const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path)
{
ensure_cache_init();
- return config_from_path(&the_submodule_cache, commit_sha1, path);
+ return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
}
void submodule_free(void)
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-21 23:27 UTC (permalink / raw)
To: git, gitster; +Cc: bmwill, jacob.keller, Stefan Beller
In-Reply-To: <20161121232709.8906-1-sbeller@google.com>
It is also possible to pass in a tree hash to lookup a submodule config.
Make it clear by naming the variables accrodingly. Looking up a submodule
config by tree hash will come in handy in a later patch.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/technical/api-submodule-config.txt | 8 ++--
submodule-config.c | 47 ++++++++++++------------
submodule-config.h | 4 +-
t/t7411-submodule-config.sh | 11 ++++++
4 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..768458580f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,15 @@ Functions
Can be passed to the config parsing infrastructure to parse
local (worktree) submodule configurations.
-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`::
+`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
The same as above but lookup by name.
-If given the null_sha1 as commit_sha1 the local configuration of a
+If given the null_sha1 as commit_or_tree the local configuration of a
submodule will be returned (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
diff --git a/submodule-config.c b/submodule-config.c
index 15ffab6af4..4c5f5d074b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, const char *arg)
return parse_push_recurse(opt, arg, 1);
}
-static void warn_multiple_config(const unsigned char *commit_sha1,
+static void warn_multiple_config(const unsigned char *commit_or_tree,
const char *name, const char *option)
{
const char *commit_string = "WORKTREE";
- if (commit_sha1)
- commit_string = sha1_to_hex(commit_sha1);
+ if (commit_or_tree)
+ commit_string = sha1_to_hex(commit_or_tree);
warning("%s:.gitmodules, multiple configurations found for "
"'submodule.%s.%s'. Skipping second one!",
commit_string, name, option);
@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char *commit_sha1,
struct parse_config_parameter {
struct submodule_cache *cache;
- const unsigned char *commit_sha1;
+ const unsigned char *commit_or_tree;
const unsigned char *gitmodules_sha1;
int overwrite;
};
@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"path");
else {
if (submodule->path)
@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, void *data)
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"fetchrecursesubmodules");
else
submodule->fetch_recurse = parse_fetch_recurse(
@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
strcmp(value, "dirty") &&
@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && submodule->url) {
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"url");
} else {
free((void *) submodule->url);
@@ -351,21 +351,21 @@ static int parse_config(const char *var, const char *value, void *data)
ret = config_error_nonbool(var);
else if (!me->overwrite &&
submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"update");
else if (parse_submodule_update_strategy(value,
&submodule->update_strategy) < 0)
die(_("invalid value for %s"), var);
} else if (!strcmp(item.buf, "shallow")) {
if (!me->overwrite && submodule->recommend_shallow != -1)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"shallow");
else
submodule->recommend_shallow =
git_config_bool(var, value);
} else if (!strcmp(item.buf, "branch")) {
if (!me->overwrite && submodule->branch)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"branch");
else {
free((void *)submodule->branch);
@@ -379,18 +379,18 @@ 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,
+static int gitmodule_sha1_from_commit(const unsigned char *commit_or_tree,
unsigned char *gitmodules_sha1,
struct strbuf *rev)
{
int ret = 0;
- if (is_null_sha1(commit_sha1)) {
+ if (is_null_sha1(commit_or_tree)) {
hashclr(gitmodules_sha1);
return 1;
}
- strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1));
+ strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_or_tree));
if (get_sha1(rev->buf, gitmodules_sha1) >= 0)
ret = 1;
@@ -402,7 +402,7 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
* revisions.
*/
static const struct submodule *config_from(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *key,
+ const unsigned char *commit_or_tree, const char *key,
enum lookup_type lookup_type)
{
struct strbuf rev = STRBUF_INIT;
@@ -418,7 +418,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
* return the first submodule. Can be used to check whether
* there are any submodules parsed.
*/
- if (!commit_sha1 || !key) {
+ if (!commit_or_tree || !key) {
struct hashmap_iter iter;
struct submodule_entry *entry;
@@ -428,7 +428,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
return entry->config;
}
- if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev))
+ if (!gitmodule_sha1_from_commit(commit_or_tree, sha1, &rev))
goto out;
switch (lookup_type) {
@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
/* fill the submodule config into the cache */
parameter.cache = cache;
- parameter.commit_sha1 = commit_sha1;
+ // todo: get the actual tree here:
+ parameter.commit_or_tree = commit_or_tree;
parameter.gitmodules_sha1 = sha1;
parameter.overwrite = 0;
git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
@@ -484,7 +485,7 @@ int parse_submodule_config_option(const char *var, const char *value)
{
struct parse_config_parameter parameter;
parameter.cache = &the_submodule_cache;
- parameter.commit_sha1 = NULL;
+ parameter.commit_or_tree = NULL;
parameter.gitmodules_sha1 = null_sha1;
parameter.overwrite = 1;
@@ -492,18 +493,18 @@ int parse_submodule_config_option(const char *var, const char *value)
return parse_config(var, value, ¶meter);
}
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
const char *name)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
+ return config_from(&the_submodule_cache, commit_or_tree, name, lookup_name);
}
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
const char *path)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
+ return config_from(&the_submodule_cache, commit_or_tree, path, lookup_path);
}
void submodule_free(void)
diff --git a/submodule-config.h b/submodule-config.h
index d05c542d2c..99df8e593c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -25,9 +25,9 @@ struct submodule {
int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
const char *path);
void submodule_free(void);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 47562ce465..301ed5e48f 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -93,6 +93,17 @@ test_expect_success 'error message contains blob reference' '
)
'
+test_expect_success 'using tree sha1 works' '
+ (
+ cd super &&
+ tree=$(git rev-parse HEAD^{tree}) &&
+ commit=$(git rev-parse HEAD^{commit}) &&
+ test-submodule-config $commit b >expect &&
+ test-submodule-config $tree b >actual &&
+ test_cmp expect actual
+ )
+'
+
cat >super/expect_url <<EOF
Submodule url: 'git@somewhere.else.net:a.git' for path 'b'
Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element
From: Stefan Beller @ 2016-11-21 23:27 UTC (permalink / raw)
To: git, gitster; +Cc: bmwill, jacob.keller, Stefan Beller
In-Reply-To: <20161121232709.8906-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/technical/api-submodule-config.txt | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 768458580f..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -55,8 +55,11 @@ Functions
The same as above but lookup by name.
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
+zeroed out. So in the normal case, when HEAD:.gitmodules is parsed first and
+then overlayed with the repository configuration, the null_sha1 entry contains
+the local configuration of a submodule (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
For an example usage see test-submodule-config.c.
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Jeff King @ 2016-11-21 23:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <xmqqtwb0ir4o.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 21, 2016 at 11:12:39AM -0800, Junio C Hamano wrote:
> > Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
> > this case, but called out the fact that "cd subdir && git stripspace"
> > would never have worked. So one step back, 2 steps forward, and Dscho's
> > patch is the right step forward.
>
> Yes, absolutely.
>
> I sent out a set of proposed amends, and the one for this step 1/3
> runs the command inside a subdirectory to force it not to find the
> .git/config file relative to its pwd, which can reveal the existing
> breakage without help by b9605bc4f2 ;-) hence can be forked for
> older maintenance tracks.
Makes sense, and your amended patch looks good.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting
From: Stefan Beller @ 2016-11-22 0:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79kY=tzrsn7rS4UsRfSku_pKNNWNDc2OiTO-4-vg5h8NwWQ@mail.gmail.com>
On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Can the effect of this change demonstrated in a new test? There
>> must be a scenario where the current behaviour is broken and this
>> change fixes an incorrect computation of relative path, no?
I do not think the current usage exposes this bug in
connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
which fills the second parameter `git_dir` via a call to read_gitfile,
which itself produces an absolute path.
The latest patch of this series however passes in relative path for the
respective git directories.
So the commit message of this patch is misleading at least.
Maybe:
The current caller of connect_work_tree_and_git_dir passes
an absolute path for the `git_dir` parameter. In the future patch
we will also pass in relative path for `git_dir`. Extend the functionality
of connect_work_tree_and_git_dir to take relative paths for parameters.
We could work around this in the future patch by computing the absolute
path for the git_dir in the calling site, however accepting relative
paths for either parameter makes the API for this function easier
to use.
While at it, change `real_work_tree` to be non const as we own
the memory.
>
> I found the latest patch of this series broken without this patch.
> I'll try to find existing broken code, though.
^ permalink raw reply
* Re: [PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element
From: Brandon Williams @ 2016-11-22 0:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jacob.keller
In-Reply-To: <20161121232709.8906-4-sbeller@google.com>
On 11/21, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/technical/api-submodule-config.txt | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
> index 768458580f..a91c1f085e 100644
> --- a/Documentation/technical/api-submodule-config.txt
> +++ b/Documentation/technical/api-submodule-config.txt
> @@ -55,8 +55,11 @@ Functions
>
> The same as above but lookup by name.
>
> -If given the null_sha1 as commit_or_tree the local configuration of a
> -submodule will be returned (e.g. consolidated values from local git
> +Whenever a submodule configuration is parsed in `parse_submodule_config_option`
> +via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
s/will be overwrite/will overwrite
> +zeroed out. So in the normal case, when HEAD:.gitmodules is parsed first and
> +then overlayed with the repository configuration, the null_sha1 entry contains
> +the local configuration of a submodule (e.g. consolidated values from local git
> configuration and the .gitmodules file in the worktree).
>
> For an example usage see test-submodule-config.c.
> --
> 2.11.0.rc2.18.g0126045.dirty
>
--
Brandon Williams
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Brandon Williams @ 2016-11-22 0:11 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jacob.keller
In-Reply-To: <20161121232709.8906-3-sbeller@google.com>
On 11/21, Stefan Beller wrote:
>
> switch (lookup_type) {
> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>
> /* fill the submodule config into the cache */
> parameter.cache = cache;
> - parameter.commit_sha1 = commit_sha1;
> + // todo: get the actual tree here:
s/todo/TODO
Makes it more clear that this is a TODO
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
From: Stephan Beyer @ 2016-11-22 0:12 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1b54-0ac61448-98b1-4c7d-bfdd-47af95f52153-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> Reimplement the `bisect_state` shell function in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call it from
> git-bisect.sh .
>
> Using `--bisect-state` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> `bisect_head` is called from `bisect_state` thus its not required to
> introduce another subcommand.
Missing comma before "thus", and "it is" (or "it's") instead of "its" :)
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1767916..1481c6d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
> return 0;
> }
>
> +static char *bisect_head(void)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_head()))
> + return "HEAD";
> + else
> + return "BISECT_HEAD";
> +}
This is very shellish.
In C I'd expect something like
static int bisect_head_sha1(unsigned char *sha)
{
int res;
if (is_empty_or_missing_file(git_path_bisect_head()))
res = get_sha1("HEAD", sha);
else
res = get_sha1("BISECT_HEAD", sha);
if (res)
return error(_("Could not find BISECT_HEAD or HEAD."));
return 0;
}
> +
> +static int bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
> + const char *state = argv[0];
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, state))
> + return -1;
> +
> + if (!argc)
> + die(_("Please call `--bisect-state` with at least one argument"));
I think this check should move to cmd_bisect__helper. There are also the
other argument number checks.
> +
> + if (argc == 1 && one_of(state, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + const char *bisected_head = xstrdup(bisect_head());
> + const char *hex[1];
Maybe:
const char *hex;
> + unsigned char sha1[20];
> +
> + if (get_sha1(bisected_head, sha1))
> + die(_("Bad rev input: %s"), bisected_head);
And instead of...
> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
> + return -1;
> +
> + *hex = xstrdup(sha1_to_hex(sha1));
> + if (check_expected_revs(hex, 1))
> + return -1;
... simply:
hex = xstrdup(sha1_to_hex(sha1));
if (set_state(terms, state, hex)) {
free(hex);
return -1;
}
free(hex);
where:
static int set_state(struct bisect_terms *terms, const char *state,
const char *hex)
{
if (bisect_write(state, hex, terms, 0))
return -1;
if (check_expected_revs(&hex, 1))
return -1;
return 0;
}
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
> + one_of(state, terms->term_good, "skip", NULL)) {
> + int i;
> + struct string_list hex = STRING_LIST_INIT_DUP;
> +
> + for (i = 1; i < argc; i++) {
> + unsigned char sha1[20];
> +
> + if (get_sha1(argv[i], sha1)) {
> + string_list_clear(&hex, 0);
> + die(_("Bad rev input: %s"), argv[i]);
> + }
> + string_list_append(&hex, sha1_to_hex(sha1));
> + }
> + for (i = 0; i < hex.nr; i++) {
... And replace this:
> + const char **hex_string = (const char **) &hex.items[i].string;
> + if(bisect_write(state, *hex_string, terms, 0)) {
> + string_list_clear(&hex, 0);
> + return -1;
> + }
> + if (check_expected_revs(hex_string, 1)) {
> + string_list_clear(&hex, 0);
> + return -1;
> + }
by:
const char *hex_str = hex.items[i].string;
if (set_state(terms, state, hex_string)) {
string_list_clear(&hex, 0);
return -1;
}
> + }
> + string_list_clear(&hex, 0);
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if (!strcmp(state, terms->term_bad))
> + die(_("'git bisect %s' can take only one argument."),
> + terms->term_bad);
> +
> + return -1;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
> + N_("mark the state of ref (or refs)"), BISECT_STATE),
"mark the state of the given revs"
Note that rev != ref
> @@ -902,6 +980,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> terms.term_bad = "bad";
> res = bisect_autostart(&terms);
> break;
> + case BISECT_STATE:
> + if (argc == 0)
> + die(_("--bisect-state requires at least 1 argument"));
"at least one revision"
> diff --git a/git-bisect.sh b/git-bisect.sh
> index cd56551..a9eebbb 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -61,44 +51,7 @@ bisect_skip() {
> esac
> all="$all $revs"
> done
> - eval bisect_state 'skip' $all
[...deleted lines...]
> + eval git bisect--helper --bisect-state 'skip' $all
I think you don't need "eval" here any longer.
> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state="$TERM_GOOD"
> fi
>
> - # We have to use a subshell because "bisect_state" can exit.
> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> + # We have to use a subshell because "--bisect-state" can exit.
> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
The new comment is funny, but you don't need a subshell here any longer.
~Stephan
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-22 0:15 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano, Jacob Keller
In-Reply-To: <20161122001148.GD149321@google.com>
On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/21, Stefan Beller wrote:
>>
>> switch (lookup_type) {
>> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>>
>> /* fill the submodule config into the cache */
>> parameter.cache = cache;
>> - parameter.commit_sha1 = commit_sha1;
>> + // todo: get the actual tree here:
>
> s/todo/TODO
>
> Makes it more clear that this is a TODO
>
The // is more annoying here. I'll review these changes closely
before sending out v3.
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Brandon Williams @ 2016-11-22 0:16 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Junio C Hamano, Jacob Keller
In-Reply-To: <CAGZ79kY+0WHFgXfVmEmw7H-qwuUcWcSssRyMzg2XqsGSDq+FCw@mail.gmail.com>
On 11/21, Stefan Beller wrote:
> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams <bmwill@google.com> wrote:
> > On 11/21, Stefan Beller wrote:
> >>
> >> switch (lookup_type) {
> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
> >>
> >> /* fill the submodule config into the cache */
> >> parameter.cache = cache;
> >> - parameter.commit_sha1 = commit_sha1;
> >> + // todo: get the actual tree here:
> >
> > s/todo/TODO
> >
> > Makes it more clear that this is a TODO
> >
>
> The // is more annoying here. I'll review these changes closely
> before sending out v3.
Well I prefer // to /* */ but that's not the style we use :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jonathan Nieder @ 2016-11-22 0:44 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen
In-Reply-To: <20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> This passes the test suite (after the adjustments in the
> previous patches), but there's a risk of regression for any
> cases where the fallback usually works fine but the code
> isn't exercised by the test suite. So by itself, this
> commit is a potential step backward, but lets us take two
> steps forward once we've identified and fixed any such
> instances.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> environment.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/environment.c b/environment.c
> index cd5aa57..b1743e6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -164,8 +164,11 @@ static void setup_git_env(void)
> const char *replace_ref_base;
>
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> - if (!git_dir)
> + if (!git_dir) {
> + if (!startup_info->have_repository)
> + die("BUG: setup_git_env called without repository");
> git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> + }
This trips reproducibly for
git ls-remote https://kernel.googlesource.com/pub/scm/git/git
when run outside of a git repository.
Backtrace:
#0 setup_git_env () at environment.c:172
#1 get_git_dir () at environment.c:214
#2 get_helper at transport-helper.c:127
#3 get_refs_list (for_push=0) at transport-helper.c:1038
#4 transport_get_remote_refs at transport.c:1076
#5 cmd_ls_remote at builtin/ls-remote.c:97
transport-helper.c:127 is
argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
get_git_dir());
That code is pretty clearly wrong. Should it be made conditional
on have_git_dir(), like this?
Thanks,
Jonathan
transport-helper.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35..e4fd982 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
helper->git_cmd = 0;
helper->silent_exec_failure = 1;
- argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
- get_git_dir());
+ if (have_git_dir())
+ argv_array_pushf(&helper->env_array, "%s=%s",
+ GIT_DIR_ENVIRONMENT, get_git_dir());
code = start_command(helper);
if (code < 0 && errno == ENOENT)
--
^ permalink raw reply related
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Stephan Beyer @ 2016-11-22 0:49 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1b29-65f79716-42c6-4327-acda-8c8d0fe05471-000000@eu-west-1.amazonses.com>
Okay Pranit,
this is the last patch for me to review in this series.
Now that I have a coarse overview of what you did, I have the general
remark that imho the "terms" variable should simply be global instead of
being passed around all the time.
I also had some other remarks but I forgot them... maybe they come to my
mind again when I see patch series v16.
I also want to remark again that I am not a Git developer and only
reviewed this because of my interest in git-bisect. So some of my
suggestions might conflict with other beliefs here. For example, I
consider it very bad style to leak memory... but Git is rather written
as a scripting tool than a genuine library, so perhaps many people here
do not care about it as long as it works...
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c18ca07..b367d8d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> terms->term_good = arg;
> } else if (!strcmp(arg, "--term-bad") ||
> !strcmp(arg, "--term-new")) {
> - const char *next_arg;
This should already have been removed in patch 15/27, not here.
> @@ -875,6 +875,117 @@ static int bisect_log(void)
> return status;
> }
>
> +static int get_next_word(const char *line, int pos, struct strbuf *word)
> +{
> + int i, len = strlen(line), begin = 0;
> + strbuf_reset(word);
> + for (i = pos; i < len; i++) {
> + if (line[i] == ' ' && begin)
> + return i + 1;
> +
> + if (!begin)
> + begin = 1;
> + strbuf_addch(word, line[i]);
> + }
> +
> + return i;
> +}
> +
> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
> +{
> + struct strbuf line = STRBUF_INIT;
> + struct strbuf word = STRBUF_INIT;
> + FILE *fp = NULL;
(The initialization is not necessary here.)
> + int res = 0;
> +
> + if (is_empty_or_missing_file(filename)) {
> + error(_("no such file with name '%s' exists"), filename);
The error message is misleading if the file exists but is empty.
Maybe something like "cannot read file '%s' for replaying"?
> + res = -1;
> + goto finish;
goto fail;
:D
> + }
> +
> + if (bisect_reset(NULL)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + fp = fopen(filename, "r");
> + if (!fp) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + while (strbuf_getline(&line, fp) != EOF) {
> + int pos = 0;
> + while (pos < line.len) {
> + pos = get_next_word(line.buf, pos, &word);
> +
> + if (!strcmp(word.buf, "git")) {
> + continue;
> + } else if (!strcmp(word.buf, "git-bisect")) {
> + continue;
> + } else if (!strcmp(word.buf, "bisect")) {
> + continue;
> + } else if (!strcmp(word.buf, "#")) {
> + break;
Maybe it is more robust to check whether word.buf begins with #
> + }
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, word.buf)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + if (!strcmp(word.buf, "start")) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + sq_dequote_to_argv_array(line.buf+pos, &argv);
> + if (bisect_start(terms, 0, argv.argv, argv.argc)) {
> + argv_array_clear(&argv);
> + res = -1;
> + goto finish;
goto fail;
> + }
> + argv_array_clear(&argv);
> + break;
> + }
> +
> + if (one_of(word.buf, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + if (bisect_write(word.buf, line.buf+pos, terms, 0)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> + break;
> + }
> +
> + if (!strcmp(word.buf, "terms")) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + sq_dequote_to_argv_array(line.buf+pos, &argv);
> + if (bisect_terms(terms, argv.argv, argv.argc)) {
> + argv_array_clear(&argv);
> + res = -1;
> + goto finish;
goto fail;
> + }
> + argv_array_clear(&argv);
> + break;
> + }
> +
> + error(_("?? what are you talking about?"));
I know this string is taken from the original source. However, even
something like error(_("Replay file contains rubbish (\"%s\")"),
word.buf) is more informative.
> + res = -1;
> + goto finish;
goto fail;
> + }
> + }
> + goto finish;
+fail:
+ res = -1;
I just wanted to make finally clear what I was referring to by the
"goto fail" trick. :D
Also I think the readability could be improved by extracting the body of
the outer while loop into an extra function replay_line(). Then most of
my suggested "goto fail;" lines simply become "return -1;" :)
> @@ -998,6 +1112,13 @@ int cmd_bisect__helper(...)
> die(_("--bisect-log requires 0 arguments"));
> res = bisect_log();
> break;
> + case BISECT_REPLAY:
> + if (argc != 1)
> + die(_("--bisect-replay requires 1 argument"));
I'd keep the (already translated) string from the original source:
"No logfile given"
Cheers,
Stephan
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-22 1:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano, Jacob Keller
In-Reply-To: <20161122001658.GE149321@google.com>
On Mon, Nov 21, 2016 at 4:16 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/21, Stefan Beller wrote:
>> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams <bmwill@google.com> wrote:
>> > On 11/21, Stefan Beller wrote:
>> >>
>> >> switch (lookup_type) {
>> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>> >>
>> >> /* fill the submodule config into the cache */
>> >> parameter.cache = cache;
>> >> - parameter.commit_sha1 = commit_sha1;
>> >> + // todo: get the actual tree here:
>> >
>> > s/todo/TODO
>> >
>> > Makes it more clear that this is a TODO
>> >
>>
>> The // is more annoying here. I'll review these changes closely
>> before sending out v3.
>
> Well I prefer // to /* */ but that's not the style we use :)
background: Initially I assume we would need to do work here as that part
is exposed to the user in error messages, such that we maybe want to
go the reverse direction and not state a tree but instead the commit containing
it. Since then I decided that it is not worth to optimize for some
hypothetical future
and hence I did not go with the internal todo note I put there. Then I
forgot about
it and it just showed up in the patch here.
Having looked through the patch again, the rest looks clean to me.
Stefan
>
> --
> Brandon Williams
^ permalink raw reply
* [PATCHv3 0/3] submodule-config: clarify/cleanup docs and header
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
replacing sb/submodule-config-cleanup
v3:
diff to current origin/sb/submodule-config-cleanup:
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..e06a3fd2de 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,17 +49,17 @@ Functions
`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
The same as above but lookup by name.
Whenever a submodule configuration is parsed in `parse_submodule_config_option`
-via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
-zeroed out. So in the normal case, when HEAD:.gitmodules is parsed first and
-then overlayed with the repository configuration, the null_sha1 entry contains
-the local configuration of a submodule (e.g. consolidated values from local git
+via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
+So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
+with the repository configuration, the null_sha1 entry contains the local
+configuration of a submodule (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
For an example usage see test-submodule-config.c.
diff --git a/submodule-config.c b/submodule-config.c
index 4c5f5d074b..d88a746c56 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -448,7 +448,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,
/* fill the submodule config into the cache */
parameter.cache = cache;
- // todo: get the actual tree here:
parameter.commit_or_tree = commit_or_tree;
parameter.gitmodules_sha1 = sha1;
parameter.overwrite = 0;
v2:
addressed Jacobs concerns in patch2, fixing all occurrences of commit_sha1.
Thanks,
Stefan
interdiff to v1:
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,7 +49,7 @@ Functions
`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
v1:
A small series that would have helped me understand the submodule config
once again.
Thanks,
Stefan
Stefan Beller (3):
submodule config: inline config_from_{name, path}
submodule-config: rename commit_sha1 to commit_or_tree
submodule-config: clarify parsing of null_sha1 element
Documentation/technical/api-submodule-config.txt | 13 ++++--
submodule-config.c | 58 ++++++++++--------------
submodule-config.h | 4 +-
t/t7411-submodule-config.sh | 11 +++++
4 files changed, 44 insertions(+), 42 deletions(-)
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv3 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122013550.1800-1-sbeller@google.com>
It is also possible to pass in a tree hash to lookup a submodule config.
Make it clear by naming the variables accrodingly. Looking up a submodule
config by tree hash will come in handy in a later patch.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/technical/api-submodule-config.txt | 8 ++---
submodule-config.c | 46 ++++++++++++------------
submodule-config.h | 4 +--
t/t7411-submodule-config.sh | 11 ++++++
4 files changed, 40 insertions(+), 29 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..768458580f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,15 @@ Functions
Can be passed to the config parsing infrastructure to parse
local (worktree) submodule configurations.
-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`::
+`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
The same as above but lookup by name.
-If given the null_sha1 as commit_sha1 the local configuration of a
+If given the null_sha1 as commit_or_tree the local configuration of a
submodule will be returned (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
diff --git a/submodule-config.c b/submodule-config.c
index 15ffab6af4..d88a746c56 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, const char *arg)
return parse_push_recurse(opt, arg, 1);
}
-static void warn_multiple_config(const unsigned char *commit_sha1,
+static void warn_multiple_config(const unsigned char *commit_or_tree,
const char *name, const char *option)
{
const char *commit_string = "WORKTREE";
- if (commit_sha1)
- commit_string = sha1_to_hex(commit_sha1);
+ if (commit_or_tree)
+ commit_string = sha1_to_hex(commit_or_tree);
warning("%s:.gitmodules, multiple configurations found for "
"'submodule.%s.%s'. Skipping second one!",
commit_string, name, option);
@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char *commit_sha1,
struct parse_config_parameter {
struct submodule_cache *cache;
- const unsigned char *commit_sha1;
+ const unsigned char *commit_or_tree;
const unsigned char *gitmodules_sha1;
int overwrite;
};
@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"path");
else {
if (submodule->path)
@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, void *data)
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"fetchrecursesubmodules");
else
submodule->fetch_recurse = parse_fetch_recurse(
@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
strcmp(value, "dirty") &&
@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && submodule->url) {
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"url");
} else {
free((void *) submodule->url);
@@ -351,21 +351,21 @@ static int parse_config(const char *var, const char *value, void *data)
ret = config_error_nonbool(var);
else if (!me->overwrite &&
submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"update");
else if (parse_submodule_update_strategy(value,
&submodule->update_strategy) < 0)
die(_("invalid value for %s"), var);
} else if (!strcmp(item.buf, "shallow")) {
if (!me->overwrite && submodule->recommend_shallow != -1)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"shallow");
else
submodule->recommend_shallow =
git_config_bool(var, value);
} else if (!strcmp(item.buf, "branch")) {
if (!me->overwrite && submodule->branch)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"branch");
else {
free((void *)submodule->branch);
@@ -379,18 +379,18 @@ 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,
+static int gitmodule_sha1_from_commit(const unsigned char *commit_or_tree,
unsigned char *gitmodules_sha1,
struct strbuf *rev)
{
int ret = 0;
- if (is_null_sha1(commit_sha1)) {
+ if (is_null_sha1(commit_or_tree)) {
hashclr(gitmodules_sha1);
return 1;
}
- strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1));
+ strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_or_tree));
if (get_sha1(rev->buf, gitmodules_sha1) >= 0)
ret = 1;
@@ -402,7 +402,7 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
* revisions.
*/
static const struct submodule *config_from(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *key,
+ const unsigned char *commit_or_tree, const char *key,
enum lookup_type lookup_type)
{
struct strbuf rev = STRBUF_INIT;
@@ -418,7 +418,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
* return the first submodule. Can be used to check whether
* there are any submodules parsed.
*/
- if (!commit_sha1 || !key) {
+ if (!commit_or_tree || !key) {
struct hashmap_iter iter;
struct submodule_entry *entry;
@@ -428,7 +428,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
return entry->config;
}
- if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev))
+ if (!gitmodule_sha1_from_commit(commit_or_tree, sha1, &rev))
goto out;
switch (lookup_type) {
@@ -448,7 +448,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
/* fill the submodule config into the cache */
parameter.cache = cache;
- parameter.commit_sha1 = commit_sha1;
+ parameter.commit_or_tree = commit_or_tree;
parameter.gitmodules_sha1 = sha1;
parameter.overwrite = 0;
git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
@@ -484,7 +484,7 @@ int parse_submodule_config_option(const char *var, const char *value)
{
struct parse_config_parameter parameter;
parameter.cache = &the_submodule_cache;
- parameter.commit_sha1 = NULL;
+ parameter.commit_or_tree = NULL;
parameter.gitmodules_sha1 = null_sha1;
parameter.overwrite = 1;
@@ -492,18 +492,18 @@ int parse_submodule_config_option(const char *var, const char *value)
return parse_config(var, value, ¶meter);
}
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
const char *name)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
+ return config_from(&the_submodule_cache, commit_or_tree, name, lookup_name);
}
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
const char *path)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
+ return config_from(&the_submodule_cache, commit_or_tree, path, lookup_path);
}
void submodule_free(void)
diff --git a/submodule-config.h b/submodule-config.h
index d05c542d2c..99df8e593c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -25,9 +25,9 @@ struct submodule {
int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
const char *path);
void submodule_free(void);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 47562ce465..301ed5e48f 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -93,6 +93,17 @@ test_expect_success 'error message contains blob reference' '
)
'
+test_expect_success 'using tree sha1 works' '
+ (
+ cd super &&
+ tree=$(git rev-parse HEAD^{tree}) &&
+ commit=$(git rev-parse HEAD^{commit}) &&
+ test-submodule-config $commit b >expect &&
+ test-submodule-config $tree b >actual &&
+ test_cmp expect actual
+ )
+'
+
cat >super/expect_url <<EOF
Submodule url: 'git@somewhere.else.net:a.git' for path 'b'
Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv3 1/3] submodule config: inline config_from_{name, path}
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122013550.1800-1-sbeller@google.com>
There is no other user of config_from_{name, path}, such that there is no
reason for the existence of these one liner functions. Just inline these
to increase readability.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
submodule-config.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..15ffab6af4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,
return submodule;
}
-static const struct submodule *config_from_path(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *path)
-{
- return config_from(cache, commit_sha1, path, lookup_path);
-}
-
-static const struct submodule *config_from_name(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *name)
-{
- return config_from(cache, commit_sha1, name, lookup_name);
-}
-
static void ensure_cache_init(void)
{
if (is_cache_init)
@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
const char *name)
{
ensure_cache_init();
- return config_from_name(&the_submodule_cache, commit_sha1, name);
+ return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
}
const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path)
{
ensure_cache_init();
- return config_from_path(&the_submodule_cache, commit_sha1, path);
+ return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
}
void submodule_free(void)
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122013550.1800-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/technical/api-submodule-config.txt | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 768458580f..e06a3fd2de 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -55,8 +55,11 @@ Functions
The same as above but lookup by name.
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
+So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
+with the repository configuration, the null_sha1 entry contains the local
+configuration of a submodule (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
For an example usage see test-submodule-config.c.
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
From: Stefan Beller @ 2016-11-22 2:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <xmqqy40ch6wp.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Does this format correctly?
>
> I somehow thought that second and subsequent paragraphs continued
> with "+" want no indentation before them. See for example the
> Values section in config.txt and see how entries for boolean:: and
> color:: use multiple '+' paragraphs.
>
> If we do not have to refrain from indenting the second and
> subsequent paragraphs, that would be great for readability, but I
> take the existing practice as telling me that we cannot do that X-<.
Will fix and test in a resend.
>
>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> + git init sub2 &&
>> + test_commit -C sub2 first &&
>> + git add sub2 &&
>> + git commit -m superproject
>> +'
>> +
>> +test_expect_success 'intern the git dir fails for incomplete submodules' '
>> + test_must_fail git submodule interngitdirs &&
>> + # check that we did not break the repository:
>> + git status
>> +'
>
> It is not clear what the last "git status" wants to test.
Any errors that I ran into when manually truing to embed a submodules
git dir, were fatal with `git status` already, e.g. missing or wrong
call of connect_work_tree_and_git_dir were my main failure points.
So I guess we should test a bit more extensively, maybe
git status >expect
git submodule embedgitdirs
git status >actual
test_cmp expect actual
# further testing via
test -f ..
test -d ..
> In the
> extreme, if the failed "git submodule" command did
>
> rm -fr .git ?* && git init
>
> wouldn't "git status" still succeed?
In that particular case you'd get
$ git status
fatal: Not a git repository (or any parent up to mount point ....)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ echo $?
128
but I get the idea, which is why I propose the double check via status.
That would detect any logical change for the repository, e.g. a
change to the .gitmodules file.
>
> What are the minimum things that we expect from "did not break" to
> see? sub2/.git is still a directory and is a valid repository? The
> contents of the .git/modules/* before and after the "git submodule"
> does not change? Some other things?
I thought about making up a name for such a repo and creating that engry
in .gitmodules. But I refrained from doing so, because it seems too much
for this command.
I dunno, but I would suspect the double status is fine here, too?
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-11-22 2:41 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Duy Nguyen
In-Reply-To: <20161122004421.GA12263@google.com>
On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > - if (!git_dir)
> > + if (!git_dir) {
> > + if (!startup_info->have_repository)
> > + die("BUG: setup_git_env called without repository");
> > git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> > + }
>
> This trips reproducibly for
>
> git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
> #0 setup_git_env () at environment.c:172
> #1 get_git_dir () at environment.c:214
> #2 get_helper at transport-helper.c:127
> #3 get_refs_list (for_push=0) at transport-helper.c:1038
> #4 transport_get_remote_refs at transport.c:1076
> #5 cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
> argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> get_git_dir());
>
> That code is pretty clearly wrong. Should it be made conditional
> on have_git_dir(), like this?
Yeah, I think making it conditional makes the most sense. Just trying to
think of cases that might not be covered by your patch:
- if we are not in a repository, we shouldn't ever need to override an
existing $GIT_DIR from the environment. Because if $GIT_DIR is
present, then we _would_ be in a repo if it's valid, and die if it
isn't.
- not setting $GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So worst case it may expend a little
effort to try to find a repository and fail (I think remote-curl
would do this to try to find repo-level config).
Both of those points assume that we've already called
setup_git_directory_gently(), but I think that's a reasonable
assumption. And certainly is true for ls-remote, and should be for any
git command that invokes the transport code.
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
> helper->git_cmd = 0;
> helper->silent_exec_failure = 1;
>
> - argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> - get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(&helper->env_array, "%s=%s",
> + GIT_DIR_ENVIRONMENT, get_git_dir());
So yeah, I think this is the extent of the change needed.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCHv2 1/3] submodule config: inline config_from_{name, path}
From: Junio C Hamano @ 2016-11-22 3:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jacob.keller
In-Reply-To: <20161121232709.8906-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> There is no other user of config_from_{name, path}, such that there is no
> reason for the existence of these one liner functions. Just inline these
> to increase readability.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Makes sense.
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Junio C Hamano @ 2016-11-22 3:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jacob.keller
In-Reply-To: <20161121232709.8906-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> It is also possible to pass in a tree hash to lookup a submodule config.
> Make it clear by naming the variables accrodingly. Looking up a submodule
> config by tree hash will come in handy in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Yeah, I noticed that while reading the previous round, too, but...
> -`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
> +`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
>
> The same as above but lookup by name.
Doesn't (the) "above", aka submodule_from_path() want the same
treatment?
Also the explanation of "above" has room for improvement. Namely it
says:
Lookup values for one submodule by its commit_sha1 and path.
I do not think the commit-sha1 (or commit-or-tree-object-name) is
"ITS" object name for the submodule. The name belongs to the
containing superproject commit (or tree), no?
Given a tree-ish in the superproject and a path, return the
submodule that is bound at the path in the named tree.
is what I would write for that one. Thinking about it a bit
further, "treeish_name" would be a much better name for the variable
than "commit_or_tree", as "treeish" is an established short and
sweet word that means "commit_or_tree", and having a "name"
somewhere in the variable name makes it clear that we are not
passing the pointer to an in-core object (e.g. "struct commit *").
> +test_expect_success 'using tree sha1 works' '
> + (
> + cd super &&
> + tree=$(git rev-parse HEAD^{tree}) &&
> + commit=$(git rev-parse HEAD^{commit}) &&
> + test-submodule-config $commit b >expect &&
> + test-submodule-config $tree b >actual &&
> + test_cmp expect actual
> + )
> +'
Perhaps also test a tag that points at the commit?
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Junio C Hamano @ 2016-11-22 3:40 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, git, Duy Nguyen
In-Reply-To: <20161122004421.GA12263@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> This trips reproducibly for
>
> git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
> #0 setup_git_env () at environment.c:172
> #1 get_git_dir () at environment.c:214
> #2 get_helper at transport-helper.c:127
> #3 get_refs_list (for_push=0) at transport-helper.c:1038
> #4 transport_get_remote_refs at transport.c:1076
> #5 cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
> argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> get_git_dir());
>
> That code is pretty clearly wrong. Should it be made conditional
> on have_git_dir(), like this?
Looks good and I agree with Peff's analysis. Care to wrap it in a
patch with a log message?
Thanks.
>
> Thanks,
> Jonathan
>
> transport-helper.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
> helper->git_cmd = 0;
> helper->silent_exec_failure = 1;
>
> - argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> - get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(&helper->env_array, "%s=%s",
> + GIT_DIR_ENVIRONMENT, get_git_dir());
>
> code = start_command(helper);
> if (code < 0 && errno == ENOENT)
^ permalink raw reply
* Re: [PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element
From: Junio C Hamano @ 2016-11-22 3:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, jacob.keller
In-Reply-To: <20161122013550.1800-4-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +Whenever a submodule configuration is parsed in `parse_submodule_config_option`
> +via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
It will overwrite? It will be overwritten? I guess it is the latter?
> +So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
> +with the repository configuration, the null_sha1 entry contains the local
> +configuration of a submodule (e.g. consolidated values from local git
> configuration and the .gitmodules file in the worktree).
>
> For an example usage see test-submodule-config.c.
^ permalink raw reply
* Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting
From: Junio C Hamano @ 2016-11-22 7:02 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79ka60_D8xfQyBegkXxkTGW5YDMuagB7kjhiCR6NriLR8+A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Can the effect of this change demonstrated in a new test? There
>>> must be a scenario where the current behaviour is broken and this
>>> change fixes an incorrect computation of relative path, no?
>
> I do not think the current usage exposes this bug in
> connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
> which fills the second parameter `git_dir` via a call to read_gitfile,
> which itself produces an absolute path.
OK. Fixing a potential bug as a preparatory step is good.
> The current caller of connect_work_tree_and_git_dir passes
> an absolute path for the `git_dir` parameter. In the future patch
> we will also pass in relative path for `git_dir`. Extend the functionality
> of connect_work_tree_and_git_dir to take relative paths for parameters.
>
> We could work around this in the future patch by computing the absolute
> path for the git_dir in the calling site, however accepting relative
> paths for either parameter makes the API for this function easier
> to use.
Yup, sounds sensible. Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
From: Junio C Hamano @ 2016-11-22 7:07 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79kb_4wWs_90AfsT932iPWbCXf6yRq875JUxoRZjUcsBW5A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So I guess we should test a bit more extensively, maybe
>
> git status >expect
> git submodule embedgitdirs
> git status >actual
> test_cmp expect actual
> # further testing via
> test -f ..
> test -d ..
Something along that line. "status should succeed" does not tell
the readers what kind of breakage the test is expecting to protect
us from. If we are expecting a breakage in embed-git-dirs would
somehow corrupt an existing submodule, which would lead to "status"
that is run in the superproject report the submodule differently,
then comparing output before and after the operation may be a
reasonable test. Going there to the submodule working tree and
checking the health of the repository (of the submodule) may be
another sensible test.
>> In the
>> extreme, if the failed "git submodule" command did
>>
>> rm -fr .git ?* && git init
>>
>> wouldn't "git status" still succeed?
>
> In that particular case you'd get
> $ git status
> fatal: Not a git repository (or any parent up to mount point ....)
Even with "&& git init"? Or you forgot that part?
^ permalink raw reply
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