* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Jeff King @ 2016-11-08 5:33 UTC (permalink / raw)
To: Bryan Turner; +Cc: Git Users, René Scharfe
In-Reply-To: <CAGyf7-FYvUgvOZm0xvFAJx=8hSc4ji=YQ5dUm3B1unU_WOcjeQ@mail.gmail.com>
On Mon, Nov 07, 2016 at 05:12:43PM -0800, Bryan Turner wrote:
> > The obvious solution is one of:
> >
> > 1. Stop calling normalize() at all when we do not have a relative base
> > and the path is not absolute. This restores the original quirky
> > behavior (plus makes the "foo/../../bar" case work).
Actually, I think we want to keep normalizing, as it is possible for
relative paths to normalize correctly (e.g., "foo/../bar"). We just need
to ignore the error, which is easy.
The patch is below, and is the absolute minimum I think we'd need for
v2.11.
Beyond that, we could go further:
a. Actually make a real absolute path based on getcwd(), which would
protect against later chdir() calls, and possibly help with
duplicate suppression. I'm not sure there are actually any
triggerable bugs here, so I went for the minimal fix.
b. Pick a more sane base for the absolute path, like $GIT_DIR. If we
did so, then people using relative paths in
GIT_ALTERNATE_OBJECT_DIRECTORIES from a bare repo would continue to
work, and people in non-bare repositories would have to add an
extra ".." to most of their paths. So a slight regression, but
saner overall semantics.
Making it relative to the object directory ($GIT_DIR/objects, or
even whatever is in $GIT_OBJECT_DIRECTORY) makes even more sense
to me, but would regress even the bare case (and would probably be
"interesting" along with the tmp-objdir stuff, which sets
$GIT_OBJECT_DIRECTORY on the fly, as that would invalidate
$GIT_ALTERNATE_OBJECT_DIRECTORIES).
I'm inclined to leave those to anybody interested post-v2.11 (or never,
if nobody cares). But it would be pretty trivial to do (a) as part of
this initial fix if anybody feels strongly.
> Is there anything I can do to help? I'm happy to test out changes.
The patch at the end of his mail obviously passes the newly-added tests
for me, but please confirm that it fixes your test suite.
I gather your suite is about noticing behavior changes between different
versions. For cases where we know there is an obvious right behavior, it
would be nice if you could contribute them as patches to git's test
suite. This case was overlooked because there was no test coverage at
all.
Barring that, running your suite and giving easily-reproducible problem
reports is valuable. The earlier the better. So I am happy to see this
on -rc0, and not on the final release. Periodically running it on
"master" during the development cycle would have caught it even sooner.
> At the moment I have limited ability to actually try to submit patches
> myself. I really need to sit down and setup a working development
> environment for Git. (My current dream, if I could get such an
> environment running, is to follow up on your git blame-tree work.
I would be happy for somebody to pick that up, too. It has been powering
GitHub's tree-view for several years now, but I know there are some
rough edges as well as opportunities to optimize it.
-- >8 --
Subject: [PATCH] alternates: re-allow relative paths from environment
Commit 670c359da (link_alt_odb_entry: handle normalize_path
errors, 2016-10-03) regressed the handling of relative paths
in the GIT_ALTERNATE_OBJECT_DIRECTORIES variable. It's not
entirely clear this was ever meant to work, but it _has_
worked for several years, so this commit restores the
original behavior.
When we get a path in GIT_ALTERNATE_OBJECT_DIRECTORIES, we
add it the path to the list of alternate object directories
as if it were found in objects/info/alternates, but with one
difference: we do not provide the link_alt_odb_entry()
function with a base for relative paths. That function
doesn't turn it into an absolute path, and we end up feeding
the relative path to the strbuf_normalize_path() function.
Most relative paths break out of the top-level directory
(e.g., "../foo.git/objects"), and thus normalizing fails.
Prior to 670c359da, we simply ignored the error, and due to
the way normalize_path_copy() was implemented it happened to
return the original path in this case. We then accessed the
alternate objects using this relative path.
By storing the relative path in the alt_odb list, the path
is relative to wherever we happen to be at the time we do an
object lookup. That means we look from $GIT_DIR in a bare
repository, and from the top of the worktree in a non-bare
repository.
If this were being designed from scratch, it would make
sense to pick a stable location (probably $GIT_DIR, or even
the object directory) and use that as the relative base,
turning the result into an absolute path. However, given
the history, at this point the minimal fix is to match the
pre-670c359da behavior.
We can do this simply by ignoring the error when we have no
relative base and using the original value (which we now
reliably have, thanks to strbuf_normalize_path()).
That still leaves us with a relative path that foils our
duplicate detection, and may act strangely if we ever
chdir() later in the process. We could solve that by storing
an absolute path based on getcwd(). That may be a good
future direction; for now we'll do just the minimum to fix
the regression.
The new t5615 script demonstrates the fix in its final three
tests. Since we didn't have any tests of the alternates
environment variable at all, it also adds some tests of
absolute paths.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_file.c | 2 +-
t/t5615-alternate-env.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+), 1 deletion(-)
create mode 100755 t/t5615-alternate-env.sh
diff --git a/sha1_file.c b/sha1_file.c
index 5457314e6..9c86d1924 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -296,7 +296,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
}
strbuf_addstr(&pathbuf, entry);
- if (strbuf_normalize_path(&pathbuf) < 0) {
+ if (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {
error("unable to normalize alternate object path: %s",
pathbuf.buf);
strbuf_release(&pathbuf);
diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
new file mode 100755
index 000000000..22d9d8178
--- /dev/null
+++ b/t/t5615-alternate-env.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+
+test_description='handling of alternates in environment variables'
+. ./test-lib.sh
+
+check_obj () {
+ alt=$1; shift
+ while read obj expect
+ do
+ echo "$obj" >&3 &&
+ echo "$obj $expect" >&4
+ done 3>input 4>expect &&
+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$alt \
+ git "$@" cat-file --batch-check='%(objectname) %(objecttype)' \
+ <input >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'create alternate repositories' '
+ git init --bare one.git &&
+ one=$(echo one | git -C one.git hash-object -w --stdin) &&
+ git init --bare two.git &&
+ two=$(echo two | git -C two.git hash-object -w --stdin)
+'
+
+test_expect_success 'objects inaccessible without alternates' '
+ check_obj "" <<-EOF
+ $one missing
+ $two missing
+ EOF
+'
+
+test_expect_success 'access alternate via absolute path' '
+ check_obj "$(pwd)/one.git/objects" <<-EOF
+ $one blob
+ $two missing
+ EOF
+'
+
+test_expect_success 'access multiple alternates' '
+ check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
+ $one blob
+ $two blob
+ EOF
+'
+
+# bare paths are relative from $GIT_DIR
+test_expect_success 'access alternate via relative path (bare)' '
+ git init --bare bare.git &&
+ check_obj "../one.git/objects" -C bare.git <<-EOF
+ $one blob
+ EOF
+'
+
+# non-bare paths are relative to top of worktree
+test_expect_success 'access alternate via relative path (worktree)' '
+ git init worktree &&
+ check_obj "../one.git/objects" -C worktree <<-EOF
+ $one blob
+ EOF
+'
+
+# path is computed after moving to top-level of worktree
+test_expect_success 'access alternate via relative path (subdir)' '
+ mkdir subdir &&
+ check_obj "one.git/objects" -C subdir <<-EOF
+ $one blob
+ EOF
+'
+
+test_done
--
2.11.0.rc0.263.g6f44bc3
^ permalink raw reply related
* Re: [PATCH v4 1/2] lib-proto-disable: variable name fix
From: Jacob Keller @ 2016-11-08 3:32 UTC (permalink / raw)
To: Jeff King
Cc: Brandon Williams, Git mailing list, Stefan Beller, bburky,
Jonathan Nieder
In-Reply-To: <20161107204838.xm463zdimiw7fx77@sigill.intra.peff.net>
On Mon, Nov 7, 2016 at 12:48 PM, Jeff King <peff@peff.net> wrote:
> 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
You're not the only one ;)
Regards,
Jake
^ permalink raw reply
* [PATCH v5 1/2] lib-proto-disable: variable name fix
From: Brandon Williams @ 2016-11-07 21:51 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, bburky, peff, jrnieder
In-Reply-To: <1478547323-47332-1-git-send-email-bmwill@google.com>
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.
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
* Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate
From: Jacob Keller @ 2016-11-08 2:28 UTC (permalink / raw)
To: Ian Jackson; +Cc: Git mailing list, Junio C Hamano
In-Reply-To: <20161108005241.19888-6-ijackson@chiark.greenend.org.uk>
On Mon, Nov 7, 2016 at 4:52 PM, Ian Jackson
<ijackson@chiark.greenend.org.uk> wrote:
> +log.noAbbrevTags::
> + Each value is a glob pattern, specifying tag nammes which
> + should always be displayed in full, even when other tags may
> + be omitted or abbreviated (for example, by linkgit:gitk[1]).
> + Values starting with `^` specify tags which should be
> + abbreviated. The order is important: the last match, in the
> + most-local configuration, wins.
> +
It seems weird that this description implies some sort of behavior
change in core git itself, but in fact is only used as a reference for
other tools that may or may not honor it. I guess the reasoning here
is to try to get other external tools that abbreviate tags to also
honor this? But it still seems pretty weird to have a documented
config that has no code in core git to honor it...
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Duy Nguyen @ 2016-11-08 1:38 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161107211522.vzl4zpsu5cpembgc@sigill.intra.peff.net>
On Tue, Nov 8, 2016 at 4:15 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:
>
>> And I'll admit my main motivation is not that index/filesystem parity,
>> but rather just that:
>>
>> git clone git://host.com/malicious-repo.git
>> git log
>>
>> might create and read symlinks to arbitrary files on the cloner's box.
>> I'm not sure to what degree to be worried about that. It's not like you
>> can't make other arbitrary symlinks which are likely to be read if the
>> user actually starts looking at checked-out files. It's just that we
>> usually try to make a clone+log of a malicious repository safe.
This I can buy.
> Another approach is to have a config option to disallow symlinks to
> destinations outside of the repository tree (I'm not sure if it should
> be on or off by default, though).
Let's err on the safe side and disable symlinks to outside repo by
default (or even all symlinks on .gitattributes and .gitignore as the
first step)
What I learned from my changes in .gitignore is, if we have not
forbidden something, people likely find some creative use for it. As
long as it's can be turned on or off, i guess those minority will stay
happy.
> Again, I don't know that there is a specific security issue, but it
> makes things easier for services which might clone untrusted
> repositories (e.g., things like CI). They'd obviously have to be careful
> with the contents of the repositories anyway, but it's one less thing to
> have to worry about.
>
> -Peff
--
Duy
^ permalink raw reply
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Bryan Turner @ 2016-11-08 1:12 UTC (permalink / raw)
To: Jeff King; +Cc: Git Users, René Scharfe
In-Reply-To: <20161108003034.apydvv3bav3s7ehq@sigill.intra.peff.net>
On Mon, Nov 7, 2016 at 4:30 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:
>
>> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
>> > }
>> >
>> > strbuf_add_absolute_path(&objdirbuf, get_object_directory());
>> > - normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
>> > + if (strbuf_normalize_path(&objdirbuf) < 0)
>> > + die("unable to normalize object directory: %s",
>> > + objdirbuf.buf);
>>
>> This appears to break the ability to use a relative alternate via an
>> environment variable, since normalize_path_copy_len is explicitly
>> documented "Returns failure (non-zero) if a ".." component appears as
>> first path"
>
> That shouldn't happen, though, because the path we are normalizing has
> been converted to an absolute path via strbuf_add_absolute_path. IOW, if
> your relative path is "../../../foo", we should be feeding something
> like "/path/to/repo/.git/objects/../../../foo" and normalizing that to
> "/path/to/foo".
>
> But in your example, you see:
>
> error: unable to normalize alternate object path: ../0/objects
>
> which cannot come from the code above, which calls die(). It should be
> coming from the call in link_alt_odb_entry().
Ah, of course. I should have looked more closely at the call.
<snip>
> No, I had no intention of disallowing relative alternates (and as you
> noticed, a commit from the same series actually expands the use of
> relative alternates). My use has been entirely within info/alternates
> files, though, not via the environment.
>
> As I said, I'm not sure this was ever meant to work, but as far as I can
> tell it mostly _has_ worked, modulo some quirks. So I think we should
> consider it a regression for it to stop working in v2.11.
>
> The obvious solution is one of:
>
> 1. Stop calling normalize() at all when we do not have a relative base
> and the path is not absolute. This restores the original quirky
> behavior (plus makes the "foo/../../bar" case work).
>
> If we want to do the minimum before releasing v2.11, it would be
> that. I'm not sure it leaves things in a very sane state, but at
> least v2.11 does no harm, and anybody who cares can build saner
> semantics for v2.12.
>
> 2. Fix it for real. Pass a real relative_base when linking from the
> environment. The question is: what is the correct relative base? I
> suppose "getcwd() at the time we prepare the alt odb" is
> reasonable, and would behave similarly to older versions ($GIT_DIR
> for bare repos, top of the working tree otherwise).
>
> If we were designing from scratch, I think saner semantics would
> probably be always relative from $GIT_DIR, or even always relative
> from the object directory (i.e., behave as if the paths were given
> in objects/info/alternates). But that breaks compatibility with
> older versions. If we are treating this as a regression, it is not
> very friendly to say "you are still broken, but you might just need
> to add an extra '..' to your path".
>
> So I dunno. I guess that inclines me towards (1), as it lets us punt on
> the harder question.
Is there anything I can do to help? I'm happy to test out changes.
I've got a set of ~1,040 tests that verify all sorts of different Git
behaviors (those tests flagged this change, for example, and found a
regression in git diff-tree in 2.0.2/2.0.3, among other things). I run
them on the "newest" patch release for every feature-bearing line of
Git from 1.8.x up to 2.10 (currently 1.8.0.3, 1.8.1.5, 1.8.2.3,
1.8.3.4, 1.8.4.5, 1.8.5.6, 1.9.5, 2.0.5, 2.1.4, 2.2.3, 2.3.10, 2.4.11,
2.5.5, 2.6.6, 2.7.4, 2.8.4, 2.9.3 and 2.10.2), and I add in RCs of new
as soon as they become available. (I also test Git for Windows; at the
moment I've got 1.8.0, 1.8.1.2, 1.8.3, 1.8.4, 1.8.5.2 and 1.9.5.1 from
msysgit and 2.3.7.1, 2.4.6, 2.5.3, 2.6.4, 2.7.4, 2.8.4, 2.9.3 and
2.10.2 from Git for Windows. 2.11.0-rc0 on Windows passes my test
suite; it looks like it's not tagging the same git/git commit as
v2.11.0-rc0 is.) I wish there was an easy way for me to open this up.
At the moment, it's something I can really only run in-house, as it
were.
At the moment I have limited ability to actually try to submit patches
myself. I really need to sit down and setup a working development
environment for Git. (My current dream, if I could get such an
environment running, is to follow up on your git blame-tree work.
>
> -Peff
^ permalink raw reply
* Re: [PATCH 0/6] Provide for config to specify tags not to abbreviate
From: Ian Jackson @ 2016-11-08 0:54 UTC (permalink / raw)
To: Ian Jackson; +Cc: git, Junio C Hamano, Paul Mackerras
In-Reply-To: <20161108005241.19888-1-ijackson@chiark.greenend.org.uk>
Ian Jackson writes ("[PATCH 0/6] Provide for config to specify tags not to abbreviate"):
> Please find in the following mails patches which provide a way to make
> gitk display certain tags in full, even if they would normally be
> abbreviated.
>
> There are four patches to gitk, three to prepare the ground, and one
> to introduce the new feature.
>
> There is one patch for git, to just document the new config variable.
The eagle-eyed reader will spot that that makes 5 patches, not 6.
There are indeed only 5. The subject mentioning 6 is a mistake -
sorry.
Thanks,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate
From: Ian Jackson @ 2016-11-08 0:52 UTC (permalink / raw)
To: git; +Cc: Ian Jackson, Junio C Hamano
In-Reply-To: <20161108005241.19888-1-ijackson@chiark.greenend.org.uk>
Tags matching a new multi-valued config option log.noAbbrevTags
should not be abbreviated. Currently this config option is
used only by gitk (and the patch to gitk will come via the
gitk maintainer tree).
The config setting is in git config logs.* rather than gitk's
own configuration, because:
- Tools which manage git trees may want to set this, depending
on their knowledge of the nature of the tags likely to be
present;
- Whether this property ought to be set is mostly a property of the
contents of the tag namespaces in the tree, not a user preference.
(Although of course user preferences are supported.)
- Other git utilities (or out of tree utilities) may want to
reference this setting for their own display purposes.
Background motivation:
Debian's dgit archive gateway tool generates and uses tags called
archive/debian/VERSION. If such a tag refers to a Debian source tree,
it is probably very interesting because it refers to a version
actually uploaded to Debian by the Debian package maintainer.
We would therefore like a way to specify that such tags should be
displayed in full. dgit will be able to set an appropriate config
setting in the trees it deals with.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
Documentation/config.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66a..6aade4f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2002,6 +2002,14 @@ log.abbrevCommit::
linkgit:git-whatchanged[1] assume `--abbrev-commit`. You may
override this option with `--no-abbrev-commit`.
+log.noAbbrevTags::
+ Each value is a glob pattern, specifying tag nammes which
+ should always be displayed in full, even when other tags may
+ be omitted or abbreviated (for example, by linkgit:gitk[1]).
+ Values starting with `^` specify tags which should be
+ abbreviated. The order is important: the last match, in the
+ most-local configuration, wins.
+
log.date::
Set the default date-time mode for the 'log' command.
Setting a value for log.date is similar to using 'git log''s
--
2.10.1
^ permalink raw reply related
* [PATCH GITK 4/6] gitk: Provide for config to specify tags not to abbreviate
From: Ian Jackson @ 2016-11-08 0:52 UTC (permalink / raw)
To: git; +Cc: Ian Jackson, Paul Mackerras
In-Reply-To: <20161108005241.19888-1-ijackson@chiark.greenend.org.uk>
Tags matching a new multi-valued config option log.noAbbrevTags
are not abbreviated.
The config setting is in git config logs.* rather than gitk's
own configuration, because:
- Tools which manage git trees may want to set this, depending
on their knowledge of the nature of the tags likely to be
present;
- Whether this property ought to be set is mostly a property of the
contents of the tag namespaces in the tree, not a user preference.
(Although of course user preferences are supported.)
- Other git utilities (or out of tree utilities) may want to
reference this setting for their own display purposes.
There will be another, separate, patch to the `git' tree to document
this config option.
Background motivation:
Debian's dgit archive gateway tool generates and uses tags called
archive/debian/VERSION. If such a tag refers to a Debian source tree,
it is probably very interesting because it refers to a version
actually uploaded to Debian by the Debian package maintainer.
We would therefore like a way to specify that such tags should be
displayed in full. dgit will be able to set an appropriate config
setting in the trees it deals with.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
gitk | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/gitk b/gitk
index d76f1e3..515d7b0 100755
--- a/gitk
+++ b/gitk
@@ -6547,6 +6547,14 @@ proc totalwidth {l font extra} {
}
proc tag_want_unabbrev {tag} {
+ global noabbrevtags
+ # noabbrevtags was reversed when we read config, so take first match
+ foreach pat $noabbrevtags {
+ set inverted [regsub {^\^} $pat {} pat]
+ if {[string match $pat $tag]} {
+ return [expr {!$inverted}]
+ }
+ }
return 0
}
@@ -12138,6 +12146,11 @@ set tclencoding [tcl_encoding $gitencoding]
if {$tclencoding == {}} {
puts stderr "Warning: encoding $gitencoding is not supported by Tcl/Tk"
}
+set noabbrevtags {}
+catch {
+ set noabbrevtags [exec git config --get-all log.noAbbrevTags]
+}
+set noabbrevtags [lreverse [split $noabbrevtags "\n"]]
set gui_encoding [encoding system]
catch {
--
2.10.1
^ permalink raw reply related
* [PATCH GITK 1/6] gitk: Internal: drawtags: Abolish "singletag" variable
From: Ian Jackson @ 2016-11-08 0:52 UTC (permalink / raw)
To: git; +Cc: Ian Jackson, Paul Mackerras
In-Reply-To: <20161108005241.19888-1-ijackson@chiark.greenend.org.uk>
We are going to want to make the contents of `marks' somewhat more
complicated in a moment, so it won't be possible to use what is
effectively a single variable to represent the status of the whole of
the non-heads part of the marks list.
Luckily the strings that replace actual tag names, in the `singletag'
case, are not themselves valid tag names. So they can be detected
directly.
(Also, `singletag' was not quite right anyway: really it meant that
the tag name(s) had been abbreviated.)
No functional change.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
gitk | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gitk b/gitk
index 805a1c7..42fa41a 100755
--- a/gitk
+++ b/gitk
@@ -6570,7 +6570,6 @@ proc drawtags {id x xt y1} {
if {$ntags > $maxtags ||
[totalwidth $marks mainfont $extra] > $maxwidth} {
# show just a single "n tags..." tag
- set singletag 1
if {$ntags == 1} {
set marks [list "tag..."]
} else {
@@ -6620,7 +6619,7 @@ proc drawtags {id x xt y1} {
$xr $yt $xr $yb $xl $yb $x [expr {$yb - $delta}] \
-width 1 -outline $tagoutlinecolor -fill $tagbgcolor \
-tags tag.$id]
- if {$singletag} {
+ if {[regexp {^tag\.\.\.|^\d+ } $tag]} {
set tagclick [list showtags $id 1]
} else {
set tagclick [list showtag $tag_quoted 1]
--
2.10.1
^ permalink raw reply related
* [PATCH GITK 2/6] gitk: Internal: drawtags: Idempotently reset "ntags"
From: Ian Jackson @ 2016-11-08 0:52 UTC (permalink / raw)
To: git; +Cc: Ian Jackson, Paul Mackerras
In-Reply-To: <20161108005241.19888-1-ijackson@chiark.greenend.org.uk>
The previous code tracked its change to the length of `marks' by
updateing the variable `ntags'. This is a bit fragile and cumbersome,
and we are going to want to modify `marks' some more in a moment.
Instead, simply reset ntags to the length of marks, after we have
possibly done any needed abbreviation.
No functional change.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
gitk | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 42fa41a..31aecda 100755
--- a/gitk
+++ b/gitk
@@ -6575,9 +6575,10 @@ proc drawtags {id x xt y1} {
} else {
set marks [list [format "%d tags..." $ntags]]
}
- set ntags 1
}
}
+ set ntags [llength $marks]
+
if {[info exists idheads($id)]} {
set marks [concat $marks $idheads($id)]
set nheads [llength $idheads($id)]
--
2.10.1
^ permalink raw reply related
* [PATCH GITK 3/6] gitk: drawtags: Introduce concept of unabbreviated marks
From: Ian Jackson @ 2016-11-08 0:52 UTC (permalink / raw)
To: git; +Cc: Ian Jackson, Paul Mackerras
In-Reply-To: <20161108005241.19888-1-ijackson@chiark.greenend.org.uk>
We are going to want to show some tags in full, even if they are long
or there are other tags. Do this by filtering the tags into
`marks_unabbrev' and `marks'. `marks_unabbrev' bypasses the tag
abbreviation, and is put on the front of the marks array after any
abbreviation has been done.
No functional change right now because no tags are considered
`unabbrev'.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
gitk | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 31aecda..d76f1e3 100755
--- a/gitk
+++ b/gitk
@@ -6546,6 +6546,10 @@ proc totalwidth {l font extra} {
return $tot
}
+proc tag_want_unabbrev {tag} {
+ return 0
+}
+
proc drawtags {id x xt y1} {
global idtags idheads idotherrefs mainhead
global linespc lthickness
@@ -6564,8 +6568,16 @@ proc drawtags {id x xt y1} {
set delta [expr {int(0.5 * ($linespc - $lthickness))}]
set extra [expr {$delta + $lthickness + $linespc}]
+ set marks_unabbrev {}
if {[info exists idtags($id)]} {
- set marks $idtags($id)
+ set marks {}
+ foreach tag $idtags($id) {
+ if {[tag_want_unabbrev $tag]} {
+ lappend marks_unabbrev $tag
+ } else {
+ lappend marks $tag
+ }
+ }
set ntags [llength $marks]
if {$ntags > $maxtags ||
[totalwidth $marks mainfont $extra] > $maxwidth} {
@@ -6577,6 +6589,7 @@ proc drawtags {id x xt y1} {
}
}
}
+ set marks [concat $marks_unabbrev $marks]
set ntags [llength $marks]
if {[info exists idheads($id)]} {
--
2.10.1
^ permalink raw reply related
* [PATCH 0/6] Provide for config to specify tags not to abbreviate
From: Ian Jackson @ 2016-11-08 0:52 UTC (permalink / raw)
To: git; +Cc: Ian Jackson, Junio C Hamano, Paul Mackerras
Hi.
Please find in the following mails patches which provide a way to make
gitk display certain tags in full, even if they would normally be
abbreviated.
There are four patches to gitk, three to prepare the ground, and one
to introduce the new feature.
There is one patch for git, to just document the new config variable.
I hope this is the right way to submit this series. Thanks for your
attention.
As I say in the patch "gitk: Provide for config to specify tags not to
abbreviate":
The config setting is in git config logs.* rather than gitk's
own configuration, because:
- Tools which manage git trees may want to set this, depending
on their knowledge of the nature of the tags likely to be
present;
- Whether this property ought to be set is mostly a property of the
contents of the tag namespaces in the tree, not a user preference.
(Although of course user preferences are supported.)
- Other git utilities (or out of tree utilities) may want to
reference this setting for their own display purposes.
There will be another, separate, patch to the `git' tree to document
this config option.
Background motivation:
Debian's dgit archive gateway tool generates and uses tags called
archive/debian/VERSION. If such a tag refers to a Debian source tree,
it is probably very interesting because it refers to a version
actually uploaded to Debian by the Debian package maintainer.
We would therefore like a way to specify that such tags should be
displayed in full. dgit will be able to set an appropriate config
setting in the trees it deals with.
Ian Jackson (4):
gitk: Internal: drawtags: Abolish "singletag" variable
gitk: Internal: drawtags: Idempotently reset "ntags"
gitk: drawtags: Introduce concept of unabbreviated marks
gitk: Provide for config to specify tags not to abbreviate
gitk | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
Ian Jackson (1):
config docs: Provide for config to specify tags not to abbreviate
Documentation/config.txt | 8 ++++++++
1 file changed, 8 insertions(+)
--
2.10.1
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Jeff King @ 2016-11-08 0:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Lars Schneider
In-Reply-To: <xmqqk2cgc95m.fsf@gitster.mtv.corp.google.com>
On Sun, Nov 06, 2016 at 06:32:05PM -0800, Junio C Hamano wrote:
> I regret to report that I won't be able to tag 2.11-rc1 as scheduled
> in tinyurl.com/gitCal (I am feverish and my brain is not keeping
> track of things correctly) any time soon. I'll report back an
> updated schedule when able.
Take your time. Even if we end up bumping the release by a whole week, I
don't think it's a big deal (which seems likely given the holiday in the
middle, unless you really want to release on Thanksgiving).
> found on the list. I am aware of only two right now ("cast enum to
> int to work around compiler warning", in Dscho's prepare sequencer
> series, and "wc -l may give leading whitespace" fix J6t pointed out
> in Lars's filter process series), but it is more than likely that I
> am missing a few more.
In addition to J6t's fix in t0021, we need mine that you queued in
jk/filter-process-fix.
I think we also need to make a final decision on the indent/compaction
heuristic naming. After reading Michael's [0], and the claim by you and
Stefan that the "compaction" name was declared sufficiently experimental
that we could later take it away, I'm inclined to follow this plan:
1. Ship v2.11 with what is in master; i.e., both compaction and indent
heuristics, triggerable by config or command line.
2. Post-v2.11, retire the compaction heuristic as a failed experiment.
Keeping it in v2.11 doesn't hurt anything (it was already
released), and lets us take our time coming up with and cooking the
patch.
3. Post-v2.11, flip the default for diff.indentHeuristic to "true".
Keep at least the command line option around indefinitely for
experimenting (i.e., "this diff looks funny; I wonder if
--no-indent-heuristic makes it look better").
Config option can either stay or go at that point. I have no
preference.
The nice thing about that plan is it punts on merging any new code to
post-v2.11. :)
Another possible regression came up today in [1]. I haven't worked up a
patch yet, but I'll do so in the next day or so.
I think that's where we're at now. I'll keep collecting and can give you
the full list when you're back in action.
Get well.
[0] http://public-inbox.org/git/8dbbd28b-af60-5e66-ae27-d7cddca233dc@alum.mit.edu/
[1] http://public-inbox.org/git/20161108003034.apydvv3bav3s7ehq@sigill.intra.peff.net/
^ permalink raw reply
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Jeff King @ 2016-11-08 0:30 UTC (permalink / raw)
To: Bryan Turner; +Cc: Git Users, René Scharfe
In-Reply-To: <CAGyf7-HWAMF8S+Bw3wcwJCS1Subc28KHjpSCc1__0qn-GSMyvA@mail.gmail.com>
On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:
> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
> > }
> >
> > strbuf_add_absolute_path(&objdirbuf, get_object_directory());
> > - normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
> > + if (strbuf_normalize_path(&objdirbuf) < 0)
> > + die("unable to normalize object directory: %s",
> > + objdirbuf.buf);
>
> This appears to break the ability to use a relative alternate via an
> environment variable, since normalize_path_copy_len is explicitly
> documented "Returns failure (non-zero) if a ".." component appears as
> first path"
That shouldn't happen, though, because the path we are normalizing has
been converted to an absolute path via strbuf_add_absolute_path. IOW, if
your relative path is "../../../foo", we should be feeding something
like "/path/to/repo/.git/objects/../../../foo" and normalizing that to
"/path/to/foo".
But in your example, you see:
error: unable to normalize alternate object path: ../0/objects
which cannot come from the code above, which calls die(). It should be
coming from the call in link_alt_odb_entry().
I think what is happening is that relative paths via environment
variables have always been slightly broken, but happened to mostly work.
In prepare_alt_odb(), we call link_alt_odb_entries() with a NULL
relative_base. That function does two things with it:
- it may unconditionally dereference it for an error message, which
would cause a segfault. This is impossible to trigger in practice,
though, because the error message is related to the depth, which we
know will always be 0 here.
- we pass the NULL along to the singular link_alt_odb_entry().
That function only creates an absolute path if given a non-NULL
relative_base; otherwise we have always fed the path to
normalize_path_copy, which is nonsense for a relative path.
So normalize_path_copy() was _always_ returning an error there, but
we ignored it and used whatever happened to be left in the buffer
anyway. And because of the way normalize_path_copy() is implemented,
that happened to be the untouched original string in most cases. But
that's mostly an accident. I think it would not be for something
like "foo/../../bar", which is technically valid (if done from a
relative base that has at least one path component).
Moreover, it means we don't have an absolute path to our alternate
odb. So the path is taken as relative whenever we do an object
lookup, meaning it will behave differently between a bare repository
(where we chdir to $GIT_DIR) and one with a working tree (where we
are generally in the root of the working tree). It can even behave
differently in the same process if we chdir between object lookups.
So it did happen to work, but I'm not sure it was planned (and obviously
we have no test coverage for it). More on that below.
> Other commits, like [1], suggest the ability to use relative paths in
> alternates is something still actively developed and enhanced. Is it
> intentional that this breaks the ability to use relative alternates?
> If this is to be the "new normal", is there any other option when
> using environment variables besides using absolute paths?
No, I had no intention of disallowing relative alternates (and as you
noticed, a commit from the same series actually expands the use of
relative alternates). My use has been entirely within info/alternates
files, though, not via the environment.
As I said, I'm not sure this was ever meant to work, but as far as I can
tell it mostly _has_ worked, modulo some quirks. So I think we should
consider it a regression for it to stop working in v2.11.
The obvious solution is one of:
1. Stop calling normalize() at all when we do not have a relative base
and the path is not absolute. This restores the original quirky
behavior (plus makes the "foo/../../bar" case work).
If we want to do the minimum before releasing v2.11, it would be
that. I'm not sure it leaves things in a very sane state, but at
least v2.11 does no harm, and anybody who cares can build saner
semantics for v2.12.
2. Fix it for real. Pass a real relative_base when linking from the
environment. The question is: what is the correct relative base? I
suppose "getcwd() at the time we prepare the alt odb" is
reasonable, and would behave similarly to older versions ($GIT_DIR
for bare repos, top of the working tree otherwise).
If we were designing from scratch, I think saner semantics would
probably be always relative from $GIT_DIR, or even always relative
from the object directory (i.e., behave as if the paths were given
in objects/info/alternates). But that breaks compatibility with
older versions. If we are treating this as a regression, it is not
very friendly to say "you are still broken, but you might just need
to add an extra '..' to your path".
So I dunno. I guess that inclines me towards (1), as it lets us punt on
the harder question.
-Peff
^ permalink raw reply
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Bryan Turner @ 2016-11-07 23:42 UTC (permalink / raw)
To: Jeff King; +Cc: Git Users, René Scharfe
In-Reply-To: <20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> When we add a new alternate to the list, we try to normalize
> out any redundant "..", etc. However, we do not look at the
> return value of normalize_path_copy(), and will happily
> continue with a path that could not be normalized. Worse,
> the normalizing process is done in-place, so we are left
> with whatever half-finished working state the normalizing
> function was in.
>
<snip>
> @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
> }
>
> strbuf_add_absolute_path(&objdirbuf, get_object_directory());
> - normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
> + if (strbuf_normalize_path(&objdirbuf) < 0)
> + die("unable to normalize object directory: %s",
> + objdirbuf.buf);
This appears to break the ability to use a relative alternate via an
environment variable, since normalize_path_copy_len is explicitly
documented "Returns failure (non-zero) if a ".." component appears as
first path"
For example, when trying to run a rev-list over commits in two
repositories using GIT_ALTERNATE_OBJECT_DIRECTORIES, in 2.10.x and
prior the following command works. I know the alternate worked
previously because I'm passing a commit that does not exist in the
repository I'm running the command in; it only exists in a repository
linked by alternate, as shown by the "fatal: bad object" when the
alternates are rejected.
Before, using Git 2.7.4 (but I've verified this behavior through to
and including 2.10.2):
bturner@elysoun /tmp/1478561282706-0/shared/data/repositories/3 $
GIT_ALTERNATE_OBJECT_DIRECTORIES=../0/objects:../1/objects git
rev-list --format="%H" 2d8897c9ac29ce42c3442cf80ac977057045e7f6
74de5497dfca9731e455d60552f9a8906e5dc1ac
^6053a1eaa1c009dd11092d09a72f3c41af1b59ad
^017caf31eca7c46eb3d1800fcac431cfa7147a01 --
commit 74de5497dfca9731e455d60552f9a8906e5dc1ac
74de5497dfca9731e455d60552f9a8906e5dc1ac
commit 3528cf690cb37f6adb85b7bd40cc7a6118d4b598
3528cf690cb37f6adb85b7bd40cc7a6118d4b598
commit 2d8897c9ac29ce42c3442cf80ac977057045e7f6
2d8897c9ac29ce42c3442cf80ac977057045e7f6
commit 9c05f43f859375e392d90d23a13717c16d0fdcda
9c05f43f859375e392d90d23a13717c16d0fdcda
Now, using Git 2.11.0-rc0
bturner@elysoun /tmp/1478561282706-0/shared/data/repositories/3 $
GIT_ALTERNATE_OBJECT_DIRECTORIES=../0/objects:../1/objects
/opt/git/2.11.0-rc0/bin/git rev-list --format="%H"
2d8897c9ac29ce42c3442cf80ac977057045e7f6
74de5497dfca9731e455d60552f9a8906e5dc1ac
^6053a1eaa1c009dd11092d09a72f3c41af1b59ad
^017caf31eca7c46eb3d1800fcac431cfa7147a01 --
error: unable to normalize alternate object path: ../0/objects
error: unable to normalize alternate object path: ../1/objects
fatal: bad object 74de5497dfca9731e455d60552f9a8906e5dc1ac
Other commits, like [1], suggest the ability to use relative paths in
alternates is something still actively developed and enhanced. Is it
intentional that this breaks the ability to use relative alternates?
If this is to be the "new normal", is there any other option when
using environment variables besides using absolute paths?
Best regards,
Bryan Turner
[1]: https://github.com/git/git/commit/087b6d584062f5b704356286d6445bcc84d686fb
-- Also newly tagged in 2.11.0-rc0
^ permalink raw reply
* Re: Git issue - ignoring changes to tracked file with assume-unchanged
From: Jakub Narębski @ 2016-11-07 22:34 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Halde, Faiz, git@vger.kernel.org
In-Reply-To: <xmqqy413oysp.fsf@gitster.mtv.corp.google.com>
W dniu 01.11.2016 o 19:11, Junio C Hamano pisze:
> Jeff King <peff@peff.net> writes:
>> On Tue, Nov 01, 2016 at 10:28:57AM +0000, Halde, Faiz wrote:
>>
>>> I frequently use the following command to ignore changes done in a file
>>>
>>> git update-index --assume-unchanged somefile
>>>
>>> Now when I do a pull from my remote branch and say the file 'somefile'
>>> was changed locally and in remote, git will abort the merge saying I
>>> need to commit my changes of 'somefile'.
>>>
>>> But isn't the whole point of the above command to ignore the changes
>>> within the file?
>>
>> No. The purpose of --assume-unchanged is to promise git that you will
>> not change the file, so that it may skip checking the file contents in
>> some cases as an optimization.
>
> That's correct.
>
> The next anticipated question is "then how would I tell Git to
> ignore changes done to a file locally by me?", whose short answer is
> "You don't", of course.
Well, you can always use --skip-worktree. It is a better fit than using
--assume-unchanged, because at least you wouldn't loose your precious
local changes (which happened to me).
OTOH it doesn't solve your issue of --skip-worktree / --assume-unchanged
blocking operation (pull in your case, stash is what I noticed problem
with when using --skip-worktree).
But --skip-worktree is still workaround...
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 1/3] gitk: turn off undo manager in the text widget
From: Jacob Keller @ 2016-11-07 22:13 UTC (permalink / raw)
To: Markus Hitter; +Cc: git@vger.kernel.org, Paul Mackerras
In-Reply-To: <e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de>
On Mon, Nov 7, 2016 at 10:57 AM, Markus Hitter <mah@jump-ing.de> wrote:
> 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.
>
Wow. Nice find!
> 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.
>
Still, this is definitely the lions share of the memory issue.
Additionally, this fix seems much better overall and does not harm any
other aspects of gitk, because we only read the widget so there is as
you mentioned, zero reason to build an undo stack.
Thanks for taking the extra time to find a proper solution to this! I
think it makes perfect sense.
> 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
>
Nice that such a simple change results in a huge gain. I think this
makes perfect sense.
Regards,
Jake
^ permalink raw reply
* [PATCH v5 2/2] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-07 21:51 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, bburky, peff, jrnieder
In-Reply-To: <1478555462-132573-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 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 | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..5fe50bc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2308,6 +2308,52 @@ 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. 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
a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..c52cec8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,30 +1150,20 @@ 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:
-
- - `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
+ 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..02f49cb 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,129 @@ 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.allow=always clone --bare "$url" tmp.git &&
+ 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 v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Jeff King @ 2016-11-07 21:20 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, Torsten Bögershausen, git
In-Reply-To: <203BDCB2-1975-4590-B4B8-3C5E9D210430@gmail.com>
On Sun, Nov 06, 2016 at 10:42:36PM +0100, Lars Schneider wrote:
> > From: Lars Schneider <larsxschneider@gmail.com>
> >
> > TravisCI changed their default macOS image from 10.10 to 10.11 [1].
> > Unfortunately the HTTPD tests do not run out of the box using the
> > pre-installed Apache web server anymore. Therefore we enable these
> > tests only for Linux and disable them for macOS.
> [...]
> Hi Junio,
>
> the patch above is one of two patches to make TravisCI pass, again.
> Could you queue it?
I don't really mind disabling tests if they don't run on a platform. But
the more interesting question to me is: why don't they run any more? Is
there some config tweak needed, or is it an insurmountable problem?
Using Apache in the tests has been the source of frequent portability
problems and configuration headaches. I do wonder if we'd be better off
using some small special-purpose web server (even a short perl script
written around HTTP::Server::Simple or something).
On the other hand, testing against Apache approximates a more real-world
case, which has value. It might be nice if our tests supported multiple
web servers, but that would mean duplicating the config for each
manually.
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Jeff King @ 2016-11-07 21:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <20161107211010.xo3243egggdgscou@sigill.intra.peff.net>
On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:
> And I'll admit my main motivation is not that index/filesystem parity,
> but rather just that:
>
> git clone git://host.com/malicious-repo.git
> git log
>
> might create and read symlinks to arbitrary files on the cloner's box.
> I'm not sure to what degree to be worried about that. It's not like you
> can't make other arbitrary symlinks which are likely to be read if the
> user actually starts looking at checked-out files. It's just that we
> usually try to make a clone+log of a malicious repository safe.
Another approach is to have a config option to disallow symlinks to
destinations outside of the repository tree (I'm not sure if it should
be on or off by default, though).
Again, I don't know that there is a specific security issue, but it
makes things easier for services which might clone untrusted
repositories (e.g., things like CI). They'd obviously have to be careful
with the contents of the repositories anyway, but it's one less thing to
have to worry about.
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Jeff King @ 2016-11-07 21:10 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8AO2KtpxFu=wRjW1DoCA9bfpF1VoJUn__2ib-ML0XT66w@mail.gmail.com>
On Mon, Nov 07, 2016 at 05:03:42PM +0700, Duy Nguyen wrote:
> On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:
> > The attributes system may sometimes read in-tree files from
> > the filesystem, and sometimes from the index. In the latter
> > case, we do not resolve symbolic links (and are not likely
> > to ever start doing so). Let's open filesystem links with
> > O_NOFOLLOW so that the two cases behave consistently.
>
> This sounds backward to me. The major use case is reading
> .gitattributes on worktree, which follows symlinks so far. Only
> git-archive has a special need to read index-only versions. The
> worktree behavior should influence the in-index one, not the other way
> around. If we could die("BUG" when git-archive is used on symlinks
> (without --worktree-attributes). If people are annoyed by it, they can
> implement symlink folllowing (to another version in index, not on
> worktree).
I agree it feels a little backwards, as we are choosing the
lowest-common denominator of the two (so it would be reasonable to have
the in-index version follow symbolic links, or at least do so on
platforms where core.symlinks is true).
And I'll admit my main motivation is not that index/filesystem parity,
but rather just that:
git clone git://host.com/malicious-repo.git
git log
might create and read symlinks to arbitrary files on the cloner's box.
I'm not sure to what degree to be worried about that. It's not like you
can't make other arbitrary symlinks which are likely to be read if the
user actually starts looking at checked-out files. It's just that we
usually try to make a clone+log of a malicious repository safe.
That being said, I'm not convinced that reading the index version of
.gitattributes and .gitignore is just an optimization. Don't we read the
destination attributes when checking out a new tree? And doesn't merge
need to use the in-index version when we see conflicts?
So I was hoping that this was a practice that is unlikely to be in wide
use, and that we could simply ban in order to keep the attribute and
ignore code simpler and safer, both now and if we change them to do more
in-index lookups.
-Peff
^ permalink raw reply
* Re: [PATCH v4 2/2] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-07 21:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161107204430.z6wrazgad4e7yn66@sigill.intra.peff.net>
On 11/07, Jeff King wrote:
> > + 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.
I'll add in another test for that, no reason not to test it.
>
> Squashable documentation suggestions are below.
>
Sounds good
--
Brandon Williams
^ permalink raw reply
* Re: git push remote syntax
From: Jeff King @ 2016-11-07 21:01 UTC (permalink / raw)
To: Diggory Hardy; +Cc: git
In-Reply-To: <1613741.x6i0st30av@localhost.localdomain>
On Mon, Nov 07, 2016 at 01:49:40PM +0000, Diggory Hardy wrote:
> One thing I find a little frustrating about git is that the syntax needed
> differs by command. I wish the 'remote/branch' syntax was more universal:
The reason it's not is that "remote/branch" refers to a branch in your
local repository. Whereas fetch/push want a single remote, and then one
or more refspecs. They often _look_ the same in simple cases, but the
latter covers a lot of cases not handled by the former.
For example:
# no configured remote nor remote tracking branch at all
git pull git://host/repo.git master
# multiple branches for an octopus merge
git pull origin branchA branchB
# refspecs
git pull origin devel:tmp
It's possible that we could have some kind of do-what-I-mean syntax for
the command-line options, though. It wouldn't have to cover every
esoteric case, but could cover the common ones and expand into the more
complete syntax. E.g., if we made:
git pull origin/master
behave as if you said:
git pull origin master
that would cover many uses. There are still some corner cases, though:
- you could have a remote with a slash in it; presumably we would
check that first and fallback to the DWIM behavior
- These commands only handle a single remote at once, so something
like:
git pull origin/foo other-remote/bar
is nonsensical. We'd have to catch and disallow multiple remotes.
Probably we could only kick in the DWIM when there is a single
argument (otherwise you're just repeating the remote name over and
over, at which point you might as well use the "remote [refspec...]"
syntax.
It seems like it's probably do-able.
I'm still undecided on whether it is a good idea or not. In one sense,
it does unify the syntax you use to refer to a remote branch. But it
also blurs the meanings. Normally "origin/master" refers only to your
local refs/remotes copy of what is on the remote, but this is blurring
the line. It's not clear to me if that reduces confusion (because you
don't have to care about that line anymore), or if it increases it
(because sometimes it _does_ matter, and somebody who doesn't learn the
difference between the two will get bitten later. Plus now there are
multiple ways of spelling the same thing).
> > git pull myremote/somebranch
> complains about the syntax; IMO it should either pull from that branch (and
> merge if necessary) or complain instead that pulling from a different branch
> is not supported (and suggest using merge).
Reading this, I wonder if I've misinterpreted your request. It sounds
like you want this to be the same as:
git merge myremote/somebranch
which is at least consistent in the use of "remote/branch" syntax. But
weird, because you're asking "pull" not to pull. Or another way to think
of it is that "git pull foo/bar" effectively becomes "git pull .
foo/bar". Which seems like it may be potentially error-prone, especially
if you use slashes in your remote names.
-Peff
^ permalink raw reply
* 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
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