* [PATCH] t3600: remove useless redirect
From: Stefan Beller @ 2016-12-02 20:05 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, Stefan Beller
In the next line the `actual` is overwritten again, so no need to redirect
the output of checkout into that file.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t3600-rm.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 14f0edca2b..6e587f956f 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -709,7 +709,7 @@ test_expect_success 'checking out a commit after submodule removal needs manual
git commit -m "submodule removal" submod &&
git checkout HEAD^ &&
git submodule update &&
- git checkout -q HEAD^ 2>actual &&
+ git checkout -q HEAD^ &&
git checkout -q master 2>actual &&
test_i18ngrep "^warning: unable to rmdir submod:" actual &&
git status -s submod >actual &&
--
2.10.2.613.g22f2156
^ permalink raw reply related
* Re: EXT: Re: "Your branch is ahead of 'origin' by X commits"
From: Alfonsogonzalez, Ernesto (GE Digital) @ 2016-12-02 18:35 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git@vger.kernel.org
Hi Matthieu,
It was my mistake, I had a local branch called “origin” which was the
upstream for master.
I sent more details in a later email, reproduced below. Sorry for the
false alarm.
Thanks,
Ernesto
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>
Yes, it looks like I had a local branch “origin” which was behind by 108
commits.
Setting upstream to the local branch correctly states "track local branch
origin”.
It was my mistake, there is no bug.
Thanks,
$ git rev-parse --symbolic-full-name origin
refs/heads/origin
# origin is a local branch
$ git show refs/heads/origin
commit ad8c3ee6cb7740627e4ecddb418c826bc8597d3d # old commit, 108 commits
behind master
$ git branch
...
* master
...
origin
...
$ git show origin
commit ad8c3ee6cb7740627e4ecddb418c826bc8597d3d
Merge: e16bda3 4b7564d
$ git branch --set-upstream-to=origin/master
Branch master set up to track remote branch master from origin
$ git branch --set-upstream-to=origin #correctly says "track local branch
origin"
Branch master set up to track local branch origin.
$ git status
On branch master
Your branch is ahead of 'origin' by 108 commits.
(use "git push" to publish your local commits)
Untracked files:
(use "git add <file>..." to include in what will be committed)
...
nothing added to commit but untracked files present (use "git add" to
track)
$ git branch -d origin
Deleted branch origin (was ad8c3ee).
$
$ git status
On branch master
Your branch is based on 'origin', but the upstream is gone.
(use "git branch --unset-upstream" to fixup)
On 12/2/16, 5:42 AM, "Matthieu Moy" <Matthieu.Moy@grenoble-inp.fr> wrote:
>"Alfonsogonzalez, Ernesto (GE Digital)" <ernesto.alfonsogonzalez@ge.com>
>writes:
>
>> Hi,
>>
>> Git status tells me "Your branch is ahead of 'origin' by 108 commits.²,
>> but my local and origin/master are pointing to the same commit.
>>
>> What am I doing wrong?
>>
>> $ git diff origin/master
>> $ git status
>> On branch master
>> Your branch is ahead of 'origin' by 108 commits.
>
>This line should say "ahead of 'origin/master'" in common setups, where
>'origin/master' is the remote-tracking branch configured as upstream for
>'master'.
>
>My guess is that you have a badly configured upstream.
>
>What does "git pull -v" say? What's the content of the [branch "master"]
>section of .git/config?
>
>--
>Matthieu Moy
>https://urldefense.proofpoint.com/v2/url?u=http-3A__www-2Dverimag.imag.fr_
>-7Emoy_&d=DQIDAw&c=IV_clAzoPDE253xZdHuilRgztyh_RiV3wUrLrDQYWSI&r=izgsnUPc7
>Qu6cAMH0HZnmitAbT-LmaLVojOu8tH2KqE&m=0Kz9dpPPuw6T-zLqzY80QygN-4VoMVQeauR_x
>WR5fN8&s=DnD-WMGSm71Y05CEheJw-NdYA0FVP2ze6KqWxrEKswE&e=
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Stefan Beller @ 2016-12-02 19:28 UTC (permalink / raw)
To: Jacob Keller
Cc: Brandon Williams, Jeff King, Junio C Hamano, git@vger.kernel.org,
Jonathan Tan
In-Reply-To: <CA+P7+xoi_UNSv-dKVFWf7T4o3uBtGa0wR6=nnYX+kjzLAmw6ug@mail.gmail.com>
On Fri, Dec 2, 2016 at 11:20 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>
>> So is there a reason why the library function realpath() can't be used?
>> From a cursory look at its man page it seems to do the symlink
>> resolution.
>>
>> --
>> Brandon Williams
>
> I believe it uses the same method and thus wouldn't actually resolve
> the issue. But I'm not really 100% sure on this.
>
> Thanks,
> Jake
I just reviewed 2 libc implementations (glibc and an Android libc) and
both of them
do not use chdir internally, but use readlink and compose the path 'manually'
c.f. http://osxr.org:8080/glibc/source/stdlib/canonicalize.c?v=glibc-2.13
^ permalink raw reply
* RE: [PATCH] unpack-trees: fix grammar for untracked files in directories
From: David Turner @ 2016-12-02 19:23 UTC (permalink / raw)
To: 'Stefan Beller', gitster@pobox.com; +Cc: git@vger.kernel.org
In-Reply-To: <20161202191741.12693-1-sbeller@google.com>
LGTM.
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 2:18 PM
> To: gitster@pobox.com
> Cc: git@vger.kernel.org; David Turner; Stefan Beller
> Subject: [PATCH] unpack-trees: fix grammar for untracked files in
> directories
>
> Noticed-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This was noticed by David when reviewing the submodule checkout series,
> though rolling this as an independent fix is better :)
>
> Thanks,
> Stefan
>
> t/t7609-merge-co-error-msgs.sh | 2 +-
> unpack-trees.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-
> msgs.sh index f80bdb81e1..e90413204e 100755
> --- a/t/t7609-merge-co-error-msgs.sh
> +++ b/t/t7609-merge-co-error-msgs.sh
> @@ -105,7 +105,7 @@ test_expect_success 'not uptodate file porcelain
> checkout error' '
> '
>
> cat >expect <<\EOF
> -error: Updating the following directories would lose untracked files in
> it:
> +error: Updating the following directories would lose untracked files in
> them:
> rep
> rep2
>
> diff --git a/unpack-trees.c b/unpack-trees.c index ea6bdd20e0..7a6df99d10
> 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -78,7 +78,7 @@ void setup_unpack_trees_porcelain(struct
> unpack_trees_options *opts,
> xstrfmt(msg, cmd, cmd);
>
> msgs[ERROR_NOT_UPTODATE_DIR] =
> - _("Updating the following directories would lose untracked
> files in it:\n%s");
> + _("Updating the following directories would lose untracked
> files in
> +them:\n%s");
>
> if (!strcmp(cmd, "checkout"))
> msg = advice_commit_before_merge
> --
> 2.10.2.613.g22f2156
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jacob Keller @ 2016-12-02 19:20 UTC (permalink / raw)
To: Brandon Williams
Cc: Jeff King, Stefan Beller, Junio C Hamano, git@vger.kernel.org,
Jonathan Tan
In-Reply-To: <20161202184944.GC117792@google.com>
On Fri, Dec 2, 2016 at 10:49 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/02, Jacob Keller wrote:
>> On Fri, Dec 2, 2016 at 10:36 AM, Brandon Williams <bmwill@google.com> wrote:
>> > On 12/01, Jeff King wrote:
>> >> On Thu, Dec 01, 2016 at 01:56:32PM -0800, Stefan Beller wrote:
>> >>
>> >> > > Bleh. Looks like it happens as part of the recently-added
>> >> > > get_common_dir(). I'm not sure if that is ever relevant for submodules,
>> >> > > but I guess in theory you could have a submodule clone that is part of a
>> >> > > worktree?
>> >> >
>> >> > Sure we can, for a test that we don't have that, see the embedgitdirs series. ;)
>> >> >
>> >> > For now each submodule has its own complete git dir, but the vision
>> >> > would be to have a common git dir for submodules in the common
>> >> > superprojects git dir as well, such that objects are shared actually. :)
>> >>
>> >> Fair enough. Given that it seems to behave OK even in error cases, the
>> >> simple stat() test may be the best option, then. I do think we should
>> >> consider adding a few test cases to make sure it continues to behave in
>> >> the error cases (just because we are relying partially on what git's
>> >> setup code happens to do currently, and we'd want to protect ourselves
>> >> against regressions).
>> >
>> > For the naive (ie me), is there a reason why real_path() couldn't be
>> > re-implemented to avoid using chdir? I tried looking into the history of
>> > the function but couldn't find anything explaining why it was done that
>> > way. I assume it has to do with symlinks, but I thought there was a
>> > syscall (readlink?) that could do the resolution.
>> >
>> > --
>> > Brandon Williams
>>
>> The reason as far as I understand it, is that it uses chdir() to
>> guarantee that it follows symlinks correctly and then looks up the
>> resulting path after the chdir(). I do not think there is a syscall
>> that actually correctly works like real_path() does. You *could*
>> re-write real_path() to do the symlink lookups itself, but as Jeff
>> recently pointed out, that way lies madness.
>
> So is there a reason why the library function realpath() can't be used?
> From a cursory look at its man page it seems to do the symlink
> resolution.
>
> --
> Brandon Williams
I believe it uses the same method and thus wouldn't actually resolve
the issue. But I'm not really 100% sure on this.
Thanks,
Jake
^ permalink raw reply
* [PATCH] unpack-trees: fix grammar for untracked files in directories
From: Stefan Beller @ 2016-12-02 19:17 UTC (permalink / raw)
To: gitster; +Cc: git, dturner, Stefan Beller
Noticed-by: David Turner <dturner@twosigma.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This was noticed by David when reviewing the submodule checkout series,
though rolling this as an independent fix is better :)
Thanks,
Stefan
t/t7609-merge-co-error-msgs.sh | 2 +-
unpack-trees.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh
index f80bdb81e1..e90413204e 100755
--- a/t/t7609-merge-co-error-msgs.sh
+++ b/t/t7609-merge-co-error-msgs.sh
@@ -105,7 +105,7 @@ test_expect_success 'not uptodate file porcelain checkout error' '
'
cat >expect <<\EOF
-error: Updating the following directories would lose untracked files in it:
+error: Updating the following directories would lose untracked files in them:
rep
rep2
diff --git a/unpack-trees.c b/unpack-trees.c
index ea6bdd20e0..7a6df99d10 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -78,7 +78,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
xstrfmt(msg, cmd, cmd);
msgs[ERROR_NOT_UPTODATE_DIR] =
- _("Updating the following directories would lose untracked files in it:\n%s");
+ _("Updating the following directories would lose untracked files in them:\n%s");
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
--
2.10.2.613.g22f2156
^ permalink raw reply related
* [PATCH v2] diff: handle --no-abbrev outside of repository
From: Jack Bates @ 2016-12-02 18:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161129070637.eult6o3m34r2mima@sigill.intra.peff.net>
The "git diff --no-index" codepath didn't handle the --no-abbrev option.
Also it didn't behave the same as find_unique_abbrev()
in the case where abbrev == 0.
find_unique_abbrev() returns the full, unabbreviated string in that
case, but the "git diff --no-index" codepath returned an empty string.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-12-02 18:49 UTC (permalink / raw)
To: Jacob Keller
Cc: Jeff King, Stefan Beller, Junio C Hamano, git@vger.kernel.org,
Jonathan Tan
In-Reply-To: <CA+P7+xpoO=ieRyQb8r8Xz12nN10f53LKeVMgAO8XPdSwvG7fuA@mail.gmail.com>
On 12/02, Jacob Keller wrote:
> On Fri, Dec 2, 2016 at 10:36 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/01, Jeff King wrote:
> >> On Thu, Dec 01, 2016 at 01:56:32PM -0800, Stefan Beller wrote:
> >>
> >> > > Bleh. Looks like it happens as part of the recently-added
> >> > > get_common_dir(). I'm not sure if that is ever relevant for submodules,
> >> > > but I guess in theory you could have a submodule clone that is part of a
> >> > > worktree?
> >> >
> >> > Sure we can, for a test that we don't have that, see the embedgitdirs series. ;)
> >> >
> >> > For now each submodule has its own complete git dir, but the vision
> >> > would be to have a common git dir for submodules in the common
> >> > superprojects git dir as well, such that objects are shared actually. :)
> >>
> >> Fair enough. Given that it seems to behave OK even in error cases, the
> >> simple stat() test may be the best option, then. I do think we should
> >> consider adding a few test cases to make sure it continues to behave in
> >> the error cases (just because we are relying partially on what git's
> >> setup code happens to do currently, and we'd want to protect ourselves
> >> against regressions).
> >
> > For the naive (ie me), is there a reason why real_path() couldn't be
> > re-implemented to avoid using chdir? I tried looking into the history of
> > the function but couldn't find anything explaining why it was done that
> > way. I assume it has to do with symlinks, but I thought there was a
> > syscall (readlink?) that could do the resolution.
> >
> > --
> > Brandon Williams
>
> The reason as far as I understand it, is that it uses chdir() to
> guarantee that it follows symlinks correctly and then looks up the
> resulting path after the chdir(). I do not think there is a syscall
> that actually correctly works like real_path() does. You *could*
> re-write real_path() to do the symlink lookups itself, but as Jeff
> recently pointed out, that way lies madness.
So is there a reason why the library function realpath() can't be used?
From a cursory look at its man page it seems to do the symlink
resolution.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jacob Keller @ 2016-12-02 18:44 UTC (permalink / raw)
To: Brandon Williams
Cc: Jeff King, Stefan Beller, Junio C Hamano, git@vger.kernel.org,
Jonathan Tan
In-Reply-To: <20161202183622.GB117792@google.com>
On Fri, Dec 2, 2016 at 10:36 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/01, Jeff King wrote:
>> On Thu, Dec 01, 2016 at 01:56:32PM -0800, Stefan Beller wrote:
>>
>> > > Bleh. Looks like it happens as part of the recently-added
>> > > get_common_dir(). I'm not sure if that is ever relevant for submodules,
>> > > but I guess in theory you could have a submodule clone that is part of a
>> > > worktree?
>> >
>> > Sure we can, for a test that we don't have that, see the embedgitdirs series. ;)
>> >
>> > For now each submodule has its own complete git dir, but the vision
>> > would be to have a common git dir for submodules in the common
>> > superprojects git dir as well, such that objects are shared actually. :)
>>
>> Fair enough. Given that it seems to behave OK even in error cases, the
>> simple stat() test may be the best option, then. I do think we should
>> consider adding a few test cases to make sure it continues to behave in
>> the error cases (just because we are relying partially on what git's
>> setup code happens to do currently, and we'd want to protect ourselves
>> against regressions).
>
> For the naive (ie me), is there a reason why real_path() couldn't be
> re-implemented to avoid using chdir? I tried looking into the history of
> the function but couldn't find anything explaining why it was done that
> way. I assume it has to do with symlinks, but I thought there was a
> syscall (readlink?) that could do the resolution.
>
> --
> Brandon Williams
The reason as far as I understand it, is that it uses chdir() to
guarantee that it follows symlinks correctly and then looks up the
resulting path after the chdir(). I do not think there is a syscall
that actually correctly works like real_path() does. You *could*
re-write real_path() to do the symlink lookups itself, but as Jeff
recently pointed out, that way lies madness.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-12-02 18:36 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org, Jonathan Tan
In-Reply-To: <20161201215934.g7dt5ioekmx6ssii@sigill.intra.peff.net>
On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 01:56:32PM -0800, Stefan Beller wrote:
>
> > > Bleh. Looks like it happens as part of the recently-added
> > > get_common_dir(). I'm not sure if that is ever relevant for submodules,
> > > but I guess in theory you could have a submodule clone that is part of a
> > > worktree?
> >
> > Sure we can, for a test that we don't have that, see the embedgitdirs series. ;)
> >
> > For now each submodule has its own complete git dir, but the vision
> > would be to have a common git dir for submodules in the common
> > superprojects git dir as well, such that objects are shared actually. :)
>
> Fair enough. Given that it seems to behave OK even in error cases, the
> simple stat() test may be the best option, then. I do think we should
> consider adding a few test cases to make sure it continues to behave in
> the error cases (just because we are relying partially on what git's
> setup code happens to do currently, and we'd want to protect ourselves
> against regressions).
For the naive (ie me), is there a reason why real_path() couldn't be
re-implemented to avoid using chdir? I tried looking into the history of
the function but couldn't find anything explaining why it was done that
way. I assume it has to do with symlinks, but I thought there was a
syscall (readlink?) that could do the resolution.
--
Brandon Williams
^ permalink raw reply
* git 2.11.0 error when pushing to remote located on a windows share
From: thomas.attwood @ 2016-12-02 18:02 UTC (permalink / raw)
To: git
After updating git from 2.10.0 to 2.11.0 when trying to push any changes to a repo located in a windows share, the following error occurs:
$ git push origin test
Counting objects: 2, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (2/2), 284 bytes | 0 bytes/s, done.
Total 2 (delta 1), reused 1 (delta 0)
remote: error: object directory /path/to/dir/objects does not exist; check .git/objects/info/alternates.
remote: fatal: unresolved deltas left after unpacking
error: unpack failed: unpack-objects abnormal exit
To //path/to/dir
! [remote rejected] test -> test (unpacker error)
error: failed to push some refs to '//path/to/dir'
No error occurs if pushing to the same repo (a direct copy into a local directory) using 2.11.0.
$ git push local_test test
Counting objects: 2, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (2/2), 284 bytes | 0 bytes/s, done.
Total 2 (delta 1), reused 1 (delta 0)
To C:/path/to/dir
* [new branch] test -> test
---
git remotes:
$ git remote -v
local_test C:/path/to/dir (fetch)
local_test C:/path/to/dir (push)
origin //path/to/dir (fetch)
origin //path/to/dir (push)
---
Using `git fsck --full` in both 2.11.0 and 2.10.0, it doesn't reveal any additional problems.
$ git fsck --full
Checking object directories: 100% (256/256), done.
Checking objects: 100% (2710/2710), done.
---
I'm not sure if it's related but ` object directory /path/to/dir/objects does not exist` doesn't contain an expected second slash.
Thomas 'Panda' Attwood.
^ permalink raw reply
* Where is Doc to configure Git + Apache + kerberos for Project level access in repo?
From: ken edward @ 2016-12-02 18:15 UTC (permalink / raw)
To: git
Where is Doc to configure Git + Apache + kerberos for Project level
access in repo?
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-02 17:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161202001353.jiw4hjqg75dr6psw@sigill.intra.peff.net>
On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:
>
> > > I started taking a look at your http redirect series (I really should
> > > have taking a look at it sooner) and I see exactly what you're talking
> > > about. We can easily move this logic into a function to make it easier
> > > to generate the two whitelists.
> >
> > Thinking about this some more...I was told that having http redirect to
> > file:// could be scary. The way the new protocol configuration is setup
> > we have file:// as a default known-safe protocol. Do we need to worry
> > about this or can we leave this be since this can be overridden by the
> > user?
>
> Hmm. I'm not sure if file:// should actually be USER_ONLY. The submodule
> code allows it, and it's certainly a convenience, but I guess you could
> do tricky things by probing somebody's filesystem with submodules URLs.
> On the other hand, if you are recursively cloning untrusted repos and
> have sensitive contents on disk, you really _should_ be setting up a
> protocol whitelist.
>
> For HTTP redirects within curl, I think it's a non-issue; curl
> automatically disallows file:// for redirects, even without us telling
> it so.
>
> For redirects via http-alternates, it's a bit more tricky, as we feed
> the URL to curl ourselves, so it can't tell the difference between
> trusted and untrusted input. The main protection provided by my series
> is "don't follow http-alternates at all". But assuming you did want to
> use them (by setting http.followRedirects to "true", at least for the
> server in question), we could then feed file:// directly to curl. But I
> think we are still OK, because the restricted CURLOPT_PROTOCOL setting
> would prevent that from working. I.e., git _never_ wants curl to handle
> file://, because it handles it without calling into remote-curl.c at
> all.
>
> So arguably file:// should be USER_ONLY, but I'm not sure how much it
> matters in practice.
Ah ok thanks for the good explanation. I was mostly interested in the
http redirect case which, as you said, becomes a non-issue due to how we
configure curl.
Thanks!
--
Brandon Williams
^ permalink raw reply
* Re: Error after calling git difftool -d with
From: Johannes Schindelin @ 2016-12-02 16:05 UTC (permalink / raw)
To: P. Duijst; +Cc: git
In-Reply-To: <5f630c90-cf54-3a23-c9a9-af035d4514e0@gmail.com>
Hi Peter,
On Fri, 2 Dec 2016, P. Duijst wrote:
> Incase filenames are used with a quote ' or a bracket [ (and maybe some more
> characters), git "diff" and "difftool -y" works fine, but git *difftool **-d*
> gives the next error message:
>
> peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> $ git diff
> diff --git a/Test ''inch.txt b/Test ''inch.txt
> index dbff793..41f3257 100644
> --- a/Test ''inch.txt
> +++ b/Test ''inch.txt
> @@ -1 +1,3 @@
> +
> +ddd
> Test error in simple repository
> warning: LF will be replaced by CRLF in Test ''inch.txt.
> The file will have its original line endings in your working directory.
>
> peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> *$ git difftool -d*
> *fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
> directory*
> *hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*
>
> peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> $
>
>
> This issue is inside V2.10.x and V2.11.0.
> V2.9.0 is working correctly...
You say v2.11.0, but did you also try the new, experimental builtin
difftool? You can test without reinstalling:
git -c difftool.useBuiltin=true difftool -d ...
Ciao,
Johannes
^ permalink raw reply
* Re: "Your branch is ahead of 'origin' by X commits"
From: Matthieu Moy @ 2016-12-02 13:42 UTC (permalink / raw)
To: Alfonsogonzalez, Ernesto (GE Digital); +Cc: git@vger.kernel.org
In-Reply-To: <D465BDE6.B7DE%ernesto.alfonsogonzalez@ge.com>
"Alfonsogonzalez, Ernesto (GE Digital)" <ernesto.alfonsogonzalez@ge.com>
writes:
> Hi,
>
> Git status tells me "Your branch is ahead of 'origin' by 108 commits.²,
> but my local and origin/master are pointing to the same commit.
>
> What am I doing wrong?
>
> $ git diff origin/master
> $ git status
> On branch master
> Your branch is ahead of 'origin' by 108 commits.
This line should say "ahead of 'origin/master'" in common setups, where
'origin/master' is the remote-tracking branch configured as upstream for
'master'.
My guess is that you have a badly configured upstream.
What does "git pull -v" say? What's the content of the [branch "master"]
section of .git/config?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work
From: Torsten Bögershausen @ 2016-12-02 12:20 UTC (permalink / raw)
To: Junio C Hamano, Jacob Keller; +Cc: Git mailing list, eevee.reply
In-Reply-To: <xmqqtwan8kjv.fsf@gitster.mtv.corp.google.com>
> Yup, this is what I queued.
>
Looks good, thanks you all.
^ permalink raw reply
* Error after calling git difftool -d with
From: P. Duijst @ 2016-12-02 12:15 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]
Dear,
Incase filenames are used with a quote ' or a bracket [ (and maybe some
more characters), git "diff" and "difftool -y" works fine, but git
*difftool **-d* gives the next error message:
peter@scm_ws_10 MINGW64 /d/Dev/test (master)
$ git diff
diff --git a/Test ''inch.txt b/Test ''inch.txt
index dbff793..41f3257 100644
--- a/Test ''inch.txt
+++ b/Test ''inch.txt
@@ -1 +1,3 @@
+
+ddd
Test error in simple repository
warning: LF will be replaced by CRLF in Test ''inch.txt.
The file will have its original line endings in your working directory.
peter@scm_ws_10 MINGW64 /d/Dev/test (master)
*$ git difftool -d*
*fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
directory*
*hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*
peter@scm_ws_10 MINGW64 /d/Dev/test (master)
$
This issue is inside V2.10.x and V2.11.0.
V2.9.0 is working correctly...
I am using git for windows and beyond compare 4.0 as difftool.
Best regards,
Peter
[-- Attachment #2: IssueWithFiles.zip --]
[-- Type: application/zip, Size: 13922 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion
From: Austin English @ 2016-12-02 1:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqfum78jq0.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 1, 2016 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Austin English <austinenglish@gmail.com> writes:
>
>> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
>> index ed8b4ff..5fb2dc5 100755
>> --- a/Documentation/install-webdoc.sh
>> +++ b/Documentation/install-webdoc.sh
>> @@ -18,7 +18,7 @@ do
>> else
>> echo >&2 "# install $h $T/$h"
>> rm -f "$T/$h"
>> - mkdir -p $(dirname "$T/$h")
>> + mkdir -p "$(dirname "$T/$h")"
>> cp "$h" "$T/$h"
>> fi
>> done
>
> We know $h is safe without quoting (see what the for loop iterates
> over a list and binding each element of it to this variable), but T
> is the parameter given to this script, which comes from these
>
> install-html: html
> '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
>
> install-webdoc : html
> '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)
>
> in the Makefile. So quoting the result of $(dirname "$T/$h") is
> just as necessary as quoting the argument given to this dirname.
>
> But I do not think it is sufficient, if we are truly worried about
> people who specify a path that contains IFS whitespace in DESTDIR,
> WEBDOC_DEST, htmldir and other *dir variables used in the Makefile.
> The references to these variables, when they are mentioned on the
> command lines of Makefile actions, all need to be quoted. The
> remainder of the Makefile tells me that we decided that we are not
> worried about those people at all.
>
> So while I could take your patch as-is, I am not sure how much value
> it adds to the overall callchain that would reach the location that
> is updated by the patch. If you run
>
> make DESTDIR="/tmp/My Temporary Place" install
>
> it would still not do the right thing even with your patch, I would
> suspect.
>
> Thanks.
Hi Junio,
Thanks for reply and reviewing. Your concerns are totally valid.
Some context for the change. I wrote a wrapper script for
checkbashisms/shellcheck that I use in my project. I decided to run it
on other projects I have checked out, out of curiosity, and looked at
some of the results. This was the only one in git, so I thought it was
worth fixing. I did not test the full pipeline.
I'll look again and send a follow up patch soon.
--
-Austin
GPG: 14FB D7EA A041 937B
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Ramsay Jones @ 2016-12-02 1:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Philip Oakley, Brandon Williams, git, Jann Horn
In-Reply-To: <20161202001852.arasy44d6iczeeez@sigill.intra.peff.net>
On 02/12/16 00:18, Jeff King wrote:
> On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:
>
>>>> In a British context "Mallory and Irvine" were two (male) climbers who
>>>> died on Everest in 1924 (tales of daring...), so it's easy to expect
>>>> (from this side of the pond) that 'Mallory' would be male. However he
>>>> was really George Mallory.
>>>>
>>>> Meanwhile that search engine's images shows far more female Mallorys,
>>>> so I've learnt something.
>>>
>>> "baby name Mallory" in search engine gave me several sites, most of
>>> them telling me that is a girl's name except for one.
>>>
>>> Didn't think of doing image search, but that's a good way ;-)
>>
>> Heh, I didn't think about any of this. I was remembering the
>> description of 'Man-in-the-middle Attack' from Applied Cryptography
>> (Bruce Schneier) which implies that Mallory is male.
>
> I admit that I always assumed Applied Cryptography (and other papers)
> were always talking about a female. But that's probably because I
> started with an assumption about the name in the first place. That
> probably came from watching the Family Ties sitcom as a kid; the older
> daughter is named Mallory (and if you google it, you can see some
> amazing 80's haircuts and clothes).
>
> We can call her Marsha if you want, instead evoking Brady Bunch memories
> of 60's clothing and haircuts.
I'm not sure it matters too much, but if you are going to
change the name then Eve is also used in the description
of Man-in-the-middle (see "Practical Cryptography", Ferguson
and Schneier).
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-02 0:13 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161201232656.GK54082@google.com>
On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:
> > I started taking a look at your http redirect series (I really should
> > have taking a look at it sooner) and I see exactly what you're talking
> > about. We can easily move this logic into a function to make it easier
> > to generate the two whitelists.
>
> Thinking about this some more...I was told that having http redirect to
> file:// could be scary. The way the new protocol configuration is setup
> we have file:// as a default known-safe protocol. Do we need to worry
> about this or can we leave this be since this can be overridden by the
> user?
Hmm. I'm not sure if file:// should actually be USER_ONLY. The submodule
code allows it, and it's certainly a convenience, but I guess you could
do tricky things by probing somebody's filesystem with submodules URLs.
On the other hand, if you are recursively cloning untrusted repos and
have sensitive contents on disk, you really _should_ be setting up a
protocol whitelist.
For HTTP redirects within curl, I think it's a non-issue; curl
automatically disallows file:// for redirects, even without us telling
it so.
For redirects via http-alternates, it's a bit more tricky, as we feed
the URL to curl ourselves, so it can't tell the difference between
trusted and untrusted input. The main protection provided by my series
is "don't follow http-alternates at all". But assuming you did want to
use them (by setting http.followRedirects to "true", at least for the
server in question), we could then feed file:// directly to curl. But I
think we are still OK, because the restricted CURLOPT_PROTOCOL setting
would prevent that from working. I.e., git _never_ wants curl to handle
file://, because it handles it without calling into remote-curl.c at
all.
So arguably file:// should be USER_ONLY, but I'm not sure how much it
matters in practice.
-Peff
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Jeff King @ 2016-12-02 0:18 UTC (permalink / raw)
To: Ramsay Jones
Cc: Junio C Hamano, Philip Oakley, Brandon Williams, git, Jann Horn
In-Reply-To: <7cd35131-2e0c-0dff-8864-c099e313251d@ramsayjones.plus.com>
On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:
> >> In a British context "Mallory and Irvine" were two (male) climbers who
> >> died on Everest in 1924 (tales of daring...), so it's easy to expect
> >> (from this side of the pond) that 'Mallory' would be male. However he
> >> was really George Mallory.
> >>
> >> Meanwhile that search engine's images shows far more female Mallorys,
> >> so I've learnt something.
> >
> > "baby name Mallory" in search engine gave me several sites, most of
> > them telling me that is a girl's name except for one.
> >
> > Didn't think of doing image search, but that's a good way ;-)
>
> Heh, I didn't think about any of this. I was remembering the
> description of 'Man-in-the-middle Attack' from Applied Cryptography
> (Bruce Schneier) which implies that Mallory is male.
I admit that I always assumed Applied Cryptography (and other papers)
were always talking about a female. But that's probably because I
started with an assumption about the name in the first place. That
probably came from watching the Family Ties sitcom as a kid; the older
daughter is named Mallory (and if you google it, you can see some
amazing 80's haircuts and clothes).
We can call her Marsha if you want, instead evoking Brady Bunch memories
of 60's clothing and haircuts.
-Peff
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Ramsay Jones @ 2016-12-02 0:07 UTC (permalink / raw)
To: Junio C Hamano, Philip Oakley; +Cc: Brandon Williams, Jeff King, git, Jann Horn
In-Reply-To: <xmqq8trz6wrq.fsf@gitster.mtv.corp.google.com>
On 01/12/16 23:43, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>> Depends, I only know Mallorys who are women so her seems appropriate.
>>>
>> In a British context "Mallory and Irvine" were two (male) climbers who
>> died on Everest in 1924 (tales of daring...), so it's easy to expect
>> (from this side of the pond) that 'Mallory' would be male. However he
>> was really George Mallory.
>>
>> Meanwhile that search engine's images shows far more female Mallorys,
>> so I've learnt something.
>
> "baby name Mallory" in search engine gave me several sites, most of
> them telling me that is a girl's name except for one.
>
> Didn't think of doing image search, but that's a good way ;-)
Heh, I didn't think about any of this. I was remembering the
description of 'Man-in-the-middle Attack' from Applied Cryptography
(Bruce Schneier) which implies that Mallory is male.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 5/6] http: treat http-alternates like redirects
From: Jeff King @ 2016-12-02 0:06 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, Jann Horn
In-Reply-To: <20161201230223.GI54082@google.com>
On Thu, Dec 01, 2016 at 03:02:23PM -0800, Brandon Williams wrote:
> > diff --git a/http.c b/http.c
> > index 825118481..051fe6e5a 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)
> > if (is_transport_allowed("ftps"))
> > allowed_protocols |= CURLPROTO_FTPS;
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> > + curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
> > #else
> > if (transport_restrict_protocols())
> > warning("protocol restrictions not applied to curl redirects because\n"
>
> Because I don't know much about how curl works....Only
> http/https/ftp/ftps protocols are allowed to be passed to curl? Is that
> because curl only understands those particular protocols?
No, curl understands more protocols, and that is exactly the problem. We
don't want to accidentally have curl access file://, smtp://, or
similar, based on what some server puts in their http-alternates file.
You should only be able to get to this code-path by calling one of
git-remote-{http,https,ftp,ftps}. So there is no problem with
restricting the protocol beyond those options. And there should be no
problem with restricting within that set; if the protocol we intend to
feed to curl had been disallowed by policy, git would have blocked it
before hitting git-remote in the first place.
-Peff
^ permalink raw reply
* [PATCH v8 4/5] http: create function to get curl allowed protocols
From: Brandon Williams @ 2016-12-02 0:01 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-1-git-send-email-bmwill@google.com>
Move the creation of an allowed protocols whitelist to a helper
function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
http.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/http.c b/http.c
index fee128b..a1c3a0e 100644
--- a/http.c
+++ b/http.c
@@ -624,11 +624,25 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
+static long get_curl_allowed_protocols(void)
+{
+ long allowed_protocols = 0;
+
+ if (is_transport_allowed("http"))
+ allowed_protocols |= CURLPROTO_HTTP;
+ if (is_transport_allowed("https"))
+ allowed_protocols |= CURLPROTO_HTTPS;
+ if (is_transport_allowed("ftp"))
+ allowed_protocols |= CURLPROTO_FTP;
+ if (is_transport_allowed("ftps"))
+ allowed_protocols |= CURLPROTO_FTPS;
+
+ return allowed_protocols;
+}
static CURL *get_curl_handle(void)
{
CURL *result = curl_easy_init();
- long allowed_protocols = 0;
if (!result)
die("curl_easy_init failed");
@@ -725,15 +739,8 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_POST301, 1);
#endif
#if LIBCURL_VERSION_NUM >= 0x071304
- if (is_transport_allowed("http"))
- allowed_protocols |= CURLPROTO_HTTP;
- if (is_transport_allowed("https"))
- allowed_protocols |= CURLPROTO_HTTPS;
- if (is_transport_allowed("ftp"))
- allowed_protocols |= CURLPROTO_FTP;
- if (is_transport_allowed("ftps"))
- allowed_protocols |= CURLPROTO_FTPS;
- curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
+ curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
+ get_curl_allowed_protocols());
#else
warning("protocol restrictions not applied to curl redirects because\n"
"your curl version is too old (>= 7.19.4)");
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v8 2/5] transport: add protocol policy config option
From: Brandon Williams @ 2016-12-02 0:00 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-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
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