* Re: [PATCH v4 1/2] lib-proto-disable: variable name fix
From: Jeff King @ 2016-11-07 20:48 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161107204028.GC143723@google.com>
On Mon, Nov 07, 2016 at 12:40:28PM -0800, Brandon Williams wrote:
> On 11/07, Jeff King wrote:
> > On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
> >
> > > Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
> >
> > Even for a trivial fixup like this, I think it's good to say why.
> > Because what seems trivial and obvious to you while working on the patch
> > may not be so to a reviewer, or somebody reading it 6 months later.
> >
> > Just something simple like:
> >
> > The test_proto function assigns the positional parameters to named
> > variables, but then still refers to "$desc" as "$1". Using $desc is
> > more readable and less error-prone.
> >
> > -Peff
>
> Alright will do. Commit messages don't seem to be an area of strength
> for me, but I'm working on it! :D
It's possible that I'm overly picky about my commit messages, but that
does not stop me from trying to train an army of picky-commit-message
clones. :)
-Peff
^ permalink raw reply
* Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick
From: Stefan Beller @ 2016-11-07 20:48 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <378e63aa70e54fe9b839acf90680917a@exmbdft7.ad.twosigma.com>
On Mon, Nov 7, 2016 at 12:38 PM, David Turner <David.Turner@twosigma.com> wrote:
>> -----Original Message-----
>> From: Stefan Beller [mailto:sbeller@google.com]
>> Sent: Monday, November 07, 2016 2:14 PM
>> To: David Turner
>> Cc: git@vger.kernel.org
>> Subject: Re: [PATCH] submodules: allow empty working-tree dirs in
>> merge/cherry-pick
>>
>> On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@twosigma.com>
>> wrote:
>> > When a submodule is being merged or cherry-picked into a working tree
>> > that already contains a corresponding empty directory, do not record a
>> > conflict.
>> >
>> > One situation where this bug appears is:
>> >
>> > - Commit 1 adds a submodule
>>
>> "... at sub1" as inferred by text below.
>>
>> > - Commit 2 removes that submodule and re-adds it into a subdirectory
>> > (sub1 to sub1/sub1).
>> > - Commit 3 adds an unrelated file.
>> >
>> > Now the user checks out commit 1 (first deinitializing the submodule),
>> > and attempts to cherry-pick commit 3. Previously, this would fail,
>> > because the incoming submodule sub1/sub1 would falsely conflict with
>> > the empty sub1 directory.
>>
>> So you'd want to achieve:
>> $ # on commit 3:
>> git checkout <commit 1>
>> git cherry-pick <commit 3>
>>
>> which essentially moves the gitlink back to its original place (from
>> sub1/sub1 -> sub1). This sounds reasonable.
>> But what if the submodule contains a (file/directory) named sub1? We'd
>> first remove the sub1/sub1 submodule (and even delete the inner
>> directory?), such that "sub1/"
>> becomes an empty dir, which is perfect for having a submodule right there
>> at "sub1/"
>
> I'm confused about the "what if" here.
>
> In our particular situation, the submodule in question was not initialized.
oops. That explains it. I somehow assumed we were talking about
initialized submodules.
>
> If your approach also fixes the same tests that mine fixes, then I am happy to use your series over mine. Please CC me so I can take a peek.
No, my series seems to be orthogonal to this one. I plan
on cc'ing you nevertheless as it is still nearby.
> basically nobody needs two copies of one submodule in the same repo.
IIRC this is how gitlinks were used in very early days :/
(kernel people were using gitlinks to track different kernel versions
and see if they were interoperable or working at all.
e.g. see d92a39590d1126e195f1bbccf182a2cdb79218e7, which
only makes sense (for the update command) if the referenced repository
contains references of all submodules, which either means a huge reference
pile that contains different projects at the same time, or the same project
at different versions.
> I think that case fails for other reasons anyway.
>
Yes. I agree that the patch as-is is applicable. I did not try to oppose
your approach, but rather give some thoughts I had.
Stefan
^ permalink raw reply
* Re: [PATCH v4 1/2] lib-proto-disable: variable name fix
From: Brandon Williams @ 2016-11-07 20:40 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161107202637.ooaddlkfntw37xg3@sigill.intra.peff.net>
On 11/07, Jeff King wrote:
> On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
>
> > Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
>
> Even for a trivial fixup like this, I think it's good to say why.
> Because what seems trivial and obvious to you while working on the patch
> may not be so to a reviewer, or somebody reading it 6 months later.
>
> Just something simple like:
>
> The test_proto function assigns the positional parameters to named
> variables, but then still refers to "$desc" as "$1". Using $desc is
> more readable and less error-prone.
>
> -Peff
Alright will do. Commit messages don't seem to be an area of strength
for me, but I'm working on it! :D
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 2/2] transport: add protocol policy config option
From: Jeff King @ 2016-11-07 20:44 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <1478547323-47332-2-git-send-email-bmwill@google.com>
On Mon, Nov 07, 2016 at 11:35:23AM -0800, Brandon Williams wrote:
> Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
> specify a whitelist of protocols to be used in clone/fetch/push
> commands. This patch introduces new configuration options for more
> fine-grained control for allowing/disallowing protocols. This also has
> the added benefit of allowing easier construction of a protocol
> whitelist on systems where setting an environment variable is
> non-trivial.
>
> Now users can specify a policy to be used for each type of protocol via
> the 'protocol.<name>.allow' config option. A default policy for all
> unconfigured protocols can be set with the 'protocol.allow' config
> option. If no user configured default is made git, by default, will
> allow known-safe protocols (http, https, git, ssh, file), disallow
A minor nit, but in "If no user configured default is made git, by
default, will..." the second "by default" is redundant. And possibly
misleading. This _is_ the default case, there is no other way to change
it. :)
> +protocol.<name>.allow::
> + Set a policy to be used by protocol <name> with clone/fetch/push commands.
`<name>` isn't defined here at all. I still think the list of protocols
should go here, but at the very least, you need to point the user to the
existing list in git(1).
> `GIT_ALLOW_PROTOCOL`::
> - If set, 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. The protocol names
> - currently used by git are:
> + The preferred way to configure allowed protocols is done through the
> + config interface, though this setting takes precedences. See
s/precedences/precedence/.
I actually wonder if we should even drop "the preferred way" here. I had
initially thought we would want it just for backwards-compatibility, but
I actually think it is useful in its own right as a shorthand for more
complicated config (and since we have to keep it around effectively
forever anyway, there's no real cost to continuing to call it a feature
versus a deprecated feature).
I'm including a squashable patch at the end of this email with suggested
wording (and which also moves the protocol list).
> + test_expect_success "clone $desc (env var has precedence)" '
> + rm -rf tmp.git &&
> + (
> + GIT_ALLOW_PROTOCOL=none &&
> + export GIT_ALLOW_PROTOCOL &&
> + test_must_fail git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
> + )
> + '
This test is a good addition in this round.
I suppose we could test also that GIT_ALLOW_PROTOCOL overrides
protocol.allow, but I'm not sure if there is a point. If git were a
black box, it's a thing I might check, but we know from the design that
this is an unlikely bug (and that the implementation is unlikely to
change in a way to cause it). So I could go either way.
> [...]
The rest of it looks good to me.
Squashable documentation suggestions are below.
-Peff
---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e89b33f9e..a9dc23f82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2331,7 +2331,28 @@ protocol.allow::
--
protocol.<name>.allow::
- Set a policy to be used by protocol <name> with clone/fetch/push commands.
+ Set a policy to be used by protocol `<name>` with clone/fetch/push
+ commands. See `protocol.allow` above for the available policies.
++
+The protocol names currently used by git are:
++
+--
+ - `file`: any local file-based path (including `file://` URLs,
+ or local paths)
+
+ - `git`: the anonymous git protocol over a direct TCP
+ connection (or proxy, if configured)
+
+ - `ssh`: git over ssh (including `host:path` syntax,
+ `ssh://`, etc).
+
+ - `http`: git over http, both "smart http" and "dumb http".
+ Note that this does _not_ include `https`; if you want to configure
+ both, you must do so individually.
+
+ - any external helpers are named by their protocol (e.g., use
+ `hg` to allow the `git-remote-hg` helper)
+--
pull.ff::
By default, Git does not create an extra merge commit when merging
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c9823e34a..c52cec840 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,30 +1150,13 @@ of clones and fetches.
cloning a repository to make a backup).
`GIT_ALLOW_PROTOCOL`::
- The preferred way to configure allowed protocols is done through the
- config interface, though this setting takes precedences. See
- linkgit:git-config[1] for more details. If set to a colon-separated
- list of protocols, behave as if `protocol.allow` is set to `never`, and
- each of the listed protocols has `protocol.<name>.allow` set to
- `always`. In other words, any protocol not mentioned will be
- disallowed (i.e., this is a whitelist, not a blacklist). The protocol
- names currently used by git are:
-
- - `file`: any local file-based path (including `file://` URLs,
- or local paths)
-
- - `git`: the anonymous git protocol over a direct TCP
- connection (or proxy, if configured)
-
- - `ssh`: git over ssh (including `host:path` syntax,
- `ssh://`, etc).
-
- - `http`: git over http, both "smart http" and "dumb http".
- Note that this does _not_ include `https`; if you want both,
- you should specify both as `http:https`.
-
- - any external helpers are named by their protocol (e.g., use
- `hg` to allow the `git-remote-hg` helper)
+ If set to a colon-separated list of protocols, behave as if
+ `protocol.allow` is set to `never`, and each of the listed
+ protocols has `protocol.<name>.allow` set to `always`
+ (overriding any existing configuration). In other words, any
+ protocol not mentioned will be disallowed (i.e., this is a
+ whitelist, not a blacklist). See the description of
+ `protocol.allow` in linkgit:git-config[1] for more details.
`GIT_PROTOCOL_FROM_USER`::
Set to 0 to prevent protocols used by fetch/push/clone which are
^ permalink raw reply related
* RE: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick
From: David Turner @ 2016-11-07 20:38 UTC (permalink / raw)
To: 'Stefan Beller'; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kbp82VssqutEg5=TnciQrGkDRQ3iNm_vUo9eBBGMXQRNg@mail.gmail.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Monday, November 07, 2016 2:14 PM
> To: David Turner
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] submodules: allow empty working-tree dirs in
> merge/cherry-pick
>
> On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@twosigma.com>
> wrote:
> > When a submodule is being merged or cherry-picked into a working tree
> > that already contains a corresponding empty directory, do not record a
> > conflict.
> >
> > One situation where this bug appears is:
> >
> > - Commit 1 adds a submodule
>
> "... at sub1" as inferred by text below.
>
> > - Commit 2 removes that submodule and re-adds it into a subdirectory
> > (sub1 to sub1/sub1).
> > - Commit 3 adds an unrelated file.
> >
> > Now the user checks out commit 1 (first deinitializing the submodule),
> > and attempts to cherry-pick commit 3. Previously, this would fail,
> > because the incoming submodule sub1/sub1 would falsely conflict with
> > the empty sub1 directory.
>
> So you'd want to achieve:
> $ # on commit 3:
> git checkout <commit 1>
> git cherry-pick <commit 3>
>
> which essentially moves the gitlink back to its original place (from
> sub1/sub1 -> sub1). This sounds reasonable.
> But what if the submodule contains a (file/directory) named sub1? We'd
> first remove the sub1/sub1 submodule (and even delete the inner
> directory?), such that "sub1/"
> becomes an empty dir, which is perfect for having a submodule right there
> at "sub1/"
I'm confused about the "what if" here.
In our particular situation, the submodule in question was not initialized. Basically, the submodule move by developer A messed up developer B's rebase, where developers A and B had been working on completely disjoint sets of submodules. If it had been initialized, that might be a different story. It would be somewhat less surprising, and thus probably OK. The "first deinitializing the submodule" bit above, I think, describes the situation.
If the "what if" you are worried about is corruption caused the move of sub1/sub1 into sub1, don't worry about it. sub1/ would still contain the .git file, and so would not be empty. Even if this patch were really wacky, the worst it could do is delete already-empty directories.
> > This patch ignores the empty sub1 directory, fixing the bug. We only
> > ignore the empty directory if the object being emplaced is a
> > submodule, which expects an empty directory.
> >
> > Signed-off-by: David Turner <dturner@twosigma.com>
> > ---
> > merge-recursive.c | 21 +++++++++++++++------
> > t/t3030-merge-recursive.sh | 4 ++-- t/t3426-rebase-submodule.sh |
> > 3 ---
> > 3 files changed, 17 insertions(+), 11 deletions(-)
> >
> > Note that there are four calls to dir_in_way, and only two of them
> > have changed their semantics. This is because the merge code is quite
> > complicated, and I don't fully understand it.
>
> A good approach. I was trying to haggle with unpack-trees.c and the
> merging code and put way more on my plate than I could eat in one sitting.
> Trying to get the mess sorted now to prepare a patch series this week.
If your approach also fixes the same tests that mine fixes, then I am happy to use your series over mine. Please CC me so I can take a peek.
> > So I did not have time
> > to analyze the remaining calls to see whether they, too, should be
> > changed.
>
> The call in line 1205 (in handle_file, which is only called from
> conflict_rename_rename_1to2) may be relevant if we move around submodules
> on the same level and modifying it in different branches.
> However I think preserving current behavior is ok.
So, the case there would be moving sub1 to sub2, where sub2 was previously a different submodule? It appears that this works at least after my patch, if not before. But I gather from the name rename_1to2 that I actually need to copy the submodule not move it? This seems like such a rare case that I don't actually need to handle it; basically nobody needs two copies of one submodule in the same repo. I think that case fails for other reasons anyway.
> The other one in handle_change_delete also doesn't look obvious one way or
> another, so I'd stick with current behavior.
This appears to be implicated in the t6022 test that I mentioned -- if I change empty_ok unconditionally to 1, the test fails.
> >For me, there are no test failures either way, indicating that
> >probably these cases are rare.
>
> The tests have to be crafted for this specific code pattern,
>
> >
> > The reason behind the empty_ok parameter (as opposed to just always
> > allowing empy directories to be blown away) is found in t6022's 'pair
> > rename to parent of other (D/F conflicts) w/ untracked dir'. This
> > test would fail with an unconditional rename, because it wouldn't
> > generate the conflict file.
>
> Or the submodule from your commit message contains a "sub1/..." itself.
See above.
^ permalink raw reply
* Re: [PATCH v4 1/2] lib-proto-disable: variable name fix
From: Jeff King @ 2016-11-07 20:26 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <1478547323-47332-1-git-send-email-bmwill@google.com>
On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
> Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
Even for a trivial fixup like this, I think it's good to say why.
Because what seems trivial and obvious to you while working on the patch
may not be so to a reviewer, or somebody reading it 6 months later.
Just something simple like:
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
-Peff
^ permalink raw reply
* [PATCH v4 1/2] lib-proto-disable: variable name fix
From: Brandon Williams @ 2016-11-07 19:35 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, bburky, peff, jrnieder
In-Reply-To: <1478292933-7873-1-git-send-email-bmwill@google.com>
Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..be88e9a 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -9,7 +9,7 @@ test_proto () {
proto=$2
url=$3
- test_expect_success "clone $1 (enabled)" '
+ test_expect_success "clone $desc (enabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +18,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (enabled)" '
+ test_expect_success "fetch $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +27,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (enabled)" '
+ test_expect_success "push $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +36,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (disabled)" '
+ test_expect_success "push $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +45,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (disabled)" '
+ test_expect_success "fetch $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +54,7 @@ test_proto () {
)
'
- test_expect_success "clone $1 (disabled)" '
+ test_expect_success "clone $desc (disabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=none &&
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 2/2] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-07 19:35 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, bburky, peff, jrnieder
In-Reply-To: <1478547323-47332-1-git-send-email-bmwill@google.com>
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git, by default, will
allow known-safe protocols (http, https, git, ssh, file), disallow
known-dangerous protocols (ext), and have a default policy of `user` for
all other protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King <peff@peff.net>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/config.txt | 25 ++++++++
Documentation/git.txt | 21 ++++---
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 129 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 ++++++++++++++++++++++-
7 files changed, 242 insertions(+), 22 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..0b1c186 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2308,6 +2308,31 @@ pretty.<name>::
Note that an alias with the same name as a built-in format
will be silently ignored.
+protocol.allow::
+ If set, provide a user defined default policy for all protocols which
+ don't explicitly have a policy (`protocol.<name>.allow`). By default,
+ if unset, known-safe protocols (http, https, git, ssh, file) have a
+ default policy of `always`, known-dangerous protocols (ext) have a
+ default policy of `never`, and all other protocols have a default
+ policy of `user`. Supported policies:
++
+--
+
+* `always` - protocol is always able to be used.
+
+* `never` - protocol is never able to be used.
+
+* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
+ either unset or has a value of 1. This policy should be used when you want a
+ protocol to be directly usable by the user but don't want it used by commands which
+ execute clone/fetch/push commands without user input, e.g. recursive
+ submodule initialization.
+
+--
+
+protocol.<name>.allow::
+ Set a policy to be used by protocol <name> with clone/fetch/push commands.
+
pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..c9823e3 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,13 +1150,14 @@ of clones and fetches.
cloning a repository to make a backup).
`GIT_ALLOW_PROTOCOL`::
- If set, 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. The protocol names
- currently used by git are:
+ The preferred way to configure allowed protocols is done through the
+ config interface, though this setting takes precedences. See
+ linkgit:git-config[1] for more details. If set to a colon-separated
+ list of protocols, behave as if `protocol.allow` is set to `never`, and
+ each of the listed protocols has `protocol.<name>.allow` set to
+ `always`. In other words, any protocol not mentioned will be
+ disallowed (i.e., this is a whitelist, not a blacklist). The protocol
+ names currently used by git are:
- `file`: any local file-based path (including `file://` URLs,
or local paths)
@@ -1174,6 +1175,12 @@ of clones and fetches.
- any external helpers are named by their protocol (e.g., use
`hg` to allow the `git-remote-hg` helper)
+`GIT_PROTOCOL_FROM_USER`::
+ Set to 0 to prevent protocols used by fetch/push/clone which are
+ configured to the `user` state. This is useful to restrict recursive
+ submodule initialization from an untrusted repository or for programs
+ which feed potentially-untrusted URLS to git commands. See
+ linkgit:git-config[1] for more details.
Discussion[[Discussion]]
------------------------
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..0a477b4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -21,14 +21,10 @@ require_work_tree
wt_prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
-# Restrict ourselves to a vanilla subset of protocols; the URLs
-# we get are under control of a remote repository, and we do not
-# want them kicking off arbitrary git-remote-* programs.
-#
-# 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}
-export GIT_ALLOW_PROTOCOL
+# Tell the rest of git that any URLs we get don't come
+# directly from the user, so it can apply policy as appropriate.
+GIT_PROTOCOL_FROM_USER=0
+export GIT_PROTOCOL_FROM_USER
command=
branch=
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index be88e9a..4b2db2f 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -1,10 +1,7 @@
# Test routines for checking protocol disabling.
-# test cloning a particular protocol
-# $1 - description of the protocol
-# $2 - machine-readable name of the protocol
-# $3 - the URL to try cloning
-test_proto () {
+# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
+test_whitelist () {
desc=$1
proto=$2
url=$3
@@ -62,6 +59,128 @@ test_proto () {
test_must_fail git clone --bare "$url" tmp.git
)
'
+
+ test_expect_success "clone $desc (env var has precedence)" '
+ rm -rf tmp.git &&
+ (
+ GIT_ALLOW_PROTOCOL=none &&
+ export GIT_ALLOW_PROTOCOL &&
+ test_must_fail git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+ )
+ '
+}
+
+test_config () {
+ desc=$1
+ proto=$2
+ url=$3
+
+ # Test clone/fetch/push with protocol.<type>.allow config
+ test_expect_success "clone $desc (enabled with config)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git -c protocol.$proto.allow=never clone --bare "$url" tmp.git
+ '
+
+ # Test clone/fetch/push with protocol.user.allow and its env var
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user push origin HEAD:pushed
+ )
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user fetch
+ )
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ (
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ )
+ '
+
+ # Test clone/fetch/push with protocol.allow user defined default
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git config --global protocol.allow always &&
+ git clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ git config --global protocol.allow never &&
+ test_must_fail git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git clone --bare "$url" tmp.git
+ '
+}
+
+# test cloning a particular protocol
+# $1 - description of the protocol
+# $2 - machine-readable name of the protocol
+# $3 - the URL to try cloning
+test_proto () {
+ test_whitelist "$@"
+
+ test_config "$@"
}
# set up an ssh wrapper that will access $host/$repo in the
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index bc44ac3..75c570a 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git init original &&
(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d..c6c2661 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git commit --allow-empty -m initial &&
test_tick &&
diff --git a/transport.c b/transport.c
index d57e8de..2c0ec76 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER = 0,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ /* first check the per-protocol config */
+ if (!git_config_get_string(key, &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config(key, value);
+ free(key);
+ free(value);
+ return ret;
+ }
+ free(key);
+
+ /* if defined, fallback to user-defined default for unknown protocols */
+ if (!git_config_get_string("protocol.allow", &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config("protocol.allow", value);
+ free(value);
+ return ret;
+ }
+
+ /* fallback to built-in defaults */
+ /* known safe */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh") ||
+ !strcmp(type, "file"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; by default let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
+
+ die("BUG: invalid protocol_allow_config type");
}
void transport_check_allowed(const char *type)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick
From: Stefan Beller @ 2016-11-07 19:13 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <1478543491-6286-1-git-send-email-dturner@twosigma.com>
On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@twosigma.com> wrote:
> When a submodule is being merged or cherry-picked into a working
> tree that already contains a corresponding empty directory, do not
> record a conflict.
>
> One situation where this bug appears is:
>
> - Commit 1 adds a submodule
"... at sub1" as inferred by text below.
> - Commit 2 removes that submodule and re-adds it into a subdirectory
> (sub1 to sub1/sub1).
> - Commit 3 adds an unrelated file.
>
> Now the user checks out commit 1 (first deinitializing the submodule),
> and attempts to cherry-pick commit 3. Previously, this would fail,
> because the incoming submodule sub1/sub1 would falsely conflict with
> the empty sub1 directory.
So you'd want to achieve:
$ # on commit 3:
git checkout <commit 1>
git cherry-pick <commit 3>
which essentially moves the gitlink back to its original place
(from sub1/sub1 -> sub1). This sounds reasonable.
But what if the submodule contains a (file/directory)
named sub1? We'd first remove the sub1/sub1 submodule
(and even delete the inner directory?), such that "sub1/"
becomes an empty dir, which is perfect for having a
submodule right there at "sub1/"
>
> This patch ignores the empty sub1 directory, fixing the bug. We only
> ignore the empty directory if the object being emplaced is a
> submodule, which expects an empty directory.
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
> merge-recursive.c | 21 +++++++++++++++------
> t/t3030-merge-recursive.sh | 4 ++--
> t/t3426-rebase-submodule.sh | 3 ---
> 3 files changed, 17 insertions(+), 11 deletions(-)
>
> Note that there are four calls to dir_in_way, and only two of them
> have changed their semantics. This is because the merge code is quite
> complicated, and I don't fully understand it.
A good approach. I was trying to haggle with unpack-trees.c and
the merging code and put way more on my plate than I could eat
in one sitting. Trying to get the mess sorted now to prepare a patch
series this week.
> So I did not have time
> to analyze the remaining calls to see whether they, too, should be
> changed.
The call in line 1205 (in handle_file, which is only called from
conflict_rename_rename_1to2) may be relevant if we move around
submodules on the same level and modifying it in different branches.
However I think preserving current behavior is ok.
The other one in handle_change_delete also doesn't look obvious
one way or another, so I'd stick with current behavior.
>For me, there are no test failures either way, indicating
> that probably these cases are rare.
The tests have to be crafted for this specific code pattern,
>
> The reason behind the empty_ok parameter (as opposed to just always
> allowing empy directories to be blown away) is found in t6022's 'pair
> rename to parent of other (D/F conflicts) w/ untracked dir'. This
> test would fail with an unconditional rename, because it wouldn't
> generate the conflict file.
Or the submodule from your commit message contains a "sub1/..." itself.
> It's not clear how important that
> behavior is (I do not recall ever noticing the file~branch thing
> before), but it seemed better to preserve it in case it was important.
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 9041c2f..e64b48b 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const char *path, const char *
> return strbuf_detach(&newpath, NULL);
> }
>
> -static int dir_in_way(const char *path, int check_working_copy)
> +/**
> + * Check whether a directory in the index is in the way of an incoming
> + * file. Return 1 if so. If check_working_copy is non-zero, also
> + * check the working directory. If empty_ok is non-zero, also return
> + * 0 in the case where the working-tree dir exists but is empty.
> + */
Thanks for the documenting comment! This is probably fine as is with just
two boolean parameters. If we'd add more, we might have thought about
adding a flags parameter with bits for each flag.
Looks good to me,
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-07 19:17 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161104230613.epbziphiqyl57bcn@sigill.intra.peff.net>
On 11/04, Jeff King wrote:
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 27069ac..5d845c4 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2308,6 +2308,31 @@ pretty.<name>::
> > Note that an alias with the same name as a built-in format
> > will be silently ignored.
> >
> > +protocol.allow::
> > + If set, provide a user defined default policy for all protocols which
> > + don't explicitly have a policy (protocol.<name>.allow). By default,
> > + if unset, known-safe protocols (http, https, git, ssh, file) have a
> > + default policy of `always`, known-dangerous protocols (ext) have a
> > + default policy of `never`, and all other protocols have a default policy
> > + of `user`. Supported policies:
> > ++
> > +--
> > +
> > +* `always` - protocol is always able to be used.
> > +
> > +* `never` - protocol is never able to be used.
> > +
> > +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
> > + either unset or has a value of 1. This policy should be used when you want a
> > + protocol to be usable by the user but don't want it used by commands which
> > + execute clone/fetch/pull commands without user input, e.g. recursive
> > + submodule initialization.
>
> Makes sense. I wonder if it would be good to emphasize _directly_ usable
> here. I.e., "...when you want a protocol to be directly usable by the
> user but don't want...".
>
> Should clone/fetch/pull also include push?
You're right, that should really have been clone/fetch/push.
>
> > +protocol.<name>.allow::
> > + Set a policy to be used by protocol <name> with clone/fetch/pull commands.
> > +
>
> Nice that this matches protocol.allow, so we don't need to re-explain
> that.
>
> Should the list of protocols be here? I know they're covered under
> GIT_ALLOW_PROTOCOL already, but if this is the preferred system, we
> should probably explain them here, and then just have GIT_ALLOW_PROTOCOL
> refer the user.
Right now the list of protocols under GIT_ALLOW_PROTOCOL looks like it
has a bit more documentation with how the colon list works. The
protocols are also mentioned above with their default behaviour.
>
> > diff --git a/Documentation/git.txt b/Documentation/git.txt
> > index ab7215e..ab25580 100644
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -1150,13 +1150,13 @@ of clones and fetches.
> > cloning a repository to make a backup).
> >
> > `GIT_ALLOW_PROTOCOL`::
> > - If set, 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. The protocol names
> > - currently used by git are:
> > + The new way to configure allowed protocols is done through the config
> > + interface, though this setting takes precedences. See
> > + linkgit:git-config[1] for more details. If set, 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). The protocol names currently
> > + used by git are:
>
> I wonder if we can explain this in terms of the config system. Something
> like:
>
> If set to a colon-separated list of zero or more protocols, behave as
> if `protocol.allow` is set to `never`, and each of the listed
> protocols has `protocol.$protocol.allow` set to `always`.
Yeah that makes sense.
>
> > +`GIT_PROTOCOL_FROM_USER`::
> > + Set to 0 to prevent protocols used by fetch/push/clone which are
> > + configured to the `user` state. This is useful to restrict recursive
> > + submodule initialization from an untrusted repository. See
> > + linkgit:git-config[1] for more details.
>
> Under "this is useful", it may make sense to make it clear that external
> programs can use this, too. Something like:
>
> It may also be useful for programs which feed potentially-untrusted
> URLs to git commands.
I'll put that in too.
>
> > diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
> > index b0917d9..5950fbf 100644
> > --- a/t/lib-proto-disable.sh
> > +++ b/t/lib-proto-disable.sh
> > @@ -1,15 +1,12 @@
> > # Test routines for checking protocol disabling.
> >
> > -# test cloning a particular protocol
> > -# $1 - description of the protocol
> > -# $2 - machine-readable name of the protocol
> > -# $3 - the URL to try cloning
> > -test_proto () {
> > +# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
> > +test_whitelist () {
> > desc=$1
> > proto=$2
> > url=$3
> >
> > - test_expect_success "clone $1 (enabled)" '
> > + test_expect_success "clone $desc (enabled)" '
>
> Yeah, this should have been $desc all along. It makes the diff really
> noisy, though. Should it be split out into a preparatory change?
I'll pull it out to make the patch a bit cleaner.
--
Brandon Williams
^ permalink raw reply
* [PATCH 3/3] gitk: clear array 'commitinfo' on reload
From: Markus Hitter @ 2016-11-07 19:03 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Paul Mackerras
In-Reply-To: <8e1c5923-d2a6-bc77-97ab-3f154b41d2ea@jump-ing.de>
From 8359452f426c68cc02250f25f20eaaacd2ddd001 Mon Sep 17 00:00:00 2001
From: Markus Hitter <mah@jump-ing.de>
Date: Mon, 7 Nov 2016 19:02:51 +0100
Subject: [PATCH 3/3] gitk: clear array 'commitinfo' on reload
After a reload we might have an entirely different set of commits,
so keeping all of them leaks memory. Remove them all because
re-creating them is not more expensive than testing wether they're
still valid. Lazy (re-)creation is already well established, so
a missing entry can't cause harm.
Signed-off-by: Markus Hitter <mah@jump-ing.de>
---
gitk | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 518a4ce..aef6db6 100755
--- a/gitk
+++ b/gitk
@@ -588,7 +588,7 @@ proc updatecommits {} {
proc reloadcommits {} {
global curview viewcomplete selectedline currentid thickerline
global showneartags treediffs commitinterest cached_commitrow
- global targetid
+ global targetid commitinfo
set selid {}
if {$selectedline ne {}} {
@@ -609,6 +609,7 @@ proc reloadcommits {} {
getallcommits
}
clear_display
+ unset -nocomplain commitinfo
unset -nocomplain commitinterest
unset -nocomplain cached_commitrow
unset -nocomplain targetid
--
2.9.3
^ permalink raw reply related
* [PATCH 2/3] gitk: remove closed file descriptors from $blobdifffd
From: Markus Hitter @ 2016-11-07 19:01 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Paul Mackerras
In-Reply-To: <e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de>
From 0a463fcd977dc9558835c373e24a095e35ca3c82 Mon Sep 17 00:00:00 2001
From: Markus Hitter <mah@jump-ing.de>
Date: Mon, 7 Nov 2016 16:01:17 +0100
Subject: [PATCH 2/3] gitk: remove closed file descriptors from $blobdifffd
One shouldn't have descriptors of already closed files around.
The first idea to deal with this (previously) ever growing array
was to remove it entirely, but it's needed to detect start of a
new diff with ths old diff not yet done. This happens when a user
clicks on the same commit in the commit list repeatedly without
delay.
Signed-off-by: Markus Hitter <mah@jump-ing.de>
---
gitk | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/gitk b/gitk
index 8654e29..518a4ce 100755
--- a/gitk
+++ b/gitk
@@ -8069,7 +8069,11 @@ proc getblobdiffline {bdf ids} {
$ctext conf -state normal
while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {
if {$ids != $diffids || $bdf != $blobdifffd($ids)} {
+ # Older diff read. Abort it.
catch {close $bdf}
+ if {$ids != $diffids} {
+ array unset blobdifffd $ids
+ }
return 0
}
parseblobdiffline $ids $line
@@ -8078,6 +8082,7 @@ proc getblobdiffline {bdf ids} {
blobdiffmaybeseehere [eof $bdf]
if {[eof $bdf]} {
catch {close $bdf}
+ array unset blobdifffd $ids
return 0
}
return [expr {$nr >= 1000? 2: 1}]
--
2.9.3
^ permalink raw reply related
* [PATCH 1/3] gitk: turn off undo manager in the text widget
From: Markus Hitter @ 2016-11-07 18:57 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Paul Mackerras
In-Reply-To: <de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de>
From e965e1deb9747bbc2b40dc2de95afb65aee9f7fd Mon Sep 17 00:00:00 2001
From: Markus Hitter <mah@jump-ing.de>
Date: Sun, 6 Nov 2016 20:38:03 +0100
Subject: [PATCH 1/3] gitk: turn off undo manager in the text widget
The diff text widget is read-only, so there's zero point in
building an undo stack. This change reduces memory consumption of
this widget by about 95%.
Memory usage of the whole program for viewing a reference commit
before; 579'692'744 bytes, after: 32'724'446 bytes.
Test procedure:
- Choose a largish commit and check it out. In this case one with
90'802 lines, 5'006'902 bytes.
- Have a Tcl version with memory debugging enabled. This is,
build one with --enable-symbols=mem passed to configure.
- Instrument Gitk to regularly show a memory dump. E.g. by adding
these code lines at the very bottom:
proc memDump {} {
catch {
set output [memory info]
puts $output
}
after 3000 memDump
}
memDump
- Start Gitk, it'll load this largish commit into the diff text
field automatically (because it's the current commit).
- Wait until memory consumption levels out and note the numbers.
Note that the numbers reported by [memory info] are much smaller
than the ones reported in 'top' (1.75 GB vs. 105 MB in this case),
likely due to all the instrumentation coming with the debug
version of Tcl.
Signed-off-by: Markus Hitter <mah@jump-ing.de>
---
gitk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 805a1c7..8654e29 100755
--- a/gitk
+++ b/gitk
@@ -2403,7 +2403,7 @@ proc makewindow {} {
set ctext .bleft.bottom.ctext
text $ctext -background $bgcolor -foreground $fgcolor \
- -state disabled -font textfont \
+ -state disabled -undo 0 -font textfont \
-yscrollcommand scrolltext -wrap none \
-xscrollcommand ".bleft.bottom.sbhorizontal set"
if {$have_tk85} {
--
2.9.3
^ permalink raw reply related
* [PATCH 0/3] gitk: memory consumption improvements
From: Markus Hitter @ 2016-11-07 18:54 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Paul Mackerras
List, Paul,
after searching for a while on why Gitk sometimes consumes exorbitant amounts of memory I found a pair of minor issues and also a big one: the text widget comes with an unlimited undo manager, which is turned on be default. Considering that each line is inserted seperately, this piles up a huuuge undo stack ... for a read-only text widget. Simply turning off this undo manager saves about 95% of memory when viewing large commits (with tens of thousands of diff lines).
3 patches are about to follow:
- turn off the undo manager,
- forget already closed file descriptors and
- forget the 'commitinfo' array on a reload to enforce reloading it.
I hope this finds you appreciation.
Markus
--
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-07 18:14 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Blake Burkhart, Jeff King, Jonathan Nieder
In-Reply-To: <CAGZ79ka18s3-nzkCog3+g70NCoAawoqO1t-qghKSZJxjK6jkmw@mail.gmail.com>
On 11/04, Stefan Beller wrote:
> By default, if unset, ... have a default policy ...
> sounds strange. How about just dropping the first 4 words here:
>
> Known-safe protocols (http, https, git, ssh, file) have a
> default policy of `always`, known-dangerous protocols (ext) have a
> default policy of `never`, and all other protocols have a default policy
> of `user`. Supported policies:
>
>
> What happens if protocol.allow is set to true?
That wouldn't be allowed, its an unknown value as the only permitted
values are always, never, and user.
>
>
>
> > + if unset, known-safe protocols (http, https, git, ssh, file) have a
> > + default policy of `always`, known-dangerous protocols (ext) have a
> > + default policy of `never`, and all other protocols have a default policy
> > + of `user`. Supported policies:
> > ++
> > +--
> > +
> > +* `always` - protocol is always able to be used.
> > +
> > +* `never` - protocol is never able to be used.
> > +
> > +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
> > + either unset or has a value of 1. This policy should be used when you want a
> > + protocol to be usable by the user but don't want it used by commands which
> > + execute clone/fetch/pull commands without user input, e.g. recursive
> > + submodule initialization.
> > +
> > +--
> > +
> > +protocol.<name>.allow::
> > + Set a policy to be used by protocol <name> with clone/fetch/pull commands.
>
> How does this interact with protocol.allow?
>
> When protocol.allow is set, this overrides the specific protocol.
> If protocol is not set, it overrides the specific protocol as well(?)
protocol.allow is a default for protocols which don't have a specific
protocol.<name>.allow entry
> > + test_expect_success "clone $desc (disabled)" '
> > + rm -rf tmp.git &&
> > + test_must_fail git clone --bare "$url" tmp.git
> > + '
>
>
> I could not spot a test for GIT_ALLOW_PROTOCOL overriding
> any protocol*allow policy. Is that also worth testing? (for
> backwards compatibility of tools that make use of GIT_ALLOW_PROTOCOL
> but the user already setup a policy.
I can add in one quick test for that.
--
Brandon Williams
^ permalink raw reply
* Re: git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n
From: Stefan Beller @ 2016-11-07 18:34 UTC (permalink / raw)
To: Anthony Sottile; +Cc: git@vger.kernel.org
In-Reply-To: <CA+dzEBnDZGsYyZkSO5E+3EX+d1j7Kb4kiY6OkC0mtwaq5px5aQ@mail.gmail.com>
On Mon, Nov 7, 2016 at 10:30 AM, Anthony Sottile <asottile@umich.edu> wrote:
> This has worked great up until now (and is very convenient for trying things
> out without blowing away the system installation). What changed?
>
(Just guessing myself:)
$ git log --grep git-sh-i18n v2.10.0..v2.11.0-rc0
commit da14d73d5eacfb2fa9d054f94d9eecb2244c3ce5
Merge: 2f445c17e5 1073094f30
Author: Junio C Hamano <gitster@pobox.com>
Date: Mon Oct 31 13:15:25 2016 -0700
Merge branch 'ak/sh-setup-dot-source-i18n-fix'
Recent update to git-sh-setup (a library of shell functions that
are used by our in-tree scripted Porcelain commands) included
another shell library git-sh-i18n without specifying where it is,
relying on the $PATH. This has been fixed to be more explicit by
prefixing $(git --exec-path) output in front.
* ak/sh-setup-dot-source-i18n-fix:
git-sh-setup: be explicit where to dot-source git-sh-i18n from.
commit 1073094f30a8dd5ae49f2146f587085c4fe86410
Author: Anders Kaseorg <andersk@mit.edu>
Date: Sat Oct 29 22:10:02 2016 -0400
git-sh-setup: be explicit where to dot-source git-sh-i18n from.
d323c6b641 ("i18n: git-sh-setup.sh: mark strings for translation",
2016-06-17) started to dot-source git-sh-i18n shell script library,
assuming that $PATH is already adjusted for our scripts, namely,
$GIT_EXEC_PATH is at the beginning of $PATH.
Old contrib scripts like contrib/convert-grafts-to-replace-refs.sh
and contrib/rerere-train.sh and third-party scripts like guilt may
however be using this as ". $(git --exec-path)/git-sh-setup",
without satisfying that assumption. Be more explicit by specifying
its path prefixed with "$(git --exec-path)/". to be safe.
While we’re here, move the sourcing of git-sh-i18n below the shell
portability fixes.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick
From: David Turner @ 2016-11-07 18:31 UTC (permalink / raw)
To: git, sbeller; +Cc: David Turner
When a submodule is being merged or cherry-picked into a working
tree that already contains a corresponding empty directory, do not
record a conflict.
One situation where this bug appears is:
- Commit 1 adds a submodule
- Commit 2 removes that submodule and re-adds it into a subdirectory
(sub1 to sub1/sub1).
- Commit 3 adds an unrelated file.
Now the user checks out commit 1 (first deinitializing the submodule),
and attempts to cherry-pick commit 3. Previously, this would fail,
because the incoming submodule sub1/sub1 would falsely conflict with
the empty sub1 directory.
This patch ignores the empty sub1 directory, fixing the bug. We only
ignore the empty directory if the object being emplaced is a
submodule, which expects an empty directory.
Signed-off-by: David Turner <dturner@twosigma.com>
---
merge-recursive.c | 21 +++++++++++++++------
t/t3030-merge-recursive.sh | 4 ++--
t/t3426-rebase-submodule.sh | 3 ---
3 files changed, 17 insertions(+), 11 deletions(-)
Note that there are four calls to dir_in_way, and only two of them
have changed their semantics. This is because the merge code is quite
complicated, and I don't fully understand it. So I did not have time
to analyze the remaining calls to see whether they, too, should be
changed. For me, there are no test failures either way, indicating
that probably these cases are rare.
The reason behind the empty_ok parameter (as opposed to just always
allowing empy directories to be blown away) is found in t6022's 'pair
rename to parent of other (D/F conflicts) w/ untracked dir'. This
test would fail with an unconditional rename, because it wouldn't
generate the conflict file. It's not clear how important that
behavior is (I do not recall ever noticing the file~branch thing
before), but it seemed better to preserve it in case it was important.
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f..e64b48b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const char *path, const char *
return strbuf_detach(&newpath, NULL);
}
-static int dir_in_way(const char *path, int check_working_copy)
+/**
+ * Check whether a directory in the index is in the way of an incoming
+ * file. Return 1 if so. If check_working_copy is non-zero, also
+ * check the working directory. If empty_ok is non-zero, also return
+ * 0 in the case where the working-tree dir exists but is empty.
+ */
+static int dir_in_way(const char *path, int check_working_copy, int empty_ok)
{
int pos;
struct strbuf dirpath = STRBUF_INIT;
@@ -684,7 +690,8 @@ static int dir_in_way(const char *path, int check_working_copy)
}
strbuf_release(&dirpath);
- return check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode);
+ return check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode) &&
+ !(empty_ok && is_empty_dir(path));
}
static int was_tracked(const char *path)
@@ -1062,7 +1069,7 @@ static int handle_change_delete(struct merge_options *o,
{
char *renamed = NULL;
int ret = 0;
- if (dir_in_way(path, !o->call_depth)) {
+ if (dir_in_way(path, !o->call_depth, 0)) {
renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
}
@@ -1195,7 +1202,7 @@ static int handle_file(struct merge_options *o,
remove_file(o, 0, rename->path, 0);
dst_name = unique_path(o, rename->path, cur_branch);
} else {
- if (dir_in_way(rename->path, !o->call_depth)) {
+ if (dir_in_way(rename->path, !o->call_depth, 0)) {
dst_name = unique_path(o, rename->path, cur_branch);
output(o, 1, _("%s is a directory in %s adding as %s instead"),
rename->path, other_branch, dst_name);
@@ -1704,7 +1711,8 @@ static int merge_content(struct merge_options *o,
o->branch2 == rename_conflict_info->branch1) ?
pair1->two->path : pair1->one->path;
- if (dir_in_way(path, !o->call_depth))
+ if (dir_in_way(path, !o->call_depth,
+ S_ISGITLINK(pair1->two->mode)))
df_conflict_remains = 1;
}
if (merge_file_special_markers(o, &one, &a, &b,
@@ -1862,7 +1870,8 @@ static int process_entry(struct merge_options *o,
oid = b_oid;
conf = _("directory/file");
}
- if (dir_in_way(path, !o->call_depth)) {
+ if (dir_in_way(path, !o->call_depth,
+ S_ISGITLINK(a_mode))) {
char *new_path = unique_path(o, path, add_branch);
clean_merge = 0;
output(o, 1, _("CONFLICT (%s): There is a directory with name %s in %s. "
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 470f334..be074a1 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -575,13 +575,13 @@ test_expect_success 'merge removes empty directories' '
test_must_fail test -d d
'
-test_expect_failure 'merge-recursive simple w/submodule' '
+test_expect_success 'merge-recursive simple w/submodule' '
git checkout submod &&
git merge remove
'
-test_expect_failure 'merge-recursive simple w/submodule result' '
+test_expect_sucess 'merge-recursive simple w/submodule result' '
git ls-files -s >actual &&
(
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index d5b896d..ebf4f5e 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -38,9 +38,6 @@ git_rebase_interactive () {
git rebase -i "$1"
}
-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
-# The real reason "replace directory with submodule" fails is because a
-# directory "sub1" exists, but we reuse the suppression added for merge here
test_submodule_switch "git_rebase_interactive"
test_done
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n
From: Stefan Beller @ 2016-11-07 17:56 UTC (permalink / raw)
To: Anthony Sottile; +Cc: git@vger.kernel.org
In-Reply-To: <CA+dzEBmP2aUit00ukJyQeg=iqUJJLVaovafo2gngf9MvEqZDPA@mail.gmail.com>
$ git --version
git version 1.8.5.6
$ if [ "$LATEST_GIT" = "1" ]; then
...
export PATH="/tmp/git:$PATH"
fi
....
make -j 8
...
$ git --version
git version 2.11.0-rc0
So you compile 2.11.0-rc0 yourself, but you do not install it, instead
the $PATH is pointed to /tmp/git instead, however the later calls fail with:
Can't open /home/travis/libexec/git-core/git-sh-i18n
which is where 1.8.5.6 is installed.
So you're running into an internationalization problem
between 2 very different versions of Git (1.8 seems to be ancient)
Not sure if i can offer advice except from
"Don't do that, instead install Git properly". ;)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
From: Paul Smith @ 2016-11-07 17:49 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, git, tboegi, gitster
In-Reply-To: <20161107174559.t72vxxkckqdbxmbg@sigill.intra.peff.net>
On Mon, 2016-11-07 at 12:46 -0500, Jeff King wrote:
> Specifically I wanted to make sure that
>
> FOO = bar
> FOO =
> ifdef FOO
> ... something ...
> endif
>
> works as if FOO had never been set in the first place. Which it seems
> to, at least in GNU make (and that is the only one we support, for
> other reasons).
Yes, it will work. Confusingly, "ifdef" actually tests whether the
variable has a non-empty value, not whether it's defined:
> The 'ifdef' form takes the _name_ of a variable as its argument, not
> a reference to a variable. The value of that variable has a non-
> empty value, the TEXT-IF-TRUE is effective; otherwise, the TEXT-IF-
> FALSE, if any, is effective
*sigh* History...
^ permalink raw reply
* Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
From: Jeff King @ 2016-11-07 17:46 UTC (permalink / raw)
To: Paul Smith; +Cc: Lars Schneider, git, tboegi, gitster
In-Reply-To: <1478540194.4171.19.camel@mad-scientist.net>
On Mon, Nov 07, 2016 at 12:36:34PM -0500, Paul Smith wrote:
> On Mon, 2016-11-07 at 12:26 -0500, Jeff King wrote:
> > I have in the back of my mind a fear that it is harder to unset a
> > make variable than it is to override it with a new value (which is
> > what you'd want to do here to turn openssl back on),
>
> It depends on what you mean by "unset".
>
> If you mean it as per the shell "unset" command, where the variable is
> completely forgotten as if it never was set at all, that's tricky. You
> have to use the "undefine" special command which was introduced in GNU
> make 3.82 (released in 2010).
>
> But if you just want to set the variable to the empty string, using
> "FOO=" works fine for that in all versions of make (GNU and otherwise)
> and using all the normal rules (command line override, etc.)
Specifically I wanted to make sure that
FOO = bar
FOO =
ifdef FOO
... something ...
endif
works as if FOO had never been set in the first place. Which it seems
to, at least in GNU make (and that is the only one we support, for other
reasons).
-Peff
^ permalink raw reply
* Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
From: Paul Smith @ 2016-11-07 17:36 UTC (permalink / raw)
To: Jeff King, Lars Schneider; +Cc: git, tboegi, gitster
In-Reply-To: <20161107172617.tlcrpwbjy2w7aoyc@sigill.intra.peff.net>
On Mon, 2016-11-07 at 12:26 -0500, Jeff King wrote:
> I have in the back of my mind a fear that it is harder to unset a
> make variable than it is to override it with a new value (which is
> what you'd want to do here to turn openssl back on),
It depends on what you mean by "unset".
If you mean it as per the shell "unset" command, where the variable is
completely forgotten as if it never was set at all, that's tricky. You
have to use the "undefine" special command which was introduced in GNU
make 3.82 (released in 2010).
But if you just want to set the variable to the empty string, using
"FOO=" works fine for that in all versions of make (GNU and otherwise)
and using all the normal rules (command line override, etc.)
It's not easy to distinguish between a variable that is empty and one
that is actually not defined, in make, so it's a difference without a
distinction in almost all situations.
^ permalink raw reply
* git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n
From: Anthony Sottile @ 2016-11-07 17:27 UTC (permalink / raw)
To: git
Noticed as part of my automated tests here:
https://travis-ci.org/pre-commit/pre-commit/jobs/173957051
Minimal reproduction:
rm -rf /tmp/git /tmp/foo /tmp/bar
git clone git://github.com/git/git --depth 1 /tmp/git
pushd /tmp/git
make -j 8
popd
export PATH="/tmp/git:$PATH"
git init /tmp/foo
git init /tmp/bar
cd /tmp/foo
git submodule add /tmp/bar baz
Output:
$ rm -rf /tmp/git /tmp/foo /tmp/bar
$ git clone git://github.com/git/git --depth 1 /tmp/git
Cloning into '/tmp/git'...
remote: Counting objects: 3074, done.
remote: Compressing objects: 100% (2735/2735), done.
remote: Total 3074 (delta 249), reused 1871 (delta 215), pack-reused 0
Receiving objects: 100% (3074/3074), 6.38 MiB | 905.00 KiB/s, done.
Resolving deltas: 100% (249/249), done.
Checking connectivity... done.
$ pushd /tmp/git
/tmp/git /tmp
$ make -j 8
GIT_VERSION = 2.11.0-rc0
... lots of make output ...
$ popd
/tmp
$ export PATH="/tmp/git:$PATH"
$ git init /tmp/foo
warning: templates not found /home/asottile/share/git-core/templates
Initialized empty Git repository in /tmp/foo/.git/
$ git init /tmp/bar
warning: templates not found /home/asottile/share/git-core/templates
Initialized empty Git repository in /tmp/bar/.git/
$ cd /tmp/foo
$ git submodule add /tmp/bar baz
/tmp/git/git-submodule: 46: .: Can't open
/home/asottile/libexec/git-core/git-sh-i18n
$ echo $?
2
Thanks
Anthony
^ permalink raw reply
* Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
From: Jeff King @ 2016-11-07 17:26 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, tboegi, gitster
In-Reply-To: <8C67FF53-C26F-4993-908F-A5183C5E48D9@gmail.com>
On Sun, Nov 06, 2016 at 08:35:04PM +0100, Lars Schneider wrote:
> Good point. I think I found an even easier way to achieve the same.
> What do you think about the patch below?
>
> [...]
>
> diff --git a/Makefile b/Makefile
> index 9d6c245..f53fcc9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1047,6 +1047,7 @@ ifeq ($(uname_S),Darwin)
> endif
> endif
> ifndef NO_APPLE_COMMON_CRYPTO
> + NO_OPENSSL = YesPlease
> APPLE_COMMON_CRYPTO = YesPlease
> COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
> endif
That is much simpler.
I have in the back of my mind a fear that it is harder to unset a make
variable than it is to override it with a new value (which is what you'd
want to do here to turn openssl back on), but I can't seem to come up
with a case that doesn't work. So I am probably misremembering, or just
thinking of something that used to be a problem long ago.
-Peff
^ permalink raw reply
* Re: git -C has unexpected behaviour
From: Johannes Schindelin @ 2016-11-07 16:54 UTC (permalink / raw)
To: Felix Nairz; +Cc: sbeller, git
In-Reply-To: <CADJspf+zqj2hHjD85dvt8Y4HKPViubvTzybbTq5mJDGCh2q1UQ@mail.gmail.com>
Hi Felix,
On Mon, 7 Nov 2016, Felix Nairz wrote:
> From what you are saying I can see that this expects as designed. It's
> confusing in the submodule case, but I get you don't want to add extra
> rules which slow down performance and mess with other people at the
> same time.
The "messing with other people" is the key point. There are most likely
other users than just me who use the fact that `git -C sub/directory/ ...`
works in a subdirectory of a checkout.
Ciao,
Johannes
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-07 16:11 UTC (permalink / raw)
To: Duy Nguyen
Cc: Jacob Keller, Junio C Hamano, Christian Couder, git,
Shawn O. Pierce, Jeff King, Mike Hommey
In-Reply-To: <CACsJy8BDySBWyp3iiLinRkBCew5FNXoQo7z9dMb6w6m9a5X=NA@mail.gmail.com>
On Mon, Nov 07, 2016 at 04:42:04PM +0700, Duy Nguyen wrote:
> On Mon, Nov 7, 2016 at 8:18 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> > Once we have gitrefs, you have both alternatives: reachable (gitref) or
> > not reachable (gitlink).
> >
> > However, if you want some way to mark reachable objects as not
> > reachable, such as for a sparse checkout, external large-object storage,
> > or similar, then you can use a single unified mechanism for that whether
> > working with gitrefs, trees, or blobs.
>
> How? Whether an object reachable or not is baked in the definition (of
> either gitlink or gitref). I don't think you can have a "maybe
> reachable" type then rely on an external source to determine
> reachability,
You'd have various "reachable by default" entries in trees, including
trees, blobs, and gitrefs, and then have an external mechanism (likely
via .git/config) to say "ignore objects with these hashes/paths". For
instance, you might say "ignore all objects only reachable from the path
'assets/video/*' within a commit's tree". With the right set of client
and server extensions, you could then avoid downloading those objects.
^ 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