* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-22 19:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <xmqqzikrpbz2.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 22, 2016 at 11:10:25AM -0800, Junio C Hamano wrote:
> Archive & Upload-archive:
>
> "cd Documentation && git archive --remote=origin" immediately hits
> "BUG: setup_git_env called without repository" if your Git is built
> with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
> 2016-10-20), which will not be part of the upcoming release. And
> 'origin' will probably not be understood from the local config.
Yeah, I think "cd Documentation && git archive --remote" has always been
broken. It doesn't barf until b1ef400eec, but yes, v2.11-rc2 breaks
finding "origin" in the local config.
I agree that doing the "gently" setup will fix the config problem, but
there's more to do to make it work with b1ef400eec outside of a repo.
It looks like read_remotes_file() and read_branches_file() in remote.c
need to learn to silently return when we are not in a git repository (or
alternatively, remote_get_1() should learn not to call them in such a
case).
I doubt this is a critical case (it only kicks in if you are outside a
repo _and_ your name looks like a remote name and not a URL, _and_ it is
not defined by your config, and the result is that we die("BUG") rather
than reporting "you don't have such a remote"). But any time it is
possible to hit a die("BUG"), we should be fixing it.
> I think we can do the "gently" thing there, as we may be retrieving
> a remote archive outside a local repository. We'd need to tweak
> "output" with prefix to compensate for the case in which the command
> is run from a subdirectory, and probably we need to futz with the
> setup_prefix parameter to write_archive(), as a local caller now
> will know if we are in nongit situation.
Makes sense. I think OPT_FILENAME() would handle "output", but yeah, it
looks like write_archive() already tries to be clever about the setup.
> On the upload-archive side that serves "--remote" request, there is
> enter_repo() so we should be covered OK.
Right, that has to run in a repo, and enter_repo() should handle it.
> Mailinfo:
>
> We may want a "gently" thing there to pick up local configuration.
> i18n.commitencoding and mailinfo.scissors in local repository would
> be ignored otherwise.
Yeah, agreed.
> Verify-pack:
>
> This calls git_config() but these days farms out its operation to
> "index-pack", so we should be OK. We may even want to lose the call
> to git_config() which does not affect anything.
I'd be slightly worried that there is a core.* config option that
affects us in a subtle way (e.g., some portability flag for how we do
run_command()). I don't think there is one now, but it seems like a
potential maintenance pitfall. IOW, I think I'd generally prefer to err
on the side of having everything do a gentle setup and load at least the
default config, to avoid surprises.
I don't think it's a high priority, though.
So what do you want to do for v2.11? I think there are minor regressions
in stripspace, archive, and mailinfo with respect to reading
.git/config. The right solution in each case is to do a gentle repo
setup. We have patches for stripspace already, but not yet for the other
two. They _should_ be fairly trivial, but we are getting awfully close
to the release. Do you want to do another round of -rc3? Ship with the
minor regressions and fix them up in v2.11.1? Last-minute revert the
original config topic that introduced the regressions (I'm biased
against that as the author, but I also think it's dangerous; it's a big
enough topic that it might introduce new problems)?
-Peff
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-22 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Jacob Keller
In-Reply-To: <xmqq8tscgp75.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 21, 2016 at 7:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Doesn't (the) "above", aka submodule_from_path() want the same
> treatment?
fixed in a resend.
> 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.
I'll take this.
>
> 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
ok, so treeish_name it is.
>> +'
>
> Perhaps also test a tag that points at the commit?
will fix in a resend.
^ permalink raw reply
* [PATCHv4 0/3] submodule-config: clarify/cleanup docs and header
From: Stefan Beller @ 2016-11-22 20:14 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
replacing sb/submodule-config-cleanup
v4:
* renamed commit_or_tree to treeish_name
* added a test with a tag
* "it will overwrite" (removed the spurious "be").
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 treeish_name
submodule-config: clarify parsing of null_sha1 element
Documentation/technical/api-submodule-config.txt | 14 ++++--
submodule-config.c | 58 ++++++++++--------------
submodule-config.h | 4 +-
t/t7411-submodule-config.sh | 14 ++++++
4 files changed, 48 insertions(+), 42 deletions(-)
--
2.11.0.rc2.4.g3396b6f.dirty
^ permalink raw reply related
* [PATCHv4 1/3] submodule config: inline config_from_{name, path}
From: Stefan Beller @ 2016-11-22 20:14 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122201438.16069-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.4.g3396b6f.dirty
^ permalink raw reply related
* [PATCHv4 2/3] submodule-config: rename commit_sha1 to treeish_name
From: Stefan Beller @ 2016-11-22 20:14 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122201438.16069-1-sbeller@google.com>
It is also possible to pass in any treeish name to lookup a submodule
config. Make it clear by naming the variables accordingly. 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 | 9 ++---
submodule-config.c | 46 ++++++++++++------------
submodule-config.h | 4 +--
t/t7411-submodule-config.sh | 14 ++++++++
4 files changed, 44 insertions(+), 29 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..8285bcc605 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,16 @@ 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 *treeish_name, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Given a tree-ish in the superproject and a path, return the
+ submodule that is bound at the path in the named tree.
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char *treeish_name, 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 treeish_name 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..ec13ab5a3d 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 *treeish_name,
const char *name, const char *option)
{
const char *commit_string = "WORKTREE";
- if (commit_sha1)
- commit_string = sha1_to_hex(commit_sha1);
+ if (treeish_name)
+ commit_string = sha1_to_hex(treeish_name);
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 *treeish_name;
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->treeish_name, 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->treeish_name, 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->treeish_name, 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->treeish_name, 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->treeish_name, 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->treeish_name, 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->treeish_name, 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 *treeish_name,
unsigned char *gitmodules_sha1,
struct strbuf *rev)
{
int ret = 0;
- if (is_null_sha1(commit_sha1)) {
+ if (is_null_sha1(treeish_name)) {
hashclr(gitmodules_sha1);
return 1;
}
- strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1));
+ strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(treeish_name));
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 *treeish_name, 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 (!treeish_name || !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(treeish_name, 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.treeish_name = treeish_name;
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.treeish_name = 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 *treeish_name,
const char *name)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
+ return config_from(&the_submodule_cache, treeish_name, name, lookup_name);
}
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *treeish_name,
const char *path)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
+ return config_from(&the_submodule_cache, treeish_name, 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..d389ae5408 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -93,6 +93,20 @@ test_expect_success 'error message contains blob reference' '
)
'
+test_expect_success 'using different treeishs works' '
+ (
+ cd super &&
+ git tag new_tag &&
+ tree=$(git rev-parse HEAD^{tree}) &&
+ commit=$(git rev-parse HEAD^{commit}) &&
+ test-submodule-config $commit b >expect &&
+ test-submodule-config $tree b >actual.1 &&
+ test-submodule-config new_tag b >actual.2 &&
+ test_cmp expect actual.1 &&
+ test_cmp expect actual.2
+ )
+'
+
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.4.g3396b6f.dirty
^ permalink raw reply related
* [PATCHv4 3/3] submodule-config: clarify parsing of null_sha1 element
From: Stefan Beller @ 2016-11-22 20:14 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122201438.16069-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 8285bcc605..2f2eda377f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -56,8 +56,11 @@ Functions
The same as above but lookup by name.
-If given the null_sha1 as treeish_name 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 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.4.g3396b6f.dirty
^ permalink raw reply related
* Re: [PATCH 2/3] stripspace: respect repository config
From: Junio C Hamano @ 2016-11-22 20:24 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <20161122195015.rztaixpz7zvdxb4u@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So what do you want to do for v2.11? I think there are minor regressions
> in stripspace, archive, and mailinfo with respect to reading
> .git/config. The right solution in each case is to do a gentle repo
> setup. We have patches for stripspace already, but not yet for the other
> two. They _should_ be fairly trivial, but we are getting awfully close
> to the release.
Right.
> Do you want to do another round of -rc3? Ship with the
> minor regressions and fix them up in v2.11.1?
I am leaning towards the former (though we may also end up doing the
latter).
Here is an initial attempt. It turns out that write_archive() must
be callable outside a repository to respond to "git archive --list",
which happens in parse_archive_args(), and the "have-repository?"
check cannot be moved before that to the caller.
No tests yet. Help appreciated ;-)
archive.c | 9 ++-------
archive.h | 2 +-
builtin/archive.c | 6 +++---
builtin/upload-archive.c | 2 +-
git.c | 4 ++--
5 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/archive.c b/archive.c
index dde1ab4c79..901b10264f 100644
--- a/archive.c
+++ b/archive.c
@@ -504,15 +504,11 @@ static int parse_archive_args(int argc, const char **argv,
}
int write_archive(int argc, const char **argv, const char *prefix,
- int setup_prefix, const char *name_hint, int remote)
+ const char *name_hint, int remote)
{
- int nongit = 0;
const struct archiver *ar = NULL;
struct archiver_args args;
- if (setup_prefix && prefix == NULL)
- prefix = setup_git_directory_gently(&nongit);
-
git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
git_config(git_default_config, NULL);
@@ -520,7 +516,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
init_zip_archiver();
argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
- if (nongit) {
+ if (!startup_info->have_repository) {
/*
* We know this will die() with an error, so we could just
* die ourselves; but its error message will be more specific
@@ -528,7 +524,6 @@ int write_archive(int argc, const char **argv, const char *prefix,
*/
setup_git_directory();
}
-
parse_treeish_arg(argv, &args, prefix, remote);
parse_pathspec_arg(argv + 1, &args);
diff --git a/archive.h b/archive.h
index 4a791e1fed..415e0152e2 100644
--- a/archive.h
+++ b/archive.h
@@ -36,7 +36,7 @@ typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
unsigned int mode);
extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
-extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix, const char *name_hint, int remote);
+extern int write_archive(int argc, const char **argv, const char *prefix, const char *name_hint, int remote);
const char *archive_format_from_filename(const char *filename);
extern void *sha1_file_to_archive(const struct archiver_args *args,
diff --git a/builtin/archive.c b/builtin/archive.c
index 49f491413a..f863465a0f 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -85,8 +85,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
const char *output = NULL;
const char *remote = NULL;
struct option local_opts[] = {
- OPT_STRING('o', "output", &output, N_("file"),
- N_("write the archive to this file")),
+ OPT_FILENAME('o', "output", &output,
+ N_("write the archive to this file")),
OPT_STRING(0, "remote", &remote, N_("repo"),
N_("retrieve the archive from remote repository <repo>")),
OPT_STRING(0, "exec", &exec, N_("command"),
@@ -105,5 +105,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
- return write_archive(argc, argv, prefix, 1, output, 0);
+ return write_archive(argc, argv, prefix, output, 0);
}
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index dc872f6a94..cde06977b7 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -43,7 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
}
/* parse all options sent by the client */
- return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);
+ return write_archive(sent_argv.argc, sent_argv.argv, prefix, NULL, 1);
}
__attribute__((format (printf, 1, 2)))
diff --git a/git.c b/git.c
index efa1059fe0..e8b2baf2d1 100644
--- a/git.c
+++ b/git.c
@@ -396,7 +396,7 @@ static struct cmd_struct commands[] = {
{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
{ "annotate", cmd_annotate, RUN_SETUP },
{ "apply", cmd_apply, RUN_SETUP_GENTLY },
- { "archive", cmd_archive },
+ { "archive", cmd_archive, RUN_SETUP_GENTLY },
{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
{ "blame", cmd_blame, RUN_SETUP },
{ "branch", cmd_branch, RUN_SETUP },
@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {
{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
- { "mailinfo", cmd_mailinfo },
+ { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
{ "mailsplit", cmd_mailsplit },
{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
{ "merge-base", cmd_merge_base, RUN_SETUP },
^ permalink raw reply related
* Re: [PATCH] git-gui: pass the branch name to git merge
From: Johannes Sixt @ 2016-11-22 20:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pat Thoyts, René Scharfe, Git Mailing List
In-Reply-To: <xmqqvavfpbph.fsf@gitster.mtv.corp.google.com>
Am 22.11.2016 um 20:16 schrieb Junio C Hamano:
> Can't this be handled on the "git merge FETCH_HEAD" codepath
> instead?
Absolutely. Any takers? ;)
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-22 21:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <xmqqoa17p8k0.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:
> > Do you want to do another round of -rc3? Ship with the
> > minor regressions and fix them up in v2.11.1?
>
> I am leaning towards the former (though we may also end up doing the
> latter).
I think I'd lead towards -rc3, as well. Our schedule is somewhat
arbitrary anyway, and one week is less important than somebody picking
up the new release and trying to puzzle out a regression that we already
have a fix for.
> Here is an initial attempt. It turns out that write_archive() must
> be callable outside a repository to respond to "git archive --list",
> which happens in parse_archive_args(), and the "have-repository?"
> check cannot be moved before that to the caller.
Right, that makes sense. t5000 already tests git-archive outside of a
repo, although it does it in a funny way (it sets GIT_DIR rather than
relying on GIT_CEILING_DIRECTORIES). We should cover that and make sure
that --remote works outside of a repo. Those work now, but we want to
make sure we don't regress them.
And then we want to test that --remote can read a configured remote
name.
And then post-v2.11, we'd want to check that --remote=foo outside of a
repo produces a sane error instead of looking for a bogus
$GIT_DIR/remotes/foo.
Something like this, which does all but the last (and that should
probably happen separately post-release).
---
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 80b238734..09df7f045 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -94,6 +94,20 @@ check_tar() {
'
}
+# run "$@" inside a non-git directory
+nongit () {
+ test -d non-repo ||
+ mkdir non-repo ||
+ return 1
+
+ (
+ GIT_CEILING_DIRECTORIES=$(pwd) &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non-repo &&
+ "$@"
+ )
+}
+
test_expect_success \
'populate workdir' \
'mkdir a &&
@@ -179,6 +193,12 @@ test_expect_success 'git archive --remote' \
'git archive --remote=. HEAD >b5.tar &&
test_cmp_bin b.tar b5.tar'
+test_expect_success 'git archive --remote with configured remote' '
+ git config remote.foo.url . &&
+ git archive --remote=foo HEAD >b5-nick.tar &&
+ test_cmp_bin b.tar b5-nick.tar
+'
+
test_expect_success \
'validate file modification time' \
'mkdir extract &&
@@ -197,9 +217,15 @@ test_expect_success 'git archive with --output, override inferred format' '
test_cmp_bin b.tar d4.zip
'
-test_expect_success \
- 'git archive --list outside of a git repo' \
- 'GIT_DIR=some/non-existing/directory git archive --list'
+test_expect_success 'git archive --list outside of a git repo' '
+ nongit git archive --list
+'
+
+test_expect_success 'git archive --remote outside of a git repo' '
+ git archive HEAD >expect.tar &&
+ nongit git archive --remote="$PWD" HEAD >actual.tar &&
+ test_cmp_bin expect.tar actual.tar
+'
test_expect_success 'clients cannot access unreachable commits' '
test_commit unreachable &&
^ permalink raw reply related
* Re: [PATCH 2/3] stripspace: respect repository config
From: Junio C Hamano @ 2016-11-22 21:22 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <20161122211920.3qdohxdktykqhyga@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:
>
>> > Do you want to do another round of -rc3? Ship with the
>> > minor regressions and fix them up in v2.11.1?
>>
>> I am leaning towards the former (though we may also end up doing the
>> latter).
>
> I think I'd lead towards -rc3, as well. Our schedule is somewhat
> arbitrary anyway, and one week is less important than somebody picking
> up the new release and trying to puzzle out a regression that we already
> have a fix for.
OK. The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
as it takes paths from the command line that needs to be adjusted
for the prefix.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 22 Nov 2016 13:13:16 -0800
Subject: [PATCH] mailinfo: read local configuration
Since b9605bc4f2 ("config: only read .git/config from configured
repos", 2016-09-12), we do not read from ".git/config" unless we
know we are in a repository. "git mailinfo" however didn't do the
repository discovery and instead relied on the old behaviour. This
was mostly OK because it was merely run as a helper program by other
porcelain scripts that first chdir's up to the root of the working
tree.
Teach the command to run a "gentle" version of repository discovery
so that local configuration variables like mailinfo.scissors are
honoured.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/mailinfo.c | 19 +++++++++++++++----
git.c | 2 +-
t/t5100-mailinfo.sh | 13 +++++++++++++
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index f6df274111..e3b62f2fc7 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -11,15 +11,20 @@
static const char mailinfo_usage[] =
"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info";
+static char *prefix_copy(const char *prefix, const char *filename)
+{
+ if (!prefix || is_absolute_path(filename))
+ return xstrdup(filename);
+ return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
+}
+
int cmd_mailinfo(int argc, const char **argv, const char *prefix)
{
const char *def_charset;
struct mailinfo mi;
int status;
+ char *msgfile, *patchfile;
- /* NEEDSWORK: might want to do the optional .git/ directory
- * discovery
- */
setup_mailinfo(&mi);
def_charset = get_commit_output_encoding();
@@ -54,8 +59,14 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
mi.input = stdin;
mi.output = stdout;
- status = !!mailinfo(&mi, argv[1], argv[2]);
+
+ msgfile = prefix_copy(prefix, argv[1]);
+ patchfile = prefix_copy(prefix, argv[2]);
+
+ status = !!mailinfo(&mi, msgfile, patchfile);
clear_mailinfo(&mi);
+ free(msgfile);
+ free(patchfile);
return status;
}
diff --git a/git.c b/git.c
index efa1059fe0..fd3abf85d2 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {
{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
- { "mailinfo", cmd_mailinfo },
+ { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
{ "mailsplit", cmd_mailsplit },
{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
{ "merge-base", cmd_merge_base, RUN_SETUP },
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e6b995161e..7171f67539 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -158,4 +158,17 @@ test_expect_success 'mailinfo handles rfc2822 comment' '
test_cmp "$DATA/comment.expect" comment/info
'
+test_expect_success 'mailinfo with mailinfo.scissors config' '
+ test_config mailinfo.scissors true &&
+ (
+ mkdir sub &&
+ cd sub &&
+ git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
+ ) &&
+ test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
+ test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
+ test_cmp "$DATA/info0014--scissors" info0014.sc
+'
+
+
test_done
--
2.11.0-rc2-170-g4c384f318f
^ permalink raw reply related
* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-22 21:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <20161122211920.3qdohxdktykqhyga@sigill.intra.peff.net>
On Tue, Nov 22, 2016 at 04:19:20PM -0500, Jeff King wrote:
> > > Do you want to do another round of -rc3? Ship with the
> > > minor regressions and fix them up in v2.11.1?
> >
> > I am leaning towards the former (though we may also end up doing the
> > latter).
>
> I think I'd lead towards -rc3, as well. Our schedule is somewhat
I meant s/lead/lean/, of course.
> Something like this, which does all but the last (and that should
> probably happen separately post-release).
As usual, I'm utterly confused by the $(pwd) versus $PWD thing. So let
me review what I did to see if it makes sense. :)
> +# run "$@" inside a non-git directory
> +nongit () {
> + test -d non-repo ||
> + mkdir non-repo ||
> + return 1
> +
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non-repo &&
> + "$@"
> + )
> +}
I copied this bit from t1515, which sets up a similar scenario. I think
it _probably_ works either way because of the environment-variable
conversion magic that Windows bash does.
> +test_expect_success 'git archive --remote outside of a git repo' '
> + git archive HEAD >expect.tar &&
> + nongit git archive --remote="$PWD" HEAD >actual.tar &&
> + test_cmp_bin expect.tar actual.tar
> +'
I'm not sure if it matters here. I almost wrote $TRASH_DIRECTORY, but
that I think is in the same form as $PWD. Either would be fine.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-22 21:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <xmqqeg23p5v3.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:
> OK. The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
> as it takes paths from the command line that needs to be adjusted
> for the prefix.
I wondered at first if our lack of parse-options here was holding us
back from using OPT_FILENAME. Though even if that was the case, it is
probably better to do the minimal fix in this instance, rather than
converting the option parsing.
However, the paths which need munged are not even options, they are
arguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm
surprised we haven't had to deal with this elsewhere, but I guess most
commands don't take arbitrary filenames (things like `add` take
pathspecs, and then the pathspec machinery takes care of the details).
The only other case I could think of is git-apply, and it does use
prefix_filename() correctly.
> +static char *prefix_copy(const char *prefix, const char *filename)
> +{
> + if (!prefix || is_absolute_path(filename))
> + return xstrdup(filename);
> + return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
> +}
So this is more or less a copy of parse-option's fix_filename(), which
makes sense.
I noticed that git-apply does not check is_absolute_path(), but that's
OK, as prefix_filename() does. So I think it would be correct to drop it
here, but it doesn't matter much either way.
> int cmd_mailinfo(int argc, const char **argv, const char *prefix)
> {
> const char *def_charset;
> struct mailinfo mi;
> int status;
> + char *msgfile, *patchfile;
>
> - /* NEEDSWORK: might want to do the optional .git/ directory
> - * discovery
> - */
> setup_mailinfo(&mi);
This part looks straightforward and correct. Dropping the NEEDSWORK is a
nice bonus.
> +test_expect_success 'mailinfo with mailinfo.scissors config' '
> + test_config mailinfo.scissors true &&
> + (
> + mkdir sub &&
> + cd sub &&
> + git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
> + ) &&
> + test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
> + test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
> + test_cmp "$DATA/info0014--scissors" info0014.sc
> +'
And this test makes sense. Even without "sub", it would show the
regression, but it's a good idea to test the sub-directory case to cover
the path-munging.
In the "archive --remote" test I added, we may want to do the same to
show that "--output" points at the correct path.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Junio C Hamano @ 2016-11-22 21:55 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <20161122214305.yrn4uqh4dzzafkd2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> +test_expect_success 'mailinfo with mailinfo.scissors config' '
>> + test_config mailinfo.scissors true &&
>> + (
>> + mkdir sub &&
>> + cd sub &&
>> + git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
>> + ) &&
>> + test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
>> + test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
>> + test_cmp "$DATA/info0014--scissors" info0014.sc
>> +'
>
> And this test makes sense. Even without "sub", it would show the
> regression, but it's a good idea to test the sub-directory case to cover
> the path-munging.
Yup. Obviously during my initial attempt I was scratching my head
wondering where these two files went--they were later found inside
t/ directory which was really bad ;-)
> In the "archive --remote" test I added, we may want to do the same to
> show that "--output" points at the correct path.
Perhaps something like this. By going down one level, we make sure
that it is not sufficient to accidentally read from .git/config to
find out what 'foo' is, and also ../b5-nick.tar that is relative to
the prefix (aka 'a/') ends up at the top-level.
t/t5000-tar-tree.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 09df7f0458..830bf2a2f6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -195,7 +195,10 @@ test_expect_success 'git archive --remote' \
test_expect_success 'git archive --remote with configured remote' '
git config remote.foo.url . &&
- git archive --remote=foo HEAD >b5-nick.tar &&
+ (
+ cd a &&
+ git archive --remote=foo --output=../b5-nick.tar HEAD
+ ) &&
test_cmp_bin b.tar b5-nick.tar
'
^ permalink raw reply related
* Re: [GIT PULL] l10n updates for 2.11.0 round 2
From: Junio C Hamano @ 2016-11-22 22:24 UTC (permalink / raw)
To: Jiang Xin
Cc: Alexander Shopov, Alex Henrie, Ralf Thielow, Jean-Noël Avila,
Marco Paolone, Changwoo Ryu, Vasco Almeida, Dimitriy Ryazantcev,
Peter Krefting, Trần Ngọc Quân, Git List
In-Reply-To: <CANYiYbFSpUmtv1eO_GE_yRU4CsXzd29qV2LTbXa67d=Uxbw2=Q@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Git 2.11.0-rc2 introduced a very small l10n update. Because the
> update windows will be
> closed tomorrow, I made a batch updates. See commit:
>
> * https://github.com/git-l10n/git-po/commit/275588f93
>
> I have a reduced diff for this commit using a custom diff driver, see:
>
> * https://gist.github.com/jiangxin/6384b1e865249228e00385fab84ef3f3
>
>
> 2016-11-22 22:59 GMT+08:00 Jiang Xin <worldhello.net@gmail.com>:
>> Hi Junio,
>>
>> The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:
>>
>> Git 2.11-rc2 (2016-11-17 13:47:36 -0800)
>>
>> are available in the git repository at:
>>
>> git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2
Thanks, pulled.
FYI, we seem to need a bit more time on the code front so we'll have
an unscheduled 2.11-rc3 tomorrow. The final one will be near the
end of the month, tenatively scheduled on Nov 29th.
^ permalink raw reply
* Re: [PATCHv4 0/3] submodule-config: clarify/cleanup docs and header
From: Brandon Williams @ 2016-11-22 22:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, jacob.keller
In-Reply-To: <20161122201438.16069-1-sbeller@google.com>
Series looks good to me. At least the issues I raised are fixed.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-22 22:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479840397-68264-6-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> diff --git a/tree-walk.c b/tree-walk.c
> index 828f435..ff77605 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> */
> if (ps->recursive && S_ISDIR(entry->mode))
> return entry_interesting;
> +
> + /*
> + * When matching against submodules with
> + * wildcard characters, ensure that the entry
> + * at least matches up to the first wild
> + * character. More accurate matching can then
> + * be performed in the submodule itself.
> + */
> + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> + !ps_strncmp(item, match + baselen,
> + entry->path,
> + item->nowildcard_len - baselen))
> + return entry_interesting;
> }
This one (and the other hunk) feels more correct than the previous
round. One thing to keep in mind however is that ps->recursive is
about "do we show a tree as a tree aka 040000, or do we descend into
it to show its contents?", not about "do we recurse into submodules?",
AFAICT.
So this change may have an impact on "git ls-tree -r" with pathspec;
I offhand do not know if that impact is undesirable or not. A test
or two may be in order to illustrate what happens? With a submodule
at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
something like that, perhaps?
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-22 23:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqqshqjnmtc.fsf@gitster.mtv.corp.google.com>
On 11/22, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > diff --git a/tree-walk.c b/tree-walk.c
> > index 828f435..ff77605 100644
> > --- a/tree-walk.c
> > +++ b/tree-walk.c
> > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> > */
> > if (ps->recursive && S_ISDIR(entry->mode))
> > return entry_interesting;
> > +
> > + /*
> > + * When matching against submodules with
> > + * wildcard characters, ensure that the entry
> > + * at least matches up to the first wild
> > + * character. More accurate matching can then
> > + * be performed in the submodule itself.
> > + */
> > + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > + !ps_strncmp(item, match + baselen,
> > + entry->path,
> > + item->nowildcard_len - baselen))
> > + return entry_interesting;
> > }
>
> This one (and the other hunk) feels more correct than the previous
> round. One thing to keep in mind however is that ps->recursive is
> about "do we show a tree as a tree aka 040000, or do we descend into
> it to show its contents?", not about "do we recurse into submodules?",
> AFAICT.
>
> So this change may have an impact on "git ls-tree -r" with pathspec;
> I offhand do not know if that impact is undesirable or not. A test
> or two may be in order to illustrate what happens? With a submodule
> at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> something like that, perhaps?
Maybe unrelated, but it looks like wildcard characters are overridden in
ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-22 23:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <20161122232105.GC65825@google.com>
On 11/22, Brandon Williams wrote:
> On 11/22, Junio C Hamano wrote:
> > Brandon Williams <bmwill@google.com> writes:
> >
> > > diff --git a/tree-walk.c b/tree-walk.c
> > > index 828f435..ff77605 100644
> > > --- a/tree-walk.c
> > > +++ b/tree-walk.c
> > > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> > > */
> > > if (ps->recursive && S_ISDIR(entry->mode))
> > > return entry_interesting;
> > > +
> > > + /*
> > > + * When matching against submodules with
> > > + * wildcard characters, ensure that the entry
> > > + * at least matches up to the first wild
> > > + * character. More accurate matching can then
> > > + * be performed in the submodule itself.
> > > + */
> > > + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > > + !ps_strncmp(item, match + baselen,
> > > + entry->path,
> > > + item->nowildcard_len - baselen))
> > > + return entry_interesting;
> > > }
> >
> > This one (and the other hunk) feels more correct than the previous
> > round. One thing to keep in mind however is that ps->recursive is
> > about "do we show a tree as a tree aka 040000, or do we descend into
> > it to show its contents?", not about "do we recurse into submodules?",
> > AFAICT.
> >
> > So this change may have an impact on "git ls-tree -r" with pathspec;
> > I offhand do not know if that impact is undesirable or not. A test
> > or two may be in order to illustrate what happens? With a submodule
> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> > something like that, perhaps?
>
> Maybe unrelated, but it looks like wildcard characters are overridden in
> ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
> ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
Wrong commit. Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
that disabled wildmatching since it is 'plumbing'
--
Brandon Williams
^ permalink raw reply
* Announce: delaying 2.11 final by one week
From: Junio C Hamano @ 2016-11-22 23:35 UTC (permalink / raw)
To: git
We'd like to cook and merge a few hot-fix topics to 'master' before
the upcoming release, in order to fix minor regressions in
"stripspace" (which affects "rebase -i"), "archive" and "mailinfo"
that were introduced during this cycle. So I'll be cutting -rc3
soon this week, and hopefully tagging the final by the end of this
month.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-22 23:37 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <20161122232815.GD65825@google.com>
Brandon Williams <bmwill@google.com> writes:
>> > So this change may have an impact on "git ls-tree -r" with pathspec;
>> > I offhand do not know if that impact is undesirable or not. A test
>> > or two may be in order to illustrate what happens? With a submodule
>> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
>> > something like that, perhaps?
>>
>> Maybe unrelated, but it looks like wildcard characters are overridden in
>> ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
>> ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
>
> Wrong commit. Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
> that disabled wildmatching since it is 'plumbing'
OK. Things that share tree-walk other than "ls-tree -r" are still
affected, no?
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-22 23:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqqk2bvnl1e.fsf@gitster.mtv.corp.google.com>
On 11/22, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> >> > So this change may have an impact on "git ls-tree -r" with pathspec;
> >> > I offhand do not know if that impact is undesirable or not. A test
> >> > or two may be in order to illustrate what happens? With a submodule
> >> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> >> > something like that, perhaps?
> >>
> >> Maybe unrelated, but it looks like wildcard characters are overridden in
> >> ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
> >> ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
> >
> > Wrong commit. Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
> > that disabled wildmatching since it is 'plumbing'
>
> OK. Things that share tree-walk other than "ls-tree -r" are still
> affected, no?
Yeah potentially, though I'm having a difficult time finding a case that
would actually be affected.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-23 0:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <xmqq7f7vp4ck.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 22, 2016 at 01:55:07PM -0800, Junio C Hamano wrote:
> > And this test makes sense. Even without "sub", it would show the
> > regression, but it's a good idea to test the sub-directory case to cover
> > the path-munging.
>
> Yup. Obviously during my initial attempt I was scratching my head
> wondering where these two files went--they were later found inside
> t/ directory which was really bad ;-)
Heh. Yeah, it's nice when the failure mode doesn't escape the trash
directory, but it's probably not worth worrying about too much.
> > In the "archive --remote" test I added, we may want to do the same to
> > show that "--output" points at the correct path.
>
> Perhaps something like this. By going down one level, we make sure
> that it is not sufficient to accidentally read from .git/config to
> find out what 'foo' is, and also ../b5-nick.tar that is relative to
> the prefix (aka 'a/') ends up at the top-level.
Yeah, your modification looks good.
-Peff
^ permalink raw reply
* dangling commits in worktree
From: Van Oostenryck Luc @ 2016-11-23 0:52 UTC (permalink / raw)
To: git
Hi,
More or less by error I used the fsck command in a worktree and I had
the surprised to see that it reported a lot of dangling commits while it was
not supposed to have one.
I quickly realized that it was the case only in the worktree, in the main dir
things were OK. While experimenting a bit I also saw that git gc had not the
same effect in a worktree than in the main tree (the pack was smaller, more
files were left in objects/xx/ dirs), which is even more odd and a bit
scary when thinking to the pruning.
This seems like a bug to me and googling about it didn't returned anything.
Luc Van Oostenryck
^ permalink raw reply
* Re: [PATCH 1/2] difftool: add the builtin
From: David Aguilar @ 2016-11-23 8:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <7aec571e1b0773ca80ea25453d2650af6a18e095.1479834051.git.johannes.schindelin@gmx.de>
On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
> This adds a builtin difftool that represents a conversion of the current
> Perl script version of the difftool.
>
> The motivation is that Perl scripts are not at all native on Windows,
> and that `git difftool` therefore is pretty slow on that platform, when
> there is no good reason for it to be slow.
>
> In addition, Perl does not really have access to Git's internals. That
> means that any script will always have to jump through unnecessary
> hoops.
Nice!
> The current version of the builtin difftool does not, however, make full
> use of the internals but instead chooses to spawn a couple of Git
> processes, still, to make for an easier conversion. There remains a lot
> of room for improvement, left for a later date.
>
> Note: the original difftool is still called by `git difftool`. To get the
> new, experimental version, call `git builtin-difftool`. The reason: this
> new, experimental, builtin difftool will be shipped as part of Git for
> Windows v2.11.0, to allow for easier large-scale testing.
I like this plan. I was going to ask for an environment
variable (to preset in git-cola) but since Git for Windows is
handling it then everyone benefits.
> diff --git a/builtin/builtin-difftool.c b/builtin/builtin-difftool.c
> new file mode 100644
> index 0000000..9feefcd
> --- /dev/null
> +++ b/builtin/builtin-difftool.c
> @@ -0,0 +1,680 @@
> +/*
> + * "git difftool" builtin command
> + *
> + * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
> + * git-difftool--helper script.
> + *
> + * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
> + * The GIT_DIFF* variables are exported for use by git-difftool--helper.
> + *
> + * Any arguments that are unknown to this script are forwarded to 'git diff'.
> + *
> + * Copyright (C) 2016 Johannes Schindelin
> + */
> +#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +#include "argv-array.h"
> +#include "strbuf.h"
> +#include "lockfile.h"
> +
> +static char *diff_gui_tool;
> +static int trust_exit_code;
> +
> +static const char * const builtin_difftool_usage[] = {
> + N_("git add [<options>] [--] <pathspec>..."),
> + NULL
> +};
The usage should probably say "difftool" (or "builtin-difftool").
> [...]
> +static void changed_files(struct hashmap *result, const char *index_path,
> + const char *workdir)
> +{
> +[...]
> +}
> +
> +#include "dir.h"
Can this mid-file #include go to the top of the file?
> +static int run_dir_diff(const char *extcmd, int symlinks,
> + int argc, const char **argv)
> +{
> + char tmpdir[PATH_MAX];
> + struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> + struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
> + struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> + struct strbuf wtdir = STRBUF_INIT;
> + size_t ldir_len, rdir_len, wtdir_len;
> + struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
> + const char *workdir, *tmp;
> + int ret = 0, i;
> + FILE *fp;
> + struct hashmap working_tree_dups, submodules, symlinks2;
> + struct hashmap_iter iter;
> + struct pair_entry *entry;
> + enum object_type type;
> + unsigned long size;
> + struct index_state wtindex;
> + struct checkout lstate, rstate;
> + int rc, flags = RUN_GIT_CMD, err = 0;
> + struct child_process child = CHILD_PROCESS_INIT;
> + const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> + struct hashmap wt_modified, tmp_modified;
> + int indices_loaded = 0;
> +
> + setup_work_tree();
> + workdir = get_git_work_tree();
> +
> + /* Setup temp directories */
> + tmp = getenv("TMPDIR");
> + sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
Maybe snprintf instead?
getenv() won't return anything longer than PATH_MAX for most
users, but users are weird.
> + if (!mkdtemp(tmpdir))
> + return error("could not create temporary directory");
Mention the tmpdir here?
> + strbuf_addf(&ldir, "%s/left/", tmpdir);
> + strbuf_addf(&rdir, "%s/right/", tmpdir);
> + strbuf_addstr(&wtdir, workdir);
> + if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
> + strbuf_addch(&wtdir, '/');
> + mkdir(ldir.buf, 0777);
> + mkdir(rdir.buf, 0777);
Seeing the perl mkpath() default 0777 spelled out this way
makes me wonder whether 0700 would be safer.
The mkdtemp() above is already using 0700 so it's ok, but it
might be worth making it consistent (later, perhaps).
> [...]
> +int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
> +{
> + int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> + tool_help = 0;
> + static char *difftool_cmd = NULL, *extcmd = NULL;
> +
> + struct option builtin_difftool_options[] = {
> + OPT_BOOL('g', "gui", &use_gui_tool,
> + N_("use `diff.guitool` instead of `diff.tool`")),
> + OPT_BOOL('d', "dir-diff", &dir_diff,
> + N_("perform a full-directory diff")),
> + { OPTION_SET_INT, 'y', "no-prompt", &prompt, NULL,
> + N_("do not prompt before launching a diff tool"),
> + PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
> + { OPTION_SET_INT, 0, "prompt", &prompt, NULL, NULL,
> + PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
> + NULL, 1 },
> + OPT_BOOL(0, "symlinks", &symlinks,
> + N_("use symlinks in dir-diff mode")),
> + OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
> + N_("use the specified diff tool")),
> + OPT_BOOL(0, "tool-help", &tool_help,
> + N_("print a list of diff tools that may be used with "
> + "`--tool`")),
> + OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
> + N_("make 'git-difftool' exit when an invoked diff "
> + "tool returns a non - zero exit code")),
> + OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
> + N_("specify a custom command for viewing diffs")),
> + OPT_END()
> + };
> +
> + symlinks = has_symlinks;
> +
> + git_config(difftool_config, NULL);
> +
> + argc = parse_options(argc, argv, prefix, builtin_difftool_options,
> + builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
> + PARSE_OPT_KEEP_DASHDASH);
> +
> + if (tool_help)
> + return print_tool_help();
> +
> + if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
> + setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
> + else if (difftool_cmd) {
> + if (*difftool_cmd)
> + setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
> + else
> + die(_("no <tool> given for --tool=<tool>"));
> + }
> +
> + if (extcmd) {
> + if (*extcmd)
> + setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1);
> + else
> + die(_("no <cmd> given for --extcmd=<cmd>"));
> + }
> +
> + setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
> + trust_exit_code ? "true" : "false", 1);
> +
> + /*
> + * In directory diff mode, 'git-difftool--helper' is called once
> + * to compare the a / b directories.In file diff mode, 'git diff'
> + * will invoke a separate instance of 'git-difftool--helper' for
> + * each file that changed.
> + */
Missing space after "." in the comment above.
> + if (dir_diff)
> + return run_dir_diff(extcmd, symlinks, argc, argv);
> + return run_file_diff(prompt, argc, argv);
> +}
> diff --git a/git.c b/git.c
> index efa1059..eaa0f67 100644
> --- a/git.c
> +++ b/git.c
> @@ -424,6 +424,7 @@ static struct cmd_struct commands[] = {
> { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
> { "diff-index", cmd_diff_index, RUN_SETUP },
> { "diff-tree", cmd_diff_tree, RUN_SETUP },
> + { "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
> { "fast-export", cmd_fast_export, RUN_SETUP },
> { "fetch", cmd_fetch, RUN_SETUP },
> { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
This isn't alphabetical anymore, but it actually is if you
consider that the final plan is to change "builtin-difftool" to
"difftool".
If we want to minimize that future diff we could name
cmd_builtin_difftool() as cmd_difftool() for consistency now so
that the future commit only needs to tweak the string here.
--
David
^ permalink raw reply
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Duy Nguyen @ 2016-11-23 9:38 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Brandon Williams, Git Mailing List
In-Reply-To: <CAGZ79kYm4LfXK=1j-ayLawt+BojnkyM4h2RLQ=kfpPgMQbdBag@mail.gmail.com>
On Wed, Nov 23, 2016 at 12:26 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Nov 22, 2016 at 2:41 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller <sbeller@google.com> wrote:
>>> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
>>> PATHSPEC_LITERAL |
>>> PATHSPEC_GLOB |
>>> PATHSPEC_ICASE |
>>> - PATHSPEC_EXCLUDE);
>>> + PATHSPEC_EXCLUDE |
>>> + PATHSPEC_ATTR);
>>
>> Hmm.. common_prefix_len() has always been a bit relaxing and can cover
>> more than needed. It's for early pruning. Exact pathspec matching
>> _will_ be done later anyway.
>>
>> Is that obvious?
>
> Yes it is.
> Not sure what your concern is, though.
None really. I was just thinking out loud and trying not to make
assumptions, because I know this code quite well and I don't know how
people see this code anymore :D So all is good then.
--
Duy
^ 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