* [PATCH v2] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 0:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, bburky, peff, jrnieder
In-Reply-To: <1478125247-62372-1-git-send-email-bmwill@google.com>
Add configuration option 'core.allowProtocol' to allow users to create a
whitelist of allowed protocols for fetch/push/clone in their gitconfig.
For git-submodule.sh, fallback to default whitelist only if the user
hasn't explicitly set `GIT_ALLOW_PROTOCOL` or doesn't have a whitelist
in their gitconfig.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/config.txt | 8 ++++++++
Documentation/git.txt | 6 ++++--
git-submodule.sh | 3 ++-
t/lib-proto-disable.sh | 27 +++++++++++++++++++++++++++
t/t5815-submodule-protos.sh | 22 ++++++++++++++++++++++
transport.c | 6 ++++++
6 files changed, 69 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..78c97e3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -455,6 +455,14 @@ core.sshCommand::
the `GIT_SSH_COMMAND` environment variable and is overridden
when the environment variable is set.
+core.allowProtocol::
+ Provide a colon-separated list of protocols which are allowed to be
+ used with fetch/push/clone. Any protocol not mentioned will be
+ disallowed (i.e., this is a whitelist, not a blacklist). If the
+ `GIT_ALLOW_PROTOCOL` environment variable is set, it is used as the
+ protocol whitelist instead of this config option. If neither is set,
+ all protocols are enabled. See git(1) for more details.
+
core.ignoreStat::
If true, Git will avoid using lstat() calls to detect if files have
changed by setting the "assume-unchanged" bit for those tracked files
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..a86ec16 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1155,8 +1155,10 @@ of clones and fetches.
restrict recursive submodule initialization from an untrusted
repository. Any protocol not mentioned will be disallowed (i.e.,
this is a whitelist, not a blacklist). If the variable is not
- set at all, all protocols are enabled. The protocol names
- currently used by git are:
+ set at all, all protocols are enabled. The exception to this is when
+ running `git-submodule` which will use a default list of known-safe
+ protocols (file:git:http:https:ssh) in the event no whitelist is
+ provided. The protocol names currently used by git are:
- `file`: any local file-based path (including `file://` URLs,
or local paths)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..bfbfb8e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -27,7 +27,8 @@ cd_to_toplevel
#
# If the user has already specified a set of allowed protocols,
# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
+whitelist=$(git config core.allowProtocol) || whitelist=file:git:http:https:ssh
+: ${GIT_ALLOW_PROTOCOL=$whitelist}
export GIT_ALLOW_PROTOCOL
command=
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..e8820a6 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -62,6 +62,33 @@ test_proto () {
test_must_fail git clone --bare "$url" tmp.git
)
'
+
+ # Run tests again using the gitconfig method for setting a whitelist
+ test_expect_success "clone $1 (enabled)" '
+ rm -rf tmp.git &&
+ git -c core.allowProtocol="$proto" clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $1 (enabled)" '
+ git -C tmp.git -c core.allowProtocol="$proto" fetch
+ '
+
+ test_expect_success "push $1 (enabled)" '
+ git -C tmp.git -c core.allowProtocol="$proto" push origin HEAD:pushed
+ '
+
+ test_expect_success "push $1 (disabled)" '
+ test_must_fail git -C tmp.git -c core.allowProtocol=none push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $1 (disabled)" '
+ test_must_fail git -C tmp.git -c core.allowProtocol=none fetch
+ '
+
+ test_expect_success "clone $1 (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git -C tmp.git -c core.allowProtocol=none clone --bare "$url" tmp.git
+ '
}
# set up an ssh wrapper that will access $host/$repo in the
diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh
index 06f55a1..f85e71c 100755
--- a/t/t5815-submodule-protos.sh
+++ b/t/t5815-submodule-protos.sh
@@ -40,4 +40,26 @@ test_expect_success 'user can override whitelist' '
GIT_ALLOW_PROTOCOL=ext git -C dst submodule update ext-module
'
+test_expect_success 'reset dst repo for config tests' '
+ rm -rf dst &&
+ git clone . dst &&
+ git -C dst submodule init
+'
+
+test_expect_success 'update of ssh not allowed when not in config whitelist' '
+ test_must_fail git -C dst -c core.allowProtocol=none submodule update ssh-module
+'
+
+test_expect_success 'update of ssh allowed via config whitelist' '
+ git -C dst -c core.allowProtocol="ssh:http:https" submodule update ssh-module
+'
+
+test_expect_success 'update of ext not allowed' '
+ test_must_fail git -C dst -c core.allowProtocol=ssh submodule update ext-module
+'
+
+test_expect_success 'user can override whitelist' '
+ git -C dst -c core.allowProtocol=ext submodule update ext-module
+'
+
test_done
diff --git a/transport.c b/transport.c
index d57e8de..e3d8e88 100644
--- a/transport.c
+++ b/transport.c
@@ -652,10 +652,16 @@ static const struct string_list *protocol_whitelist(void)
if (enabled < 0) {
const char *v = getenv("GIT_ALLOW_PROTOCOL");
+ char *w = NULL;
if (v) {
string_list_split(&allowed, v, ':', -1);
string_list_sort(&allowed);
enabled = 1;
+ } else if (!git_config_get_string("core.allowProtocol", &w)) {
+ string_list_split(&allowed, w, ':', -1);
+ string_list_sort(&allowed);
+ enabled = 1;
+ free(w);
} else {
enabled = 0;
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Blake Burkhart @ 2016-11-03 0:41 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Williams, git, Stefan Beller, Jeff King
In-Reply-To: <20161103002225.GA13369@google.com>
Thanks for CCing me.
I haven't looked at this implementation in detail, but it would be
good to move this configuration into the config system because I think
we can more easily provide a default safe configuration.
It would be nice to use this to introduce a default list of
whitelisted protocols that even applies to `git clone`. I strongly
think we need to find a way to have git-remote-ext disabled by
default. This could be a way to do it.
On Wed, Nov 2, 2016 at 7:22 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> That reminds me: external tools also set GIT_ALLOW_PROTOCOL when the
> user hasn't set it explicitly, like git-submodule.sh does. E.g.
> repo <https://gerrit.googlesource.com/git-repo/+/466b8c4e/git_command.py#171>,
> mercurial <https://www.mercurial-scm.org/repo/hg/file/b032a7b676c6/mercurial/subrepo.py#l1404>.
> Other external tools consume GIT_ALLOW_PROTOCOL, like 'go get'
> <https://go.googlesource.com/go/+/55620a0e/src/cmd/go/vcs.go#64>.
> Can we make it more convenient for them to support this configuration
> too?
Most of these are my fault too. I encouraged git-repo and mercurial to
use GIT_ALLOW_PROTOCOL to avoid security issues from git-remote-ext.
--
Blake Burkhart
^ permalink raw reply
* Re: [PATCH v11 13/14] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-11-03 0:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <3b09d218-33bd-dc7c-235c-8954a46afc41@kdbg.org>
> On 2 Nov 2016, at 18:03, Johannes Sixt <j6t@kdbg.org> wrote:
>
>> Am 17.10.2016 um 01:20 schrieb larsxschneider@gmail.com:
>> +# Compare two files and ensure that `clean` and `smudge` respectively are
>> +# called at least once if specified in the `expect` file. The actual
>> +# invocation count is not relevant because their number can vary.
>> +# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
>> +test_cmp_count () {
>> + expect=$1
>> + actual=$2
>> + for FILE in "$expect" "$actual"
>> + do
>> + sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
>> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
>> + sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
>
> This is not sufficiently portable. Some versions of uniq write the
> count left-adjusted, not right-adjusted. How about this on top:
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index a20b9f58e3..f60858c517 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -40,10 +40,9 @@ test_cmp_count () {
> actual=$2
> for FILE in "$expect" "$actual"
> do
> - sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
> - sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
> - sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
> - mv "$FILE.tmp" "$FILE"
> + sort "$FILE" | uniq -c |
> + sed -e "s/^ *[0-9][0-9]* *IN: /x IN: /" >"$FILE.tmp" &&
This looks good (thanks for cleaning up the redundant clean/smudge stuff - that was a refactoring artifact!). One minor nit: doesn't sed understand '[0-9]+' ?
> + mv "$FILE.tmp" "$FILE" || return
Why '|| return' here?
> done &&
> test_cmp "$expect" "$actual"
> }
Thank you,
Lars
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jonathan Nieder @ 2016-11-03 0:22 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, Stefan Beller, Blake Burkhart, Jeff King
In-Reply-To: <1478125247-62372-1-git-send-email-bmwill@google.com>
(+peff and bburky, who introduced GIT_ALLOW_PROTOCOL)
Brandon Williams wrote:
> Add configuration option 'core.allowProtocol' to allow users to create a
> whitelist of allowed protocols for fetch/push/clone in their gitconfig.
Ooh.
This would be especially useful at $DAYJOB, where there is a custom
sso:// protocol that is often used by submodules. Using an envvar to
whitelist it globally is painful because
- it disables other protocols even when explicitly requested on a
plain "git clone" command line by the user. By comparison, the
built-in git-submodule.sh whitelist only applies to submodules.
- platform-specific instructions to set an environment variable can
be more difficult than "just set this git configuration"
Another difficulty with setting GIT_ALLOW_PROTOCOL globally is that it
requires copy/pasting the default value from upstream and then adding
the values I want. There's no straightforward way to get the current
value and add to it, in case I want to benefit from future upstream
fixes to the default list.
That is, would it be possible to use something like
[protocol "sso"]
allow = always
instead of
[core]
allowProtocol = file:git:http:https:....:sso
?
[...]
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -27,7 +27,8 @@ cd_to_toplevel
> #
> # If the user has already specified a set of allowed protocols,
> # we assume they know what they're doing and use that instead.
> -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> +config_whitelist=$(git config core.allowProtocol)
> +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
optional: To avoid config parsing when GIT_ALLOW_PROTOCOL is already
set, could do something like
if ! test "${GIT_ALLOW_PROTOCOL+set}"
then
GIT_ALLOW_PROTOCOL=$(
git config --name-only --get-regexp 'protocol\..*\.allow' always |
sed -e 's/^protocol.//' -e 's/.allow$//' |
tr '\n' ':'
)
GIT_ALLOW_PROTOCOL=${GIT_ALLOW_PROTOCOL%:}
: ${GIT_ALLOW_PROTOCOL:=file:git:http:https:ssh}
fi
[...]
> --- a/transport.c
> +++ b/transport.c
> @@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
>
> if (enabled < 0) {
> const char *v = getenv("GIT_ALLOW_PROTOCOL");
> - if (v) {
> + if (v || !git_config_get_value("core.allowProtocol", &v)) {
> string_list_split(&allowed, v, ':', -1);
This has the effect of always disabling other protocols when
core.allowProtocol is set. Is that intended?
Like the default list used by submodule, I'd be happiest if this only
applied to repositories cloned implicitly instead of those passed
directly to 'git clone'.
That reminds me: external tools also set GIT_ALLOW_PROTOCOL when the
user hasn't set it explicitly, like git-submodule.sh does. E.g.
repo <https://gerrit.googlesource.com/git-repo/+/466b8c4e/git_command.py#171>,
mercurial <https://www.mercurial-scm.org/repo/hg/file/b032a7b676c6/mercurial/subrepo.py#l1404>.
Other external tools consume GIT_ALLOW_PROTOCOL, like 'go get'
<https://go.googlesource.com/go/+/55620a0e/src/cmd/go/vcs.go#64>.
Can we make it more convenient for them to support this configuration
too?
An example approach would be a GIT_ALLOW_PROTOCOL var returned by
"git var".
That way git-submodule.sh could do
: ${GIT_ALLOW_PROTOCOL=$(git var GIT_ALLOW_PROTOCOL)}
and it would just work. Other tools could do the same, with a
fallback to the current default until new enough git is in widespread
use.
Thanks and hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 0:08 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <20161102234613.GD183367@google.com>
On Wed, Nov 02, 2016 at 04:46:13PM -0700, Brandon Williams wrote:
> > > I thought at first we'd have to deal with leaking "v", but "get_value"
> > > is the "raw" version that gives you the uninterpreted value. I think
> > > that means it may give you NULL, though if we see an implicit bool like:
> > >
> > > [core]
> > > allowProtocol
> > >
> > > That's nonsense, of course, but we would still segfault. I
> > > think the easiest way to test is:
> > >
> > > git -c core.allowProtocol fetch
> > >
> > > which seems to segfault for me with this patch.
> >
> > what is the desired behavior when a user provides a config in a way that
> > isn't intended?
>
> oh...I can just drop in git_config_get_string_const() instead.
Yes, it will call git_config_string(), which will make sure there's an
actual value and die otherwise. But note that it also duplicates the
string, so you'd have to deal with freeing it.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-02 23:46 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller
In-Reply-To: <20161102233304.GB183367@google.com>
> > > diff --git a/transport.c b/transport.c
> > > index d57e8de..b1098cd 100644
> > > --- a/transport.c
> > > +++ b/transport.c
> > > @@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
> > >
> > > if (enabled < 0) {
> > > const char *v = getenv("GIT_ALLOW_PROTOCOL");
> > > - if (v) {
> > > + if (v || !git_config_get_value("core.allowProtocol", &v)) {
> > > string_list_split(&allowed, v, ':', -1);
> > > string_list_sort(&allowed);
> > > enabled = 1;
> >
> > I thought at first we'd have to deal with leaking "v", but "get_value"
> > is the "raw" version that gives you the uninterpreted value. I think
> > that means it may give you NULL, though if we see an implicit bool like:
> >
> > [core]
> > allowProtocol
> >
> > That's nonsense, of course, but we would still segfault. I
> > think the easiest way to test is:
> >
> > git -c core.allowProtocol fetch
> >
> > which seems to segfault for me with this patch.
>
> what is the desired behavior when a user provides a config in a way that
> isn't intended?
oh...I can just drop in git_config_get_string_const() instead.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-02 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller
In-Reply-To: <20161102230853.3xdk6ksnzzote5ud@sigill.intra.peff.net>
On 11/02, Jeff King wrote:
> On Wed, Nov 02, 2016 at 07:05:39PM -0400, Jeff King wrote:
>
> > > +core.allowProtocol::
> > > + Provide a colon-separated list of protocols which are allowed to be
> > > + used with fetch/push/clone. This is useful to restrict recursive
> > > + submodule initialization from an untrusted repository. Any protocol not
> > > + mentioned will be disallowed (i.e., this is a whitelist, not a
> > > + blacklist). If the variable is not set at all, all protocols are
> > > + enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
> > > + used as the protocol whitelist instead of this config option.
> >
> > The "not set at all, all protocols are enabled" bit is not quite
> > correct, is it? It is true for a top-level fetch, but not for submodule
> > recursion (and especially since you are talking about submodule
> > recursion immediately before, it is rather confusing).
>
> Heh, just saw that you copied this straight from the discussion of
> GIT_ALLOW_PROTOCOL. What idiot wrote the original? :)
>
> It might be worth fixing both places (or possibly just fixing the
> original and phrasing this one as "If GIT_ALLOW_PROTOCOL is not set, use
> this as the default value; see git(1) for details").
>
> -Peff
haha K I'll fix the original as well.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-02 23:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller
In-Reply-To: <20161102230538.jx3jwa4hqgrrltno@sigill.intra.peff.net>
On 11/02, Jeff King wrote:
> On Wed, Nov 02, 2016 at 03:20:47PM -0700, Brandon Williams wrote:
>
> > Add configuration option 'core.allowProtocol' to allow users to create a
> > whitelist of allowed protocols for fetch/push/clone in their gitconfig.
> >
> > For git-submodule.sh, fallback to default whitelist only if the user
> > hasn't explicitly set `GIT_ALLOW_PROTOCOL` or doesn't have a whitelist
> > in their gitconfig.
>
> This says "what", but not "why". What's the use case?
>
> I can see somebody wanting to pare down the whitelist further (e.g.,
> because they are carrying ssh credentials that they don't want to use on
> behalf of a malicious repo). But in general I'd expect this setting to
> be a function of the environment you're operating in, and not the
> on-disk config.
>
> Or is the intent to broaden it for cases where you have a clone that
> uses some non-standard protocol, and you want it to Just Work on
> subsequent recursive fetches?
>
> > +core.allowProtocol::
> > + Provide a colon-separated list of protocols which are allowed to be
> > + used with fetch/push/clone. This is useful to restrict recursive
> > + submodule initialization from an untrusted repository. Any protocol not
> > + mentioned will be disallowed (i.e., this is a whitelist, not a
> > + blacklist). If the variable is not set at all, all protocols are
> > + enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
> > + used as the protocol whitelist instead of this config option.
>
> The "not set at all, all protocols are enabled" bit is not quite
> correct, is it? It is true for a top-level fetch, but not for submodule
> recursion (and especially since you are talking about submodule
> recursion immediately before, it is rather confusing).
Yeah stefan mentioned this to me. I simply copied the documentaion from
GIT_ALLOW_PROTOCOL, perhaps that should be updated as well?
>
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -27,7 +27,8 @@ cd_to_toplevel
> > #
> > # If the user has already specified a set of allowed protocols,
> > # we assume they know what they're doing and use that instead.
> > -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> > +config_whitelist=$(git config core.allowProtocol)
> > +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
>
> The original uses "=" without a ":" so that an empty variable takes
> precedence over the stock list (i.e., allowing nothing). Would you want
> the same behavior for the config variable? I.e.:
>
> # this should probably allow nothing, right?
> git config core.allowProtocol ""
>
> I think you'd have to check the return code of "git config" to
> distinguish those cases.
Oh, I didn't think of that case. That can be done easy enough, just
makes the code a bit more verbose.
>
> > diff --git a/transport.c b/transport.c
> > index d57e8de..b1098cd 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
> >
> > if (enabled < 0) {
> > const char *v = getenv("GIT_ALLOW_PROTOCOL");
> > - if (v) {
> > + if (v || !git_config_get_value("core.allowProtocol", &v)) {
> > string_list_split(&allowed, v, ':', -1);
> > string_list_sort(&allowed);
> > enabled = 1;
>
> I thought at first we'd have to deal with leaking "v", but "get_value"
> is the "raw" version that gives you the uninterpreted value. I think
> that means it may give you NULL, though if we see an implicit bool like:
>
> [core]
> allowProtocol
>
> That's nonsense, of course, but we would still segfault. I
> think the easiest way to test is:
>
> git -c core.allowProtocol fetch
>
> which seems to segfault for me with this patch.
what is the desired behavior when a user provides a config in a way that
isn't intended?
--
Brandon Williams
^ permalink raw reply
* [PATCH 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-02 23:17 UTC (permalink / raw)
To: gitster, bmwill; +Cc: git, Stefan Beller
In-Reply-To: <20161102231722.15787-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 | 4 +-
submodule-config.c | 47 ++++++++++++------------
submodule-config.h | 4 +-
t/t7411-submodule-config.sh | 11 ++++++
4 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..81921e477b 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,11 +47,11 @@ 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.
-`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.
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.10.2.621.g399b625.dirty
^ permalink raw reply related
* [PATCH 3/3] submodule-config: clarify parsing of null_sha1 element
From: Stefan Beller @ 2016-11-02 23:17 UTC (permalink / raw)
To: gitster, bmwill; +Cc: git, Stefan Beller
In-Reply-To: <20161102231722.15787-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.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 81921e477b..1df7a827ff 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_sha1 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.10.2.621.g399b625.dirty
^ permalink raw reply related
* [PATCH 1/3] submodule config: inline config_from_{name, path}
From: Stefan Beller @ 2016-11-02 23:17 UTC (permalink / raw)
To: gitster, bmwill; +Cc: git, Stefan Beller
In-Reply-To: <20161102231722.15787-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>
---
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.10.2.621.g399b625.dirty
^ permalink raw reply related
* [PATCH 0/3] submodule-config: clarify/cleanup docs and header
From: Stefan Beller @ 2016-11-02 23:17 UTC (permalink / raw)
To: gitster, bmwill; +Cc: git, Stefan Beller
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 | 11 +++--
submodule-config.c | 59 ++++++++++--------------
submodule-config.h | 4 +-
t/t7411-submodule-config.sh | 11 +++++
4 files changed, 44 insertions(+), 41 deletions(-)
--
2.10.2.621.g399b625.dirty
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-02 23:08 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <20161102230538.jx3jwa4hqgrrltno@sigill.intra.peff.net>
On Wed, Nov 02, 2016 at 07:05:39PM -0400, Jeff King wrote:
> > +core.allowProtocol::
> > + Provide a colon-separated list of protocols which are allowed to be
> > + used with fetch/push/clone. This is useful to restrict recursive
> > + submodule initialization from an untrusted repository. Any protocol not
> > + mentioned will be disallowed (i.e., this is a whitelist, not a
> > + blacklist). If the variable is not set at all, all protocols are
> > + enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
> > + used as the protocol whitelist instead of this config option.
>
> The "not set at all, all protocols are enabled" bit is not quite
> correct, is it? It is true for a top-level fetch, but not for submodule
> recursion (and especially since you are talking about submodule
> recursion immediately before, it is rather confusing).
Heh, just saw that you copied this straight from the discussion of
GIT_ALLOW_PROTOCOL. What idiot wrote the original? :)
It might be worth fixing both places (or possibly just fixing the
original and phrasing this one as "If GIT_ALLOW_PROTOCOL is not set, use
this as the default value; see git(1) for details").
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-02 23:05 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <1478125247-62372-1-git-send-email-bmwill@google.com>
On Wed, Nov 02, 2016 at 03:20:47PM -0700, Brandon Williams wrote:
> Add configuration option 'core.allowProtocol' to allow users to create a
> whitelist of allowed protocols for fetch/push/clone in their gitconfig.
>
> For git-submodule.sh, fallback to default whitelist only if the user
> hasn't explicitly set `GIT_ALLOW_PROTOCOL` or doesn't have a whitelist
> in their gitconfig.
This says "what", but not "why". What's the use case?
I can see somebody wanting to pare down the whitelist further (e.g.,
because they are carrying ssh credentials that they don't want to use on
behalf of a malicious repo). But in general I'd expect this setting to
be a function of the environment you're operating in, and not the
on-disk config.
Or is the intent to broaden it for cases where you have a clone that
uses some non-standard protocol, and you want it to Just Work on
subsequent recursive fetches?
> +core.allowProtocol::
> + Provide a colon-separated list of protocols which are allowed to be
> + used with fetch/push/clone. This is useful to restrict recursive
> + submodule initialization from an untrusted repository. Any protocol not
> + mentioned will be disallowed (i.e., this is a whitelist, not a
> + blacklist). If the variable is not set at all, all protocols are
> + enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
> + used as the protocol whitelist instead of this config option.
The "not set at all, all protocols are enabled" bit is not quite
correct, is it? It is true for a top-level fetch, but not for submodule
recursion (and especially since you are talking about submodule
recursion immediately before, it is rather confusing).
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -27,7 +27,8 @@ cd_to_toplevel
> #
> # If the user has already specified a set of allowed protocols,
> # we assume they know what they're doing and use that instead.
> -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> +config_whitelist=$(git config core.allowProtocol)
> +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
The original uses "=" without a ":" so that an empty variable takes
precedence over the stock list (i.e., allowing nothing). Would you want
the same behavior for the config variable? I.e.:
# this should probably allow nothing, right?
git config core.allowProtocol ""
I think you'd have to check the return code of "git config" to
distinguish those cases.
> diff --git a/transport.c b/transport.c
> index d57e8de..b1098cd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
>
> if (enabled < 0) {
> const char *v = getenv("GIT_ALLOW_PROTOCOL");
> - if (v) {
> + if (v || !git_config_get_value("core.allowProtocol", &v)) {
> string_list_split(&allowed, v, ':', -1);
> string_list_sort(&allowed);
> enabled = 1;
I thought at first we'd have to deal with leaking "v", but "get_value"
is the "raw" version that gives you the uninterpreted value. I think
that means it may give you NULL, though if we see an implicit bool like:
[core]
allowProtocol
That's nonsense, of course, but we would still segfault. I
think the easiest way to test is:
git -c core.allowProtocol fetch
which seems to segfault for me with this patch.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-02 22:47 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kbytopD1UgvU6R0X18Ge6JsJ46K2KQ-SFLh+hu5tprTmA@mail.gmail.com>
On 11/02, Stefan Beller wrote:
> > This is useful to restrict recursive
> > + submodule initialization from an untrusted repository.
>
> ok. Though as a user submodules may not spring to mind immediately here.
> I think this is generally useful, too. e.g. an admin could put this in
> the system wide
> config to prevent certain protocols from being used.
Oh I pretty much copied the description from what exists for
`GIT_ALLOW_PROTOCOL` which included this bit about submodules.
> > Any protocol not
> > + mentioned will be disallowed
>
> For the regular fetch/clone/pull case. For the submodule case we still
> fall back to
> the hardcoded list of known good things?
Yep! This is done by explicitly setting GIT_ALLOW_PROTOCOL to the
hardcoded list if the user hasn't supplied a whitelist.
>
> > (i.e., this is a whitelist, not a
> > + blacklist).
>
> That is very explicit, I'd drop it. However this inspires bike
> shedding on the name:
> What about core.protocolWhitelist instead?
Simply to keep the name similar to the env variable that already exists
for this functionality.
> So the env var is of higher priority than this config.
> > -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> > +config_whitelist=$(git config core.allowProtocol)
>
> So first we lookup the configured protocols.
>
> > +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
>
> Then if they are not configured use the current hard coded white list.
The lookup of the configured whitelist is done first but wont be used
unless GIT_ALLOW_PROTOCOL is unset. If neither is set it will fallback
to the hardcoded list.
> Do we have any tests for this that could be extended? (Otherwise we'd
> maybe want to add a test for both the regular case as well as a forbidden
> submodule?)
>
I can write a couple tests for a v2 of the patch.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Stefan Beller @ 2016-11-02 22:41 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <1478125247-62372-1-git-send-email-bmwill@google.com>
On Wed, Nov 2, 2016 at 3:20 PM, Brandon Williams <bmwill@google.com> wrote:
> Add configuration option 'core.allowProtocol' to allow users to create a
> whitelist of allowed protocols for fetch/push/clone in their gitconfig.
>
> For git-submodule.sh, fallback to default whitelist only if the user
> hasn't explicitly set `GIT_ALLOW_PROTOCOL` or doesn't have a whitelist
> in their gitconfig.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/config.txt | 9 +++++++++
> git-submodule.sh | 3 ++-
> transport.c | 2 +-
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 27069ac..7f83e40 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -455,6 +455,15 @@ core.sshCommand::
> the `GIT_SSH_COMMAND` environment variable and is overridden
> when the environment variable is set.
>
> +core.allowProtocol::
> + Provide a colon-separated list of protocols which are allowed to be
> + used with fetch/push/clone.
ok.
> This is useful to restrict recursive
> + submodule initialization from an untrusted repository.
ok. Though as a user submodules may not spring to mind immediately here.
I think this is generally useful, too. e.g. an admin could put this in
the system wide
config to prevent certain protocols from being used.
> Any protocol not
> + mentioned will be disallowed
For the regular fetch/clone/pull case. For the submodule case we still
fall back to
the hardcoded list of known good things?
> (i.e., this is a whitelist, not a
> + blacklist).
That is very explicit, I'd drop it. However this inspires bike
shedding on the name:
What about core.protocolWhitelist instead?
> If the variable is not set at all, all protocols are
> + enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
> + used as the protocol whitelist instead of this config option.
So the env var is of higher priority than this config.
> +
> core.ignoreStat::
> If true, Git will avoid using lstat() calls to detect if files have
> changed by setting the "assume-unchanged" bit for those tracked files
> diff --git a/git-submodule.sh b/git-submodule.sh
> index a024a13..ad94c75 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -27,7 +27,8 @@ cd_to_toplevel
> #
> # If the user has already specified a set of allowed protocols,
> # we assume they know what they're doing and use that instead.
> -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> +config_whitelist=$(git config core.allowProtocol)
So first we lookup the configured protocols.
> +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
Then if they are not configured use the current hard coded white list.
> export GIT_ALLOW_PROTOCOL
>
> command=
> diff --git a/transport.c b/transport.c
> index d57e8de..b1098cd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
>
> if (enabled < 0) {
> const char *v = getenv("GIT_ALLOW_PROTOCOL");
> - if (v) {
> + if (v || !git_config_get_value("core.allowProtocol", &v)) {
This implementation matches what the config promised, I would think.
Do we have any tests for this that could be extended? (Otherwise we'd
maybe want to add a test for both the regular case as well as a forbidden
submodule?)
> string_list_split(&allowed, v, ':', -1);
> string_list_sort(&allowed);
> enabled = 1;
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: RFE: Discard hunks during `git add -p`
From: Jeff King @ 2016-11-02 22:37 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
In-Reply-To: <20161102221113.peur2zyfs66bdchm@sigill.intra.peff.net>
On Wed, Nov 02, 2016 at 06:11:14PM -0400, Jeff King wrote:
> > Being able to discard hunks (reset working copy to index contents)
> > during add-p would alleviate the (quite broad) hard reset.
>
> As Konstantin pointed out, you can already discard interactively with
> "git checkout -p". It might be nice to be able to do both in the same
> run, and turn the "yes/no" decision into "yes/no/discard".
>
> In theory it should be easy, as the same code drives the hunk selector
> for both commands. It's just a matter of which command we feed the
> selected hunks to. I don't know if there would be corner cases around
> hunk-editing and splitting, though. The "add" phase should never touch
> the working tree file itself, so any hunks present from the initial list
> should still apply cleanly during the "discard" phase.
The patch is something like the one below, which worked for me in a very
trivial test. I won't be surprised if there are some corner cases it's
missing. At the very least, coalesce_overlapping_hunks() needs to learn
about the differences between "apply" and "discard" hunks (and not
coalesce them!).
I don't have immediate plans for this, so if somebody wants to pick it
up and run with it, be my guest.
-Peff
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d81269..43651435a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -109,6 +109,7 @@ my %patch_modes = (
PARTICIPLE => 'staging',
FILTER => 'file-only',
IS_REVERSE => 0,
+ DISCARD => sub { apply_patch 'apply -R', @_; },
},
'stash' => {
DIFF => 'diff-index -p HEAD',
@@ -1325,6 +1326,11 @@ sub patch_update_file {
my ($prev, $next, $other, $undecided, $i);
$other = '';
+ my $discard = exists $patch_mode_flavour{DISCARD};
+ if ($discard) {
+ $other .= ',D';
+ }
+
if ($num <= $ix) {
$ix = 0;
}
@@ -1384,6 +1390,9 @@ sub patch_update_file {
elsif ($line =~ /^n/i) {
$hunk[$ix]{USE} = 0;
}
+ elsif ($discard && $line =~ /^D/) {
+ $hunk[$ix]{USE} = -1;
+ }
elsif ($line =~ /^a/i) {
while ($ix < $num) {
if (!defined $hunk[$ix]{USE}) {
@@ -1539,9 +1548,12 @@ sub patch_update_file {
my $n_lofs = 0;
my @result = ();
+ my @discard = ();
for (@hunk) {
- if ($_->{USE}) {
+ if ($_->{USE} > 0) {
push @result, @{$_->{TEXT}};
+ } elsif ($_->{USE} < 0) {
+ push @discard, @{$_->{TEXT}};
}
}
@@ -1552,6 +1564,13 @@ sub patch_update_file {
refresh();
}
+ if (@discard) {
+ my @patch = reassemble_patch($head->{TEXT}, @discard);
+ my $apply_routine = $patch_mode_flavour{DISCARD};
+ &$apply_routine(@patch);
+ refresh();
+ }
+
print "\n";
return $quit;
}
^ permalink raw reply related
* Re: send-email garbled header with trailing doublequote in email
From: Andrea Arcangeli @ 2016-11-02 22:29 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161102220437.5ewzezxs2nasyouv@sigill.intra.peff.net>
On Wed, Nov 02, 2016 at 06:04:37PM -0400, Jeff King wrote:
> Nope, it looks exactly as --dry-run reports it.
My sendmail is postfix 3.1.0.
> To see exactly what is being sent out, try:
>
> -- >8 --
>
> cat >/tmp/foo <<\EOF
> #!/bin/sh
> echo "args: $*"
> sed 's/^/stdin: /'
> EOF
>
> chmod +x /tmp/foo
>
> git send-email --smtp-server=/tmp/foo --to=whatever
>
> -- 8< --
Right it's the same as --dry-run:
stdin: To: "what ever" " <.....>
There's not my hostname and not removed space. If I add more addresses
they also go in the second line with a leading space and they're not cut.
So this must be postfix then that out of the blue decided to garble it
in a strange way while parsing the input... The removal of all
whitespaces s/what ever/whatever/ especially I've no idea how it
decided to do so.
Can you reproduce with postfix as sendmail at least? If you can
reproduce also see what happens if you add another --to.
^ permalink raw reply
* [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-02 22:20 UTC (permalink / raw)
To: git; +Cc: sbeller, Brandon Williams
Add configuration option 'core.allowProtocol' to allow users to create a
whitelist of allowed protocols for fetch/push/clone in their gitconfig.
For git-submodule.sh, fallback to default whitelist only if the user
hasn't explicitly set `GIT_ALLOW_PROTOCOL` or doesn't have a whitelist
in their gitconfig.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/config.txt | 9 +++++++++
git-submodule.sh | 3 ++-
transport.c | 2 +-
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..7f83e40 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -455,6 +455,15 @@ core.sshCommand::
the `GIT_SSH_COMMAND` environment variable and is overridden
when the environment variable is set.
+core.allowProtocol::
+ Provide a colon-separated list of protocols which are allowed to be
+ used with fetch/push/clone. This is useful to restrict recursive
+ submodule initialization from an untrusted repository. Any protocol not
+ mentioned will be disallowed (i.e., this is a whitelist, not a
+ blacklist). If the variable is not set at all, all protocols are
+ enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
+ used as the protocol whitelist instead of this config option.
+
core.ignoreStat::
If true, Git will avoid using lstat() calls to detect if files have
changed by setting the "assume-unchanged" bit for those tracked files
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..ad94c75 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -27,7 +27,8 @@ cd_to_toplevel
#
# If the user has already specified a set of allowed protocols,
# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
+config_whitelist=$(git config core.allowProtocol)
+: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
export GIT_ALLOW_PROTOCOL
command=
diff --git a/transport.c b/transport.c
index d57e8de..b1098cd 100644
--- a/transport.c
+++ b/transport.c
@@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
if (enabled < 0) {
const char *v = getenv("GIT_ALLOW_PROTOCOL");
- if (v) {
+ if (v || !git_config_get_value("core.allowProtocol", &v)) {
string_list_split(&allowed, v, ':', -1);
string_list_sort(&allowed);
enabled = 1;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: RFE: Discard hunks during `git add -p`
From: Jeff King @ 2016-11-02 22:11 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
In-Reply-To: <alpine.LSU.2.20.1611021435280.21207@nerf40.vanv.qr>
On Wed, Nov 02, 2016 at 02:46:04PM +0100, Jan Engelhardt wrote:
> Current version: 2.10.2
> Example workflow:
>
> * I would do a global substitution across a source tree, e.g. `perl -i
> -pe 's{OLD_FOO\(x\)}{NEW_BAR(x, 0)}' *.c`
> * Using `git add -p`, I would verify each of the substitutions that they
> make sense in their respective locations, and, based on that,
> answer "y" or "n" to the interactive prompting to stage good hunks.
> * When done with add-p, I would commit the so-staged hunks,
> and then use `git reset --hard` to discard all changes that were
> not acknowledged during add-p.
>
> Being able to discard hunks (reset working copy to index contents)
> during add-p would alleviate the (quite broad) hard reset.
As Konstantin pointed out, you can already discard interactively with
"git checkout -p". It might be nice to be able to do both in the same
run, and turn the "yes/no" decision into "yes/no/discard".
In theory it should be easy, as the same code drives the hunk selector
for both commands. It's just a matter of which command we feed the
selected hunks to. I don't know if there would be corner cases around
hunk-editing and splitting, though. The "add" phase should never touch
the working tree file itself, so any hunks present from the initial list
should still apply cleanly during the "discard" phase.
-Peff
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Jeff King @ 2016-11-02 22:04 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: git
In-Reply-To: <20161102213805.GJ4611@redhat.com>
On Wed, Nov 02, 2016 at 10:38:05PM +0100, Andrea Arcangeli wrote:
> > But in either case, in my test, the actual email address is still
> > extracted correctly and fed to the MTA, so the mail is delivered.
>
> The email is delivered to the right email for me too, but to see the
> problem you need to check the email itself, and look how the To: field
> looks in the actual email in your mail client or web interface.
>
> Don't you see whatever@yourhostname without spaces and @yourhostname
> instead of the email domain?
Nope, it looks exactly as --dry-run reports it.
To see exactly what is being sent out, try:
-- >8 --
cat >/tmp/foo <<\EOF
#!/bin/sh
echo "args: $*"
sed 's/^/stdin: /'
EOF
chmod +x /tmp/foo
git send-email --smtp-server=/tmp/foo --to=whatever
-- 8< --
Mine shows the same header as --dry-run. Which makes sense, because the
code is literally just dumping the $header variable, which is the same
thing that gets sent to the sendmail binary (or the SMTP server if that
is in use).
So my guess would be that either an MTA in the routing path is garbling
it (or possibly mailing list software). or maybe even the eventual MUA,
though it sounds like you checked the raw bytes.
> If you still can't reproduce, maybe it's a different perl
> Mail::Address, I'm using dev-perl/MailTools-2.140.0
Mine is 2.13, which I would imagine is comparable.
> If --dry-run complained and failed instead of passing and then sending
> an email with garbled To/Cc list, it'd be more than enough. Either
> that or it should generate a mail header that matches the output of
> --dry-run so the review of --dry-run becomes meaningful again.
OK. I think we get that first part right. The problem is that the
garbling is such that somebody in the middle is unhappy with it (which
makes sense; it's syntactically bogus). So the tool is there to see the
problem in --dry-run, but of course it's rather subtle.
In theory we should be able to detect and complain about bogus syntax
like this, but right now we don't re-parse the addresses at all. We rely
on Mail::Address to produce valid output, and it doesn't seem to be
doing so here.
-Peff
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Michael Haggerty @ 2016-11-02 21:44 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqqzilinanp.fsf@gitster.mtv.corp.google.com>
On 11/01/2016 10:38 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If it involves renaming and swapping, however, we may be talking
>> about a change that we cannot deliver with confidence in less than a
>> week before -rc1, which sounds, eh, suboptimal.
>>
>> FWIW, here is how the removal of compaction without renaming looks
>> like.
>
> And here is _with_ renaming. I think "compaction heuristics" is a
> much better phrase to call "heuristics used during the diff hunk
> compaction process" without specifying how that heuristics work
> (like taking hints in the indentation levels). If we are to retire
> one while keeping the other, compaction-heuristics should be the
> name we give and keep for the surviving one.
Personally, I'm not a fan of the name "compaction heuristics".
The name *seems* to make sense because it affects the behavior of a
function called `xdl_change_compact()`. But that means nothing to end
users. The option doesn't affect how the diff is "compacted" in the
usual sense of the word; the slider optimization usually doesn't change
the number of lines in the diff at all [1].
Moreover, if we ever want to add another heuristic (for example, imagine
a language-aware algorithm that is based on parsing the file), we would
want a different name for that option, at least temporarily. From that
point of view, it makes a little bit of sense for the name of the option
to hint at the particular heuristic being used.
That being said, I don't think it is a big deal either way, and I can
live with either name. I rather hope that this option will become the
default soon, and hopefully after that hardly anybody will need to learn
its name.
Regarding making it the default: given that it is presumably too late in
this release cycle to make this option the default behavior, I suggest
that it be made the default early in the next release cycle so that it
gets a lot of testing, and people have enough time to voice any objections.
Regarding your patches themselves, once the old compaction heuristic is
removed (with or without renaming), you can also get rid of the
`blank_lines` local variable in `xdl_change_compact()`, and also the
function `is_blank_line()` in the same source file.
Michael
[1] The only exceptions are when it causes context lines for adjacent
diff hunks to join/split, or makes the context lines for a diff hunk
extend past the beginning/end of the file. But these are uninteresting
side-effects; it doesn't *try* to do or avoid either of these things.
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Andrea Arcangeli @ 2016-11-02 21:38 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161102211118.sc4j3fezfqxg23i3@sigill.intra.peff.net>
Hello,
On Wed, Nov 02, 2016 at 05:11:18PM -0400, Jeff King wrote:
> In fact, it is not even git that does this, but rather what
> Mail::Address happens to output (though git has fallback routines if
> that module isn't available that do the same thing).
If it's something in perl it should be fixed there indeed. I didn't
debug what exactly was wrong, so I sent it here first.
> But in either case, in my test, the actual email address is still
> extracted correctly and fed to the MTA, so the mail is delivered.
The email is delivered to the right email for me too, but to see the
problem you need to check the email itself, and look how the To: field
looks in the actual email in your mail client or web interface.
Don't you see whatever@yourhostname without spaces and @yourhostname
instead of the email domain?
If you still can't reproduce, maybe it's a different perl
Mail::Address, I'm using dev-perl/MailTools-2.140.0
From who receives the email it just looks garbled, but --dry-run
showed a correct To/Cc list so it was undetectable that it would end
up garbled during the real email delivery.
> So I'm not sure what you wanted to happen that didn't. Did you want
> --dry-run to complain about the bogus address? Did the message not get
> delivered for you? Something else?
If --dry-run complained and failed instead of passing and then sending
an email with garbled To/Cc list, it'd be more than enough. Either
that or it should generate a mail header that matches the output of
--dry-run so the review of --dry-run becomes meaningful again.
Thanks,
Andrea
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Jeff King @ 2016-11-02 21:11 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: git
In-Reply-To: <20161102202709.GI4611@redhat.com>
On Wed, Nov 02, 2016 at 09:27:09PM +0100, Andrea Arcangeli wrote:
> send-email gets confused with one trailing " at the end of the
> email. Details and how to reproduce below, it breaks also with
> upstream git version 2.10.2.dirty.
I'm not quite sure what happened, and what you wanted to happen. In your
example:
> FYI: apparently I hit a git bug in this submit... reproducible with
> the below command:
>
> git send-email -1 --to '"what ever" <--your--@--email--.com>"'
The "to" address is slightly bogus here because of the extra
double-quote. That gets turned into a slightly bogus rfc2822 header:
> *snip*
> Dry-OK. Log says:
> To: "what ever" " <--your--@--email--.com>
Which is funny, but matches what we do with other addresses that are
invalid according to the rfc. E.g., there was a discussion recently on:
Stable <stable@vger.kernel.org> [4.8+]
which has historically been converted to:
"Stable [4.8+]" <stable@vger.kernel.org>
In fact, it is not even git that does this, but rather what
Mail::Address happens to output (though git has fallback routines if
that module isn't available that do the same thing).
But in either case, in my test, the actual email address is still
extracted correctly and fed to the MTA, so the mail is delivered.
So I'm not sure what you wanted to happen that didn't. Did you want
--dry-run to complain about the bogus address? Did the message not get
delivered for you? Something else?
-Peff
^ permalink raw reply
* [ANNOUNCE] Git for Windows 2.10.2
From: Johannes Schindelin @ 2016-11-02 20:58 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]
Dear Git users,
It is my pleasure to announce that Git for Windows 2.10.2 is available from:
https://git-for-windows.github.io/
Changes since Git for Windows v2.10.1(2) (October 13th 2016)
Git for windows v2.10.1(2) was a MinGit-only release (i.e. there was no
Git for windows installer for that version).
New Features
• Comes with Git v2.10.2.
• Comes with Git Credential Manager v1.8.1.
• Comes with cURL v7.51.0.
• Git for Windows can now be built easily with Visual C++ 2015.
• The installer now logs post-install errors more verbosely.
• A new option asks the installer to skip installation if Git's files
are in use.
• A new option asks the installer to quietly skip downgrading Git for
Windows, without indicating failure.
• There is now an explicit option for symbolic link support,
including a link to a more verbose explanation of the issue.
Bug Fixes
• when upgrading Git for Windows, SSH agent processes are now
auto-terminated.
• When trying to install/upgrade on a Windows version that is no
longer supported, we now refuse to do so.
Filename | SHA-256
-------- | -------
Git-2.10.2-64-bit.exe | 57238ebf86f8b51e32cab44bb91c8060e04774676b77b9fb92f78e7bc7e9a179
Git-2.10.2-32-bit.exe | 8b15054a4ea2dd5d2be0471a430d8ae7c772b94e1a1048221083a0040011011c
PortableGit-2.10.2-64-bit.7z.exe | 101314826892480043d5b11989726fc8ee448991eb7b0a1c61aca751161bc15b
PortableGit-2.10.2-32-bit.7z.exe | edc616817e96a6f15246bb0dd93c97e53d38d3b2a0b7375f26bd0bd082c41a73
MinGit-2.10.2-64-bit.zip | a10de5d5a8e71e207eff20d833ca56902a0668940e3def5f21d089e4f533ea40
MinGit-2.10.2-32-bit.zip | 2b191598bcb37565a2b80729ef8d00c03df02b75f6b9d012080c458999f89cc0
Git-2.10.2-64-bit.tar.bz2 | 7df449ac1813876b5a2e9215d94bca9458c2e0870c65e5b78bd7fc2a448a2a90
Git-2.10.2-32-bit.tar.bz2 | 3f8d0bebc53fabad863b8fe352a317fde61833efa4750f96656cf95017a621e8
Ciao,
Johannes
--
You received this message because you are subscribed to the Google Groups "git-for-windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to git-for-windows+unsubscribe@googlegroups.com.
To post to this group, send email to git-for-windows@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/git-for-windows/20161102204358.6856-1-johannes.schindelin%40gmx.de.
For more options, visit https://groups.google.com/d/optout.
^ 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