* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Philip Oakley @ 2016-12-01 23:12 UTC (permalink / raw)
To: Brandon Williams, Ramsay Jones; +Cc: Jeff King, git, Jann Horn
In-Reply-To: <20161201225331.GH54082@google.com>
From: "Brandon Williams" <bmwill@google.com>
> On 12/01, Ramsay Jones wrote:
>>
>>
>> On 01/12/16 09:04, Jeff King wrote:
>> > If a malicious server redirects the initial ref
>> > advertisement, it may be able to leak sha1s from other,
>> > unrelated servers that the client has access to. For
>> > example, imagine that Alice is a git user, she has access to
>> > a private repository on a server hosted by Bob, and Mallory
>> > runs a malicious server and wants to find out about Bob's
>> > private repository.
>> >
>> > Mallory asks Alice to clone an unrelated repository from her
>> -----------------------------------------------------------^^^
>> ... from _him_ ? (ie Mallory)
>>
>> > over HTTP. When Alice's client contacts Mallory's server for
>> > the initial ref advertisement, the server issues an HTTP
>> > redirect for Bob's server. Alice contacts Bob's server and
>> > gets the ref advertisement for the private repository. If
>> > there is anything to fetch, she then follows up by asking
>> > the server for one or more sha1 objects. But who is the
>> > server?
>> >
>> > If it is still Mallory's server, then Alice will leak the
>> > existence of those sha1s to her.
>> ------------------------------^^^
>> ... to _him_ ? (again Mallory)
>>
>> ATB,
>> Ramsay Jones
>
> Depends, I only know Mallorys who are women so her seems appropriate.
>
> --
> Brandon Williams
>
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.
--
Philip
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-01 23:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161201214004.3qujo5sfdn3y6c5u@sigill.intra.peff.net>
On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
>
> > Add the from_user parameter to the 'is_transport_allowed' function.
> > This allows callers to query if a transport protocol is allowed, given
> > that the caller knows that the protocol is coming from the user (1) or
> > not from the user (0), such as redirects in libcurl. If unknown, a -1
> > should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> > to determine if the protocol came from the user.
>
> Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
> already in 'next' anyway, though I guess we are due for a post-release
> reset of 'next').
>
> > diff --git a/http.c b/http.c
> > index fee128b..e74c0f0 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -725,13 +725,13 @@ static CURL *get_curl_handle(void)
> > curl_easy_setopt(result, CURLOPT_POST301, 1);
> > #endif
> > #if LIBCURL_VERSION_NUM >= 0x071304
> > - if (is_transport_allowed("http"))
> > + if (is_transport_allowed("http", 0))
> > allowed_protocols |= CURLPROTO_HTTP;
> > - if (is_transport_allowed("https"))
> > + if (is_transport_allowed("https", 0))
> > allowed_protocols |= CURLPROTO_HTTPS;
> > - if (is_transport_allowed("ftp"))
> > + if (is_transport_allowed("ftp", 0))
> > allowed_protocols |= CURLPROTO_FTP;
> > - if (is_transport_allowed("ftps"))
> > + if (is_transport_allowed("ftps", 0))
> > allowed_protocols |= CURLPROTO_FTPS;
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> > #else
>
> This is better, but I think we still need to deal with http-alternates
> on top.
>
> I think we'd need to move this allowed_protocols setup into a function
> like:
>
> int generate_allowed_protocols(int from_user)
> {
> int ret;
> if (is_transport_allowed("http", from_user))
> ret |= CURLPROTO_HTTP;
> ... etc ...
> return ret;
> }
>
> and then create a protocol list for each situation:
>
> allowed_protocols = generate_allowed_protocols(-1);
> allowed_redir_protocols = generate_allowed_protocols(0);
>
> and then we know we can always set up the redir protocols:
>
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);
>
> and which we feed for CURLOPT_PROTOCOLS depends on whether we are
> following an http-alternates redirect or not. But I suspect it will be a
> nasty change to plumb through the idea of "this request is on behalf of
> an http-alternates redirect".
>
> Given how few people probably care, I'm tempted to document it as a
> quirk and direct people to the upcoming http.followRedirects. The newly
> proposed default value of that disables http-alternates entirely anyway.
>
> -Peff
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.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 5/6] http: treat http-alternates like redirects
From: Brandon Williams @ 2016-12-01 23:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jann Horn
In-Reply-To: <20161201090432.wtcu2jpacwcf6a4a@sigill.intra.peff.net>
On 12/01, Jeff King wrote:
> - set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS
> restrict ourselves to a known-safe set and respect any
> user-provided whitelist.
> 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?
--
Brandon Williams
^ permalink raw reply
* Re: EXT: Re: "Your branch is ahead of 'origin' by X commits"
From: Alfonsogonzalez, Ernesto (GE Digital) @ 2016-12-01 22:36 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20161201222354.yu2q62udi56ygyoz@sigill.intra.peff.net>
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/1/16, 2:23 PM, "Jeff King" <peff@peff.net> wrote:
>On Thu, Dec 01, 2016 at 10:03:33PM +0000, Alfonsogonzalez, Ernesto (GE
>Digital) wrote:
>
>> So I used branch ‹set-upstream and see the expected behavior.
>>
>> $ git branch --set-upstream-to=origin/master
>> Branch master set up to track remote branch master from origin.
>
>Ah, that makes sense.
>
>> I¹m still not sure what it means for the branch upstream to be ³origin²
>> only.
>
>The name "origin" generally resolves to refs/remotes/origin/HEAD, which
>is a symbolic ref pointing to the "default branch" for that remote.
>That's generally set at clone time from what the remote has in its HEAD,
>but you can update it with "git remote set-head" if you want to.
>
>But that's just for resolving the name; I'm not sure that it would work
>to set a branch's upstream to just "origin". Do you possibly have
>another ref named origin?
>
>-Peff
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Brandon Williams @ 2016-12-01 22:53 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, git, Jann Horn
In-Reply-To: <331124b5-aa2b-773c-23ac-975ad3f50dbf@ramsayjones.plus.com>
On 12/01, Ramsay Jones wrote:
>
>
> On 01/12/16 09:04, Jeff King wrote:
> > If a malicious server redirects the initial ref
> > advertisement, it may be able to leak sha1s from other,
> > unrelated servers that the client has access to. For
> > example, imagine that Alice is a git user, she has access to
> > a private repository on a server hosted by Bob, and Mallory
> > runs a malicious server and wants to find out about Bob's
> > private repository.
> >
> > Mallory asks Alice to clone an unrelated repository from her
> -----------------------------------------------------------^^^
> ... from _him_ ? (ie Mallory)
>
> > over HTTP. When Alice's client contacts Mallory's server for
> > the initial ref advertisement, the server issues an HTTP
> > redirect for Bob's server. Alice contacts Bob's server and
> > gets the ref advertisement for the private repository. If
> > there is anything to fetch, she then follows up by asking
> > the server for one or more sha1 objects. But who is the
> > server?
> >
> > If it is still Mallory's server, then Alice will leak the
> > existence of those sha1s to her.
> ------------------------------^^^
> ... to _him_ ? (again Mallory)
>
> ATB,
> Ramsay Jones
Depends, I only know Mallorys who are women so her seems appropriate.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/3] compat: add qsort_s()
From: René Scharfe @ 2016-12-01 22:30 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Git List, Johannes Schindelin
In-Reply-To: <20161201201917.nqx3v5fl2ptl3bhr@sigill.intra.peff.net>
Am 01.12.2016 um 21:19 schrieb Jeff King:
> On Thu, Dec 01, 2016 at 12:14:42PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> To make matters more fun, apparently[1] there are multiple variants of
>>> qsort_r with different argument orders. _And_ apparently Microsoft
>>> defines qsort_s, but it's not quite the same thing. But all of that can
>>> be dealt with by having more specific flags (HAVE_GNU_QSORT_R, etc).
AFAIU it went like this:
// FreeBSD 5.0 (2003)
void qsort_r(void *base, size_t nmemb, size_t size,
void *context,
int (*compar)(void *context, const void *x, const void *y));
// Microsoft Visual Studio 2005
void qsort_s(void *base, size_t nmemb, size_t size,
int (*compar)(void *context, const void *x, const void *y),
void *context);
// glibc 2.8 (2008)
void qsort_r(void *base, size_t nmemb, size_t size,
int (*compar)(const void *x, const void *y, void *context),
void *context);
// C11 Annex K (2011)
errno_t qsort_s(void *base, rsize_t nmemb, rsize_t size,
int (*compar)(const void *x, const void *y, void *context),
void *context);
>>> It just seems like we should be able to do a better job of using the
>>> system qsort in many cases.
Sure, platform-specific implementations can be shorter.
>> If we were to go that route, perhaps we shouldn't have HAVE_QSORT_S
>> so that Microsoft folks won't define it by mistake (instead perhaps
>> call it HAVE_ISO_QSORT_S or something).
OK.
>> I like your suggestion in general. The body of git_qsort_s() on
>> systems without ISO_QSORT_S can do
>>
>> - GNU qsort_r() without any change in the parameters,
>>
>> - Microsoft qsort_s() with parameter reordered, or
>>
>> - Apple/BSD qsort_r() with parameter reordered.
>>
>> and that would cover the major platforms.
Yes.
However, for MSys INTERNAL_QSORT is defined for some reason, so the
platform's qsort(3) is not used there; I guess the same reason applies
to qsort_s(). If it doesn't then an implementation may want to convert
a call to the invalid parameter handler (which may show a dialog
offering to Retry, Continue or Abort) into a non-zero return value.
>> Eh, wait. BSD and Microsoft have paramters reordered in the
>> callback comparison function. I suspect that would not fly very
>> well.
>
> You can hack around it by passing a wrapper callback that flips the
> arguments. Since we have a "void *" data pointer, that would point to a
> struct holding the "real" callback and chaining to the original data
> pointer.
>
> It does incur the cost of an extra level of indirection for each
> comparison, though (not just for each qsort call).
Indeed. We'd need a perf test to measure that overhead before we could
determine if that's a problem, though.
> You could do it as zero-cost if you were willing to turn the comparison
> function definition into a macro.
Ugh. That either requires changing the signature of qsort_s() based on
the underlying native function as well, or using a void pointer to pass
the comparison function, no? Let's not do that, at least not without a
good reason.
René
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Jeff King @ 2016-12-01 22:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwan70i0.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 01, 2016 at 02:22:47PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> It took me a while to dig it up because the topic is so old, but
> >>
> >> https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
> >>
> >> is the thread I had in mind. The idea of rename detection followed
> >> soon afterwards.
> >
> > Thanks for an interesting read. Your diff-tree-helper patch is very
> > similar to what I wrote.
> >
> > I do think the right decision was made back then. The single-process
> > model is much more efficient, and it was over 10 years until somebody
> > actually wanted to expose the functionality to a script (and even now,
> > I'm not convinced enough people want it to even merit inclusion).
>
> Well, 10 years ago the person in the thread who argued "who cares
> about producing patches? each step in plumbing should do one thing
> and one thing only and do so well" was Linus, so your coming up with
> the diff-tree-helper again may indicate that we may want to step
> back and retry the experiment again, perhaps?
I think there are two questions, looking historically.
One is whether the functionality should be exposed to scripts at all.
The second is, assuming it should be exposed, in which order to do it.
You can write a series of small scripts, and then tie them together. Or
you can write tie it all together in C, and then make specific helpers
to expose the various bits.
The advantage of the first technique is that the tools are used
consistently by all parts of the system, so you know they don't grow
weird bugs or fail to handle corner cases. The advantage of the second
is that most people want the "tied-together" functionality, and it can
run a lot faster in-process.
So mostly I was suggesting that the right decision 10 years ago was to
optimize for speed in the common case, and let people worry later about
whether they wanted to expose the functionality in more flexible ways.
And that brings us to today.
It sounds like you are in favor of adding diff-pairs (and certainly it
shouldn't _hurt_ anybody if they are not interested in using it; you'll
notice the patch didn't need to touch the diff code at all).
-Peff
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-01 22:25 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161201214004.3qujo5sfdn3y6c5u@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
>
>> Add the from_user parameter to the 'is_transport_allowed' function.
>> This allows callers to query if a transport protocol is allowed, given
>> that the caller knows that the protocol is coming from the user (1) or
>> not from the user (0), such as redirects in libcurl. If unknown, a -1
>> should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
>> to determine if the protocol came from the user.
>
> Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
> already in 'next' anyway, though I guess we are due for a post-release
> reset of 'next').
Yes. I am planning to take a day off tomorrow, and probably will
rewind 'next' sometime this the weekend.
I agree with the comments you made in the remainder of the message I
am responding to, so I'll snip it.
>> diff --git a/http.c b/http.c
>> index fee128b..e74c0f0 100644
>> --- a/http.c
>> +++ b/http.c
>> ...
>
> This is better, but I think we still need to deal with http-alternates
> on top.
> ...
^ permalink raw reply
* Re: EXT: Re: "Your branch is ahead of 'origin' by X commits"
From: Jeff King @ 2016-12-01 22:23 UTC (permalink / raw)
To: Alfonsogonzalez, Ernesto (GE Digital); +Cc: git@vger.kernel.org
In-Reply-To: <D465DC74.B911%ernesto.alfonsogonzalez@ge.com>
On Thu, Dec 01, 2016 at 10:03:33PM +0000, Alfonsogonzalez, Ernesto (GE Digital) wrote:
> So I used branch ‹set-upstream and see the expected behavior.
>
> $ git branch --set-upstream-to=origin/master
> Branch master set up to track remote branch master from origin.
Ah, that makes sense.
> I¹m still not sure what it means for the branch upstream to be ³origin²
> only.
The name "origin" generally resolves to refs/remotes/origin/HEAD, which
is a symbolic ref pointing to the "default branch" for that remote.
That's generally set at clone time from what the remote has in its HEAD,
but you can update it with "git remote set-head" if you want to.
But that's just for resolving the name; I'm not sure that it would work
to set a branch's upstream to just "origin". Do you possibly have
another ref named origin?
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 22:22 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161201213019.qfkqd324ommikym2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> It took me a while to dig it up because the topic is so old, but
>>
>> https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
>>
>> is the thread I had in mind. The idea of rename detection followed
>> soon afterwards.
>
> Thanks for an interesting read. Your diff-tree-helper patch is very
> similar to what I wrote.
>
> I do think the right decision was made back then. The single-process
> model is much more efficient, and it was over 10 years until somebody
> actually wanted to expose the functionality to a script (and even now,
> I'm not convinced enough people want it to even merit inclusion).
Well, 10 years ago the person in the thread who argued "who cares
about producing patches? each step in plumbing should do one thing
and one thing only and do so well" was Linus, so your coming up with
the diff-tree-helper again may indicate that we may want to step
back and retry the experiment again, perhaps?
^ permalink raw reply
* Re: EXT: Re: "Your branch is ahead of 'origin' by X commits"
From: Junio C Hamano @ 2016-12-01 22:20 UTC (permalink / raw)
To: Alfonsogonzalez, Ernesto (GE Digital); +Cc: Jeff King, git@vger.kernel.org
In-Reply-To: <D465DC74.B911%ernesto.alfonsogonzalez@ge.com>
"Alfonsogonzalez, Ernesto (GE Digital)"
<ernesto.alfonsogonzalez@ge.com> writes:
> I'm still not sure what it means for the branch upstream to be 'origin'
> only.
If only you checked who the upstream of your 'master' was before
doing the set-upstream-to, it would have been trivial to answer that
question, but that is water under the bridge now.
A wild guess is that the upstream of your 'master' was 'origin/HEAD'
(whose name, when fully spelled out, is "refs/remotes/origin/HEAD"),
which pointed to something other than "refs/remotes/origin/master"?
^ permalink raw reply
* Re: EXT: Re: "Your branch is ahead of 'origin' by X commits"
From: Alfonsogonzalez, Ernesto (GE Digital) @ 2016-12-01 22:03 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
Hi Jeff,
I followed all your steps, but didn¹t find anything.
$ ls -d .git
.git
$ ls .git/master
ls: .git/master: No such file or directory
$ git show HEAD
commit 92d392c37e376db69d61dafdc427b379d860fb5a
Merge: 6be322c 5544904
...
$ git show refs/heads/master
commit 92d392c37e376db69d61dafdc427b379d860fb5a
Merge: 6be322c 5544904
...
$ git rev-parse --symbolic-full-name master
refs/heads/master
$
Then I realized that the message should say,
"Your branch is ahead of Œorigin/master' by X commits"
And not
"Your branch is ahead of 'origin' by X commits²
So I used branch ‹set-upstream and see the expected behavior.
$ git branch --set-upstream-to=origin/master
Branch master set up to track remote branch master from origin.
$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
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)
$
I¹m still not sure what it means for the branch upstream to be ³origin²
only.
I do have 2 remotes. A 2nd remote, called ³teamname-origin², is indeed
behind my local master by 108 commits.
So it seems there is a bug. When master¹s upstream is ³origin², it was
actually pointing to ³teamname-origin/master², which is behind by 108
commits.
However, pushing, pulling, rebasing, etc, all work against the correct
remote (³origin²).
So this could be a bug in git status?
Thanks,
Ernesto
On 12/1/16, 1:47 PM, "Jeff King" <peff@peff.net> wrote:
>On Thu, Dec 01, 2016 at 07:49:40PM +0000, Alfonsogonzalez, Ernesto (GE
>Digital) wrote:
>
>> $ git diff origin/master
>> $ 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)
>
>The "master" we are talking about here must always be
>"refs/heads/master", since it will have come from resolving the HEAD
>symbolic ref.
>
>But here:
>
>> $ git show origin/master --oneline
>> 92d392c Merge pull request #21 from org/branch
>>
>> $ git show master --oneline
>> 92d392c Merge pull request #21 from org/branch
>
>The "master" in the second case could possibly find "master" as another
>name. Is it possible you have a .git/master file (this may have been
>created by accidentally running "git update-ref master" instead of "git
>update-ref refs/heads/master")?
>
>Or other things you could check:
>
> # see what's on HEAD, which we know points to refs/heads/master
> git show HEAD
>
> # or just check refs/heads/master itself
> git show refs/heads/master
>
> # or just ask what "master" resolves to
> git rev-parse --symbolic-full-name master
>
>That last one actually seems to complain that "refname 'master' is
>ambiguous' if you do have .git/master. I think that's a minor bug, as it
>should presumably follow the normal disambiguation rules used for lookup
>(in which .git/master always takes precedence over refs/heads/master).
>
>-Peff
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jeff King @ 2016-12-01 21:59 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Junio C Hamano, git@vger.kernel.org,
Jonathan Tan
In-Reply-To: <CAGZ79kaqzssfN_bRQYpqC9HsKmyQZNCQcs+T5ke95Sf-C5PaRQ@mail.gmail.com>
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).
-Peff
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Stefan Beller @ 2016-12-01 21:56 UTC (permalink / raw)
To: Jeff King
Cc: Brandon Williams, Junio C Hamano, git@vger.kernel.org,
Jonathan Tan
In-Reply-To: <20161201205944.2py2ijranq4g2wap@sigill.intra.peff.net>
On Thu, Dec 1, 2016 at 12:59 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 01, 2016 at 12:54:44PM -0800, Brandon Williams wrote:
>
>> > I think this more robust check is probably a good idea, that way we
>> > don't step into a submodule with a .git directory that isn't really a
>> > .git dir.
>>
>> Looks like this is a no-go as well...the call to is_git_directory() ends
>> up calling real_path...which ends up performing the chdir call, which
>> puts us right back to where we started! (as a side note I was using
>> is_git_directory else where...which I now know I can't use)
>
> 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. :)
>
> -Peff
^ permalink raw reply
* Re: "Your branch is ahead of 'origin' by X commits"
From: Jeff King @ 2016-12-01 21:47 UTC (permalink / raw)
To: Alfonsogonzalez, Ernesto (GE Digital); +Cc: git@vger.kernel.org
In-Reply-To: <D465BDE6.B7DE%ernesto.alfonsogonzalez@ge.com>
On Thu, Dec 01, 2016 at 07:49:40PM +0000, Alfonsogonzalez, Ernesto (GE Digital) wrote:
> $ git diff origin/master
> $ 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)
The "master" we are talking about here must always be
"refs/heads/master", since it will have come from resolving the HEAD
symbolic ref.
But here:
> $ git show origin/master --oneline
> 92d392c Merge pull request #21 from org/branch
>
> $ git show master --oneline
> 92d392c Merge pull request #21 from org/branch
The "master" in the second case could possibly find "master" as another
name. Is it possible you have a .git/master file (this may have been
created by accidentally running "git update-ref master" instead of "git
update-ref refs/heads/master")?
Or other things you could check:
# see what's on HEAD, which we know points to refs/heads/master
git show HEAD
# or just check refs/heads/master itself
git show refs/heads/master
# or just ask what "master" resolves to
git rev-parse --symbolic-full-name master
That last one actually seems to complain that "refname 'master' is
ambiguous' if you do have .git/master. I think that's a minor bug, as it
should presumably follow the normal disambiguation rules used for lookup
(in which .git/master always takes precedence over refs/heads/master).
-Peff
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-01 21:40 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-5-git-send-email-bmwill@google.com>
On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
> Add the from_user parameter to the 'is_transport_allowed' function.
> This allows callers to query if a transport protocol is allowed, given
> that the caller knows that the protocol is coming from the user (1) or
> not from the user (0), such as redirects in libcurl. If unknown, a -1
> should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> to determine if the protocol came from the user.
Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
already in 'next' anyway, though I guess we are due for a post-release
reset of 'next').
> diff --git a/http.c b/http.c
> index fee128b..e74c0f0 100644
> --- a/http.c
> +++ b/http.c
> @@ -725,13 +725,13 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_POST301, 1);
> #endif
> #if LIBCURL_VERSION_NUM >= 0x071304
> - if (is_transport_allowed("http"))
> + if (is_transport_allowed("http", 0))
> allowed_protocols |= CURLPROTO_HTTP;
> - if (is_transport_allowed("https"))
> + if (is_transport_allowed("https", 0))
> allowed_protocols |= CURLPROTO_HTTPS;
> - if (is_transport_allowed("ftp"))
> + if (is_transport_allowed("ftp", 0))
> allowed_protocols |= CURLPROTO_FTP;
> - if (is_transport_allowed("ftps"))
> + if (is_transport_allowed("ftps", 0))
> allowed_protocols |= CURLPROTO_FTPS;
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> #else
This is better, but I think we still need to deal with http-alternates
on top.
I think we'd need to move this allowed_protocols setup into a function
like:
int generate_allowed_protocols(int from_user)
{
int ret;
if (is_transport_allowed("http", from_user))
ret |= CURLPROTO_HTTP;
... etc ...
return ret;
}
and then create a protocol list for each situation:
allowed_protocols = generate_allowed_protocols(-1);
allowed_redir_protocols = generate_allowed_protocols(0);
and then we know we can always set up the redir protocols:
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);
and which we feed for CURLOPT_PROTOCOLS depends on whether we are
following an http-alternates redirect or not. But I suspect it will be a
nasty change to plumb through the idea of "this request is on behalf of
an http-alternates redirect".
Given how few people probably care, I'm tempted to document it as a
quirk and direct people to the upcoming http.followRedirects. The newly
proposed default value of that disables http-alternates entirely anyway.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Jeff King @ 2016-12-01 21:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7f7j8iz6.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 01, 2016 at 12:58:21PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Thu, Dec 01, 2016 at 12:52:05PM -0800, Junio C Hamano wrote:
> >
> >> Jeff King <peff@peff.net> writes:
> >>
> >> > This takes the output of `diff-tree -z --raw` and feeds it
> >> > back to the later stages of the diff machinery to produce
> >> > diffs in other formats.
> >>
> >> A full circle. This reminds me of the experiment done more than 10
> >> years ago at the beginning of the "diffcore transformations" design.
> >
> > Heh, I didn't even think to dig for prior art on the list.
>
> It took me a while to dig it up because the topic is so old, but
>
> https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
>
> is the thread I had in mind. The idea of rename detection followed
> soon afterwards.
Thanks for an interesting read. Your diff-tree-helper patch is very
similar to what I wrote.
I do think the right decision was made back then. The single-process
model is much more efficient, and it was over 10 years until somebody
actually wanted to expose the functionality to a script (and even now,
I'm not convinced enough people want it to even merit inclusion).
-Peff
^ permalink raw reply
* "Your branch is ahead of 'origin' by X commits"
From: Alfonsogonzalez, Ernesto (GE Digital) @ 2016-12-01 19:49 UTC (permalink / raw)
To: git@vger.kernel.org
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.
(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 show origin/master --oneline
92d392c Merge pull request #21 from org/branch
$ git show master --oneline
92d392c Merge pull request #21 from org/branch
$ git --version
git version 2.10.2
Thanks,
Ernesto
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 21:04 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <xmqq7f7j8iz6.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> It took me a while to dig it up because the topic is so old, but
>
> https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
>
> is the thread I had in mind. The idea of rename detection followed
> soon afterwards.
... which was this one:
https://public-inbox.org/git/7vr7g4m0lz.fsf_-_@assigned-by-dhcp.cox.net/#t
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jeff King @ 2016-12-01 20:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git, sbeller, jonathantanmy
In-Reply-To: <20161201205444.GG54082@google.com>
On Thu, Dec 01, 2016 at 12:54:44PM -0800, Brandon Williams wrote:
> > I think this more robust check is probably a good idea, that way we
> > don't step into a submodule with a .git directory that isn't really a
> > .git dir.
>
> Looks like this is a no-go as well...the call to is_git_directory() ends
> up calling real_path...which ends up performing the chdir call, which
> puts us right back to where we started! (as a side note I was using
> is_git_directory else where...which I now know I can't use)
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?
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 20:58 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161201205504.flgaf7dwv3b3dkkd@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Dec 01, 2016 at 12:52:05PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > This takes the output of `diff-tree -z --raw` and feeds it
>> > back to the later stages of the diff machinery to produce
>> > diffs in other formats.
>>
>> A full circle. This reminds me of the experiment done more than 10
>> years ago at the beginning of the "diffcore transformations" design.
>
> Heh, I didn't even think to dig for prior art on the list.
It took me a while to dig it up because the topic is so old, but
https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
is the thread I had in mind. The idea of rename detection followed
soon afterwards.
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161201204042.6yslbyrg7l6ghhww@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This takes the output of `diff-tree -z --raw` and feeds it
> back to the later stages of the diff machinery to produce
> diffs in other formats.
A full circle. This reminds me of the experiment done more than 10
years ago at the beginning of the "diffcore transformations" design.
^ permalink raw reply
* Re: [RFC/PATCH] add diff-pairs tool
From: Jeff King @ 2016-12-01 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbmwv8j9m.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 01, 2016 at 12:52:05PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This takes the output of `diff-tree -z --raw` and feeds it
> > back to the later stages of the diff machinery to produce
> > diffs in other formats.
>
> A full circle. This reminds me of the experiment done more than 10
> years ago at the beginning of the "diffcore transformations" design.
Heh, I didn't even think to dig for prior art on the list.
In a sense this is just bringing the full power of diffcore out to the
scripting interface. The one missing component is that you can't
actually call diffcore_std() in the middle. The full pipeline would I
guess be something more like:
git diff-tree --raw -z $a $b |
git detect-renames |
git diff-pairs -p
or something. In my model it's sufficient for the rename detection to
happen as part of the first diff-tree (since it's a whole-tree operation
anyway, there's no benefit to breaking it up into chunks).
-Peff
^ permalink raw reply
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-12-01 20:54 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, sbeller, jonathantanmy
In-Reply-To: <20161201191603.GB54082@google.com>
On 12/01, Brandon Williams wrote:
> On 12/01, Jeff King wrote:
> > On Thu, Dec 01, 2016 at 10:46:23AM -0800, Junio C Hamano wrote:
> >
> > > > mkpath() is generally an unsafe function because it uses a static
> > > > buffer, but it's handy and safe for handing values to syscalls like
> > > > this.
> > >
> > > I think your "unsafe" is not about thread-safety but about "the
> > > caller cannot rely on returned value staying valid for long haul".
> > > If this change since v5 is about thread-safety, I am not sure if it
> > > is safe to use mkpath here.
> >
> > Oh, good point. I meant "staying valid", but somehow totally forgot that
> > we cared about thread reentrancy here. As if I hadn't just spent an hour
> > debugging a thread problem.
> >
> > My suggestion is clearly nonsense.
> >
> > > I am a bit wary of making the check too sketchy like this, but this
> > > is not about determining if a random "path" that has ".git" in a
> > > superproject working tree is a submodule or not (that information
> > > primarily comes from the superproject index), so I tend to agree
> > > with the patch that it is sufficient to check presence of ".git"
> > > alone.
> >
> > The real danger is that it is a different check than the child process
> > is going to use, so they may disagree (see the almost-infinite-loop
> > discussion elsewhere).
> >
> > It feels quite hacky, but checking:
> >
> > if (is_git_directory(suspect))
> > return 1; /* actual git dir */
> > if (!stat(suspect, &st) && S_ISREG(st.st_mode))
> > return 1; /* gitfile; may or may not be valid */
> > return 0;
> >
> > is a little more robust, because the child process will happily skip a
> > non-repo ".git" and keep walking back up to the superproject. Whereas if
> > it sees any ".git" file, even if it is bogus, it will barf then and
> > there.
> >
> > I'm actually not sure if that latter behavior is a bug or not. I don't
> > think it was really planned out, and it obviously is inconsistent with
> > the other repo-discovery cases. But it is a convenient side effect for
> > submodules, and I doubt anybody is bothered by it in practice.
> >
> > -Peff
>
> I think this more robust check is probably a good idea, that way we
> don't step into a submodule with a .git directory that isn't really a
> .git dir.
Looks like this is a no-go as well...the call to is_git_directory() ends
up calling real_path...which ends up performing the chdir call, which
puts us right back to where we started! (as a side note I was using
is_git_directory else where...which I now know I can't use)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion
From: Junio C Hamano @ 2016-12-01 20:42 UTC (permalink / raw)
To: Austin English; +Cc: Git Mailing List
In-Reply-To: <CACC5Q1eFM_G4wKopkbxabLEu8+nbt66wF1jKSoTuL1vnS5Tb4Q@mail.gmail.com>
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.
^ 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