* Bug in git rev-parse @{push}?
From: Chris Purcell @ 2016-11-03 14:53 UTC (permalink / raw)
To: git; +Cc: peff
Hi folks,
I think I have discovered a bug in rev-parse's handling of @{push}:
$ git push
Everything up-to-date
$ git rev-parse @{push}
fatal: cannot resolve 'simple' push to a single destination
The documentation for rev-parse says that "the suffix @{push} reports
the branch 'where we would push to' if git push were run while
branchname was checked out", so I would not expect this to error
unless git push does.
The relevant parts of my configuration are:
[push]
default = simple
[remote]
pushdefault = origin
[branch "foo"]
remote = origin
merge = refs/heads/develop
The code in branch_get_push_1 (remote.c) in the PUSH_DEFAULT_SIMPLE
case is calling both branch_get_upstream and tracking_for_push_dest
and erroring if they don't return the same result, which I assume is
incorrect for a triangular workflow?
Please let me know if I've missed out important information by
mistake. I'm happy to work on a patch if given guidance, but this is
definitely outside my comfort zone for an unfamiliar codebase
otherwise! e.g. I can't find the test suite.
Cheers,
Chris
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 14:38 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Williams, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103002225.GA13369@google.com>
On Wed, Nov 02, 2016 at 05:22:25PM -0700, Jonathan Nieder wrote:
> 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.
I agree that this is a big drawback of the current scheme, and it would
be nice to be able to say "also allow".
> That is, would it be possible to use something like
>
> [protocol "sso"]
> allow = always
>
> instead of
>
> [core]
> allowProtocol = file:git:http:https:....:sso
>
> ?
One complication is that the whitelist has multiple states:
1. if it's not used at all, anything goes
2. if it exists and has zero or more entries, only those entries are
allowed
And then submodules are an exception to (1), because it's not anything
goes. It's "this default safe whitelist".
So when does protocol.sso.allow kick in? We wouldn't want it to trigger
case (2) for things like fetch (disabling other non-allowed protocols).
Nor do I think we'd only want it for the submodule case, as I would
assume that "protocol.sso.allow = false" should disable it.
So I think this probably needs to be a separate parallel system where
each protocol can be white- or black-listed in a context-specific way.
Like:
protocol.X.allow = always | user | never
Where "user" specifies that the protocol is OK coming directly from the
user, but not from other sources. And we default known-common-and-good
ones like protocol.http.allow to "always", unknown ones (like "foo"
which runs "remote-foo") to "user"), and possibly known-scary ones like
"ext") to "never".
Then we need some way of telling git "you are in a context where the URL
parameter is not coming from the user". Probably via the environment in
GIT_PROTOCOL_FROM_USER or similar. Which git-submodule would sent when
recursing clones, along with things like "go get".
In other words, stop asking git-submodule or "go get" to specify policy,
and let them specify context that can be used to implement policy that
the user specifies (and have git provide a sane default policy).
I think this would all take a backseat to GIT_ALLOW_PROTOCOL, for
backwards compatibility, and then GIT_ALLOW_PROTOCOL could slowly die
off over time.
> 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?
I think under my proposal above this ugliness just goes away, as all
they have to do is say "trust my URLs less; they come from an automated
source" without specifying policy themselves.
But we may still want...
> 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.
...some automated way to say "is this protocol supported"? I think it is
not just "give me ALLOW_PROTOCOL" anymore, though, but "apply your rules
to this protocol, and tell me if it is supported".
I don't think things like "go get" would need it, but you would if you
had a porcelain built around git that was accessing a URL _not_ via
git-fetch, but wanted to apply git's rules. That could come as a step 2
later, though.
-Peff
^ permalink raw reply
* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Christian Couder @ 2016-11-03 14:34 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8Be-_n_5Aen2-7sod=wkCbXCGxoakf_H-1Cfb4Mg=T9QA@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:52 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Goal
>> ~~~~
>>
>> We want to make it possible to use the split-index feature
>> automatically by just setting a new "core.splitIndex" configuration
>> variable to true.
>
> Thanks. This definitely should help make split index a lot more
> convenient for day-to-day use. The series looks ok (well, except the
> pruning logic being discussed elsewhere in this thread, but I still
> think it's a good strategy).
Thanks for your review.
>> This can be valuable as split-index can help significantly speed up
>> `git rebase` especially along with the work to libify `git apply`
>> that has been recently merged to master
>> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).
>
> I remember this. Since the apply libification work has landed, we can
> now think about not writing index out after every apply step, which
> gives the same (or more) speed up without the complication of
> split-index. But if split-index is good enough for you, we can leave
> it for another time.
Yeah, I think they are separate things that can be done.
When people do a lot of small rebases for example, then split-index
will speed up all the index writing, including the index writing at
the end of each rebase.
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: demerphq @ 2016-11-03 14:33 UTC (permalink / raw)
To: Jeff King; +Cc: Andrea Arcangeli, Git
In-Reply-To: <20161103141848.42pg6iow24prign5@sigill.intra.peff.net>
On 3 November 2016 at 15:18, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 02, 2016 at 11:29:01PM +0100, Andrea Arcangeli wrote:
>
>> 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.
>
> Yes, I can easily reproduce without using git at all by installing
> postfix in local-delivery mode and running:
>
> sendmail peff@sigill.intra.peff.net <<\EOF
> From: Jeff King <peff@peff.net>
> To: "what ever" " <peff@sigill.intra.peff.net>
> Subject: patch
>
> This is the body
> EOF
>
> Many MTAs do this kind of header-rewriting. I don't necessarily agree
> with it as a general concept, but the real problem is the syntactically
> bogus header. The munging that postfix does makes things worse, but I
> can see why it is confused and does what it does (the whole email is
> inside a double-quoted portion that is never closed, so it probably
> thinks there is no hostname portion at all).
>
> So git is possibly at fault for passing along a bogus address. OTOH, the
> user is perhaps at fault for providing the bogus address to git in the
> first place. GIGO. :)
>
> I think if any change were to be made, it would be to recognize this
> bogosity and either clean it up or abort. That ideally would happen via
> Mail::Address so git does not have to add a bunch of ad-hoc "is this
> valid rfc822" checks. Reading the manpage for that module, though, it
> says:
>
> [we do not handle all of rfc2822]
> Often requests are made to the maintainers of this code improve this
> situation, but this is not a good idea, where it will break zillions
> of existing applications. If you wish for a fully RFC2822 compliant
> implementation you may take a look at Mail::Message::Field::Full, part
> of MailBox.
>
> So it's possible that switching to a more robust module would improve
> things.
There is an RFC2822 compliant email address validator in the perl test
suite if you guys want to use it. We use it to test recursive
patterns.
http://perl5.git.perl.org/perl.git/blob/HEAD:/t/re/reg_email.t
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Lars Schneider @ 2016-11-03 14:21 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <73aed2a3-f2ad-ecc2-d43c-86b4a110a970@kdbg.org>
> On 2 Nov 2016, at 17:43, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 02.11.2016 um 18:04 schrieb Torsten Bögershausen:
>>> * ls/filter-process (2016-10-17) 14 commits
>>> (merged to 'next' on 2016-10-19 at ffd0de042c)
>>
>> Some (late, as I recently got a new battery for the Mac OS 10.6 test system)
>> comments:
>> t0021 failes here:
>>
>>
>> Can't locate object method "flush" via package "IO::Handle" at /Users/tb/projects/git/git.next/t/t0021/rot13-filter.pl line 90.
>> fatal: The remote end hung up unexpectedly
>>
>>
>> perl itself is 5.10 and we use the one shipped with Mac OS.
>> Why that ?
>> t0021 uses the hard-coded path:
>> t0021/rot13-filter.pl (around line 345) and the nice macro
>> PERL_PATH from the Makefile is fully ignored.
>>
>> Commenting out the different "flush" makes the test hang, and I haven't digged further.
>>
>
> https://public-inbox.org/git/e8deda5f-11a6-1463-4fc5-25454084ccb1@kdbg.org/
Woooh. I am sorry Hannes - I completely missed that email! Looks like Peff addressed the issue already. His patches look very good but I want to try it on my machine tomorrow.
Thanks,
Lars
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Jeff King @ 2016-11-03 14:18 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: git
In-Reply-To: <20161102222901.GK4611@redhat.com>
On Wed, Nov 02, 2016 at 11:29:01PM +0100, Andrea Arcangeli wrote:
> 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.
Yes, I can easily reproduce without using git at all by installing
postfix in local-delivery mode and running:
sendmail peff@sigill.intra.peff.net <<\EOF
From: Jeff King <peff@peff.net>
To: "what ever" " <peff@sigill.intra.peff.net>
Subject: patch
This is the body
EOF
Many MTAs do this kind of header-rewriting. I don't necessarily agree
with it as a general concept, but the real problem is the syntactically
bogus header. The munging that postfix does makes things worse, but I
can see why it is confused and does what it does (the whole email is
inside a double-quoted portion that is never closed, so it probably
thinks there is no hostname portion at all).
So git is possibly at fault for passing along a bogus address. OTOH, the
user is perhaps at fault for providing the bogus address to git in the
first place. GIGO. :)
I think if any change were to be made, it would be to recognize this
bogosity and either clean it up or abort. That ideally would happen via
Mail::Address so git does not have to add a bunch of ad-hoc "is this
valid rfc822" checks. Reading the manpage for that module, though, it
says:
[we do not handle all of rfc2822]
Often requests are made to the maintainers of this code improve this
situation, but this is not a good idea, where it will break zillions
of existing applications. If you wish for a fully RFC2822 compliant
implementation you may take a look at Mail::Message::Field::Full, part
of MailBox.
So it's possible that switching to a more robust module would improve
things.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Jacob Keller @ 2016-11-03 5:36 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, bmwill, Git mailing list
In-Reply-To: <20161102231722.15787-3-sbeller@google.com>
On Wed, Nov 2, 2016 at 4:17 PM, Stefan Beller <sbeller@google.com> wrote:
> -`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.
>
Update the doc text here as well?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Junio C Hamano @ 2016-11-03 2:44 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Brandon Williams, git, Stefan Beller, Blake Burkhart, Jeff King
In-Reply-To: <20161103002225.GA13369@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> That is, would it be possible to use something like
>
> [protocol "sso"]
> allow = always
>
> instead of
>
> [core]
> allowProtocol = file:git:http:https:....:sso
>
> ?
That sounds like quite a large usability improvement to me.
^ permalink raw reply
* [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
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