* Re: [PATCH v3 6/6] grep: search history of moved submodules
From: Brandon Williams @ 2016-11-14 17:43 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jonathan Tan, Junio C Hamano
In-Reply-To: <CAGZ79kbf2i5s8Y84i2Wehbffsw1dUDUY6LYPEMME3vC6zo8-aw@mail.gmail.com>
On 11/11, Stefan Beller wrote:
> On Fri, Nov 11, 2016 at 3:51 PM, Brandon Williams <bmwill@google.com> wrote:
>
> > +
> > + rm -rf parent sub
>
> This line sounds like a perfect candidate for "test_when_finished"
> at the beginning of the test
K will do.
--
Brandon Williams
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-14 18:01 UTC (permalink / raw)
To: Michael J Gruber
Cc: Johannes Schindelin, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <0c39be16-76f8-0800-41a2-b7b1dccdd652@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> *My* idea of --no-index was for it to behave as similar to the
> --index-version as possible, regarding formatting etc., and to be a good
> substitute for ordinary diff. The proposed patch achieves exactly that -
Does it? It looks to me that it does a lot more.
> why should a *file* argument (which is not a pathspec in --no-index
> mode) not be treated in the same way in which every other command treats
> a file argument? The patch un-breaks the most natural expectation.
I think a filename given as a command line argument, e.g. <(cmd), is
now treated more sensibly with [2/2]. Something that is not a
directory to be descended into and is not a regular file needs to be
made into a form that we can use as a blob, and reading it into an
in-core buffer is a workable way to do so.
However, when taken together with [1/2], doesn't the proposed patch
"achieves" a lot more than "exactly that", namely, by not treating
symbolic links discovered during traversals of directories given
from the command line as such and dereferencing?
^ permalink raw reply
* Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-14 18:10 UTC (permalink / raw)
To: jonathantanmy; +Cc: git, sbeller, Brandon Williams
In-Reply-To: <1478908273-190166-6-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Teach grep to recursively search in submodules when provided with a
> <tree> object. This allows grep to search a submodule based on the state
> of the submodule that is present in a commit of the super project.
>
> When grep is provided with a <tree> object, the name of the object is
> prefixed to all output. In order to provide uniformity of output
> between the parent and child processes the option `--parent-basename`
> has been added so that the child can preface all of it's output with the
> name of the parent's object instead of the name of the commit SHA1 of
> the submodule. This changes output from the command
> `git grep -e. -l --recurse-submodules HEAD` from:
> HEAD:file
> <commit sha1 of submodule>:sub/file
>
> to:
> HEAD:file
> HEAD:sub/file
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
Unrelated tangent, but this makes readers wonder what the updated
trailer code would do to the last paragraph ;-). Does it behave
sensibly (with some sane definition of sensibleness)?
I am guessing that it would, because neither To: or HEAD: is what we
normally recognize as a known trailer block element.
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-14 18:24 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <1478729910-26232-1-git-send-email-dturner@twosigma.com>
On Wed, Nov 09, 2016 at 05:18:30PM -0500, David Turner wrote:
> In the event that a HTTP server closes the connection after giving a
> 200 but before giving any packets, we don't want to hang forever
> waiting for a response that will never come. Instead, we should die
> immediately.
I agree we don't want to hang forever, but this leaves open the
question: what is hanging?
My guess is that fetch-pack is waiting for more data from the server,
and remote-curl is waiting for fetch-pack to tell us what to send for
the next request. Neither will make forward progress because they are
effectively waiting on each other.
Which means this is likely a special case of malformed input from the
server. A server which likewise sends a partial response could end up in
the same deadlock, I would think (e.g., a half-finished pktline, or a
pktline but no trailing flush).
That doesn't make it wrong to fix this specific case (especially if it's
a common one), but I wonder if we could do better.
The root of the issue is that only fetch-pack understands the protocol,
and remote-curl is blindly proxying the data. But only remote-curl knows
that the HTTP request has ended, and it doesn't relay that information
to fetch-pack. So I can think of two solutions:
1. Some way of remote-curl communicating the EOF to fetch-pack. It
can't just close the descriptor, since we need to pass more data
over it for the followup requests. You'd need something
out-of-band, or to frame the HTTP data inside another layer of
pktlines, both of which are kind of gross.
2. Have remote-curl understand enough of the protocol that it can
abort rather than hang.
I think that's effectively the approach of your patch, but for one
specific case. But could we, for example, make sure that everything
we proxy is a complete set of pktlines and ends with a flush? And
if not, then we hang up on fetch-pack.
I _think_ that would work, because even the pack is always encased
in pktlines for smart-http.
> @@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
> curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
> curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>
> +
> + rpc->any_written = 0;
Extra blank line here?
> @@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
> if (err != HTTP_OK)
> err = -1;
>
> + if (!rpc->any_written)
> + err = -1;
> +
I wondered if there were any cases where it was normal for the server to
return zero bytes. Possibly the ref advertisement is one, but this is
_just_ handling post_rpc(), so that's OK. And I think by definition
every response has to at least return a flush packet, or we would make
no forward progress (i.e., the exact case you are dealing with here).
-Peff
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Matt McCutchen @ 2016-11-14 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqq1syezs3g.fsf@gitster.mtv.corp.google.com>
On Sun, 2016-11-13 at 18:57 -0800, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
>
> >
> > Documentation/fetch-push-security.txt | 9 +++++++++
>
> A new (consolidated) piece like this that can be included in
> multiple places is a good idea. I wonder if the original
> description in "namespaces" thing can be moved here and then
> "namespaces" page can be made to also borrow from this?
I gave this a try. New patch coming.
> > --- /dev/null
> > +++ b/Documentation/fetch-push-security.txt
> > @@ -0,0 +1,9 @@
> > +SECURITY
> > +--------
> > +The fetch and push protocols are not designed to prevent a
> > malicious
> > +server from stealing data from your repository that you did not
> > intend to
> > +share. The possible attacks are similar to the ones described in
> > the
> > +"SECURITY" section of linkgit:gitnamespaces[7]. If you have
> > private data
> > +that you need to protect from the server, keep it in a separate
> > +repository.
>
> Yup, and then "do not push to untrustworthy place without checking
> what you are pushing", too?
If there is no private data in the repository, then there is no need
for the user to check what they are pushing. As I've indicated before,
IMO manually checking each push would not be a workable security
measure in the long term anyway.
Matt
^ permalink raw reply
* [PATCH] doc: mention transfer data leaks in more places
From: Matt McCutchen @ 2016-11-14 18:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <1479148088.2406.27.camel@mattmccutchen.net>
The "SECURITY" section of the gitnamespaces(7) man page described two
ways for a client to steal data from a server that wasn't intended to be
shared. Similar attacks can be performed by a server on a client, so
adapt the section to cover both directions and add it to the
git-fetch(1), git-pull(1), and git-push(1) man pages. Also add
references to this section from the documentation of server
configuration options that attempt to control data leakage but may not
be fully effective.
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
Documentation/config.txt | 17 ++++++++++++++---
Documentation/git-fetch.txt | 2 ++
Documentation/git-pull.txt | 2 ++
Documentation/git-push.txt | 2 ++
Documentation/gitnamespaces.txt | 20 +-------------------
Documentation/transfer-data-leaks.txt | 30 ++++++++++++++++++++++++++++++
6 files changed, 51 insertions(+), 22 deletions(-)
create mode 100644 Documentation/transfer-data-leaks.txt
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 21fdddf..fc2cf83 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2898,6 +2898,11 @@ is omitted from the advertisements but `refs/heads/master` and
`refs/namespaces/bar/refs/heads/master` are still advertised as so-called
"have" lines. In order to match refs before stripping, add a `^` in front of
the ref name. If you combine `!` and `^`, `!` must be specified first.
++
+Even if you hide refs, a client may still be able to steal the target
+objects via the techniques described in the "SECURITY" section of the
+linkgit:gitnamespaces[7] man page; it's best to keep private data in a
+separate repository.
transfer.unpackLimit::
When `fetch.unpackLimit` or `receive.unpackLimit` are
@@ -2907,7 +2912,7 @@ transfer.unpackLimit::
uploadarchive.allowUnreachable::
If true, allow clients to use `git archive --remote` to request
any tree, whether reachable from the ref tips or not. See the
- discussion in the `SECURITY` section of
+ discussion in the "SECURITY" section of
linkgit:git-upload-archive[1] for more details. Defaults to
`false`.
@@ -2921,13 +2926,19 @@ uploadpack.allowTipSHA1InWant::
When `uploadpack.hideRefs` is in effect, allow `upload-pack`
to accept a fetch request that asks for an object at the tip
of a hidden ref (by default, such a request is rejected).
- see also `uploadpack.hideRefs`.
+ See also `uploadpack.hideRefs`. Even if this is false, a client
+ may be able to steal objects via the techniques described in the
+ "SECURITY" section of the linkgit:gitnamespaces[7] man page; it's
+ best to keep private data in a separate repository.
uploadpack.allowReachableSHA1InWant::
Allow `upload-pack` to accept a fetch request that asks for an
object that is reachable from any ref tip. However, note that
calculating object reachability is computationally expensive.
- Defaults to `false`.
+ Defaults to `false`. Even if this is false, a client may be able
+ to steal objects via the techniques described in the "SECURITY"
+ section of the linkgit:gitnamespaces[7] man page; it's best to
+ keep private data in a separate repository.
uploadpack.keepAlive::
When `upload-pack` has started `pack-objects`, there may be a
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 9e42169..b153aef 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -192,6 +192,8 @@ The first command fetches the `maint` branch from the repository at
objects will eventually be removed by git's built-in housekeeping (see
linkgit:git-gc[1]).
+include::transfer-data-leaks.txt[]
+
BUGS
----
Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index d033b25..4470e4b 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -237,6 +237,8 @@ If you tried a pull which resulted in complex conflicts and
would want to start over, you can recover with 'git reset'.
+include::transfer-data-leaks.txt[]
+
BUGS
----
Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 47b77e6..8eefabd 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -559,6 +559,8 @@ Commits A and B would no longer belong to a branch with a symbolic name,
and so would be unreachable. As such, these commits would be removed by
a `git gc` command on the origin repository.
+include::transfer-data-leaks.txt[]
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/Documentation/gitnamespaces.txt b/Documentation/gitnamespaces.txt
index 7685e36..b614969 100644
--- a/Documentation/gitnamespaces.txt
+++ b/Documentation/gitnamespaces.txt
@@ -61,22 +61,4 @@ For a simple local test, you can use linkgit:git-remote-ext[1]:
git clone ext::'git --namespace=foo %s /tmp/prefixed.git'
----------
-SECURITY
---------
-
-Anyone with access to any namespace within a repository can potentially
-access objects from any other namespace stored in the same repository.
-You can't directly say "give me object ABCD" if you don't have a ref to
-it, but you can do some other sneaky things like:
-
-. Claiming to push ABCD, at which point the server will optimize out the
- need for you to actually send it. Now you have a ref to ABCD and can
- fetch it (claiming not to have it, of course).
-
-. Requesting other refs, claiming that you have ABCD, at which point the
- server may generate deltas against ABCD.
-
-None of this causes a problem if you only host public repositories, or
-if everyone who may read one namespace may also read everything in every
-other namespace (for instance, if everyone in an organization has read
-permission to every repository).
+include::transfer-data-leaks.txt[]
diff --git a/Documentation/transfer-data-leaks.txt b/Documentation/transfer-data-leaks.txt
new file mode 100644
index 0000000..914bacc
--- /dev/null
+++ b/Documentation/transfer-data-leaks.txt
@@ -0,0 +1,30 @@
+SECURITY
+--------
+The fetch and push protocols are not designed to prevent one side from
+stealing data from the other repository that was not intended to be
+shared. If you have private data that you need to protect from a malicious
+peer, your best option is to store it in another repository. This applies
+to both clients and servers. In particular, namespaces on a server are not
+effective for read access control; you should only grant read access to a
+namespace to clients that you would trust with read access to the entire
+repository.
+
+The known attack vectors are as follows:
+
+. The victim sends "have" lines advertising the IDs of objects it has that
+ are not explicitly intended to be shared but can be used to optimize the
+ transfer if the peer also has them. The attacker chooses an object ID X
+ to steal and sends a ref to X, but isn't required to send the content of
+ X because the victim already has it. Now the victim believes that the
+ attacker has X, and it sends the content of X back to the attacker
+ later. (This attack is most straightforward for a client to perform on a
+ server, by creating a ref to X in the namespace the client has access
+ to and then fetching it. The most likely way for a server to perform it
+ on a client is to "merge" X into a public branch and hope that the user
+ does additional work on this branch and pushes it back to the server
+ without noticing the merge.)
+
+. As in #1, the attacker chooses an object ID X to steal. The victim sends
+ an object Y that the attacker already has, and the attacker falsely
+ claims to have X and not Y, so the victim sends Y as a delta against X.
+ The delta reveals regions of X that are similar to Y to the attacker.
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Jonathan Tan @ 2016-11-14 18:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, Brandon Williams
In-Reply-To: <xmqqk2c6x79c.fsf@gitster.mtv.corp.google.com>
On 11/14/2016 10:10 AM, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> Teach grep to recursively search in submodules when provided with a
>> <tree> object. This allows grep to search a submodule based on the state
>> of the submodule that is present in a commit of the super project.
>>
>> When grep is provided with a <tree> object, the name of the object is
>> prefixed to all output. In order to provide uniformity of output
>> between the parent and child processes the option `--parent-basename`
>> has been added so that the child can preface all of it's output with the
>> name of the parent's object instead of the name of the commit SHA1 of
>> the submodule. This changes output from the command
>> `git grep -e. -l --recurse-submodules HEAD` from:
>> HEAD:file
>> <commit sha1 of submodule>:sub/file
>>
>> to:
>> HEAD:file
>> HEAD:sub/file
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>
> Unrelated tangent, but this makes readers wonder what the updated
> trailer code would do to the last paragraph ;-). Does it behave
> sensibly (with some sane definition of sensibleness)?
>
> I am guessing that it would, because neither To: or HEAD: is what we
> normally recognize as a known trailer block element.
Yes, it behaves sensibly :-) because "Signed-off-by:" is preceded by a
blank line, so the trailer block consists only of that line.
Having said that, it is probably better to indent those examples in the
commit message (by at least one space or one tab) - then they will never
be confused with trailers (once my patch set is in).
^ permalink raw reply
* Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-14 18:56 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, sbeller, Brandon Williams
In-Reply-To: <c83066bc-5b2b-998c-7e22-c4fccbaba5de@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>>> to:
>>> HEAD:file
>>> HEAD:sub/file
>>>
>>> Signed-off-by: Brandon Williams <bmwill@google.com>
>>> ---
>>
>> Unrelated tangent, but this makes readers wonder what the updated
>> trailer code would do to the last paragraph ;-). Does it behave
>> sensibly (with some sane definition of sensibleness)?
>>
>> I am guessing that it would, because neither To: or HEAD: is what we
>> normally recognize as a known trailer block element.
>
> Yes, it behaves sensibly :-) because "Signed-off-by:" is preceded by a
> blank line, so the trailer block consists only of that line.
Oh, that was not what I was wondering. Imagine Brandon writing his
message that ends in these three questionable lines and then running
"commit -s --amend" to add his sign-off---that was the case I was
wondering.
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Junio C Hamano @ 2016-11-14 19:00 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git, Jeff King
In-Reply-To: <1479148088.2406.27.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
>> Yup, and then "do not push to untrustworthy place without checking
>> what you are pushing", too?
>
> If there is no private data in the repository, then there is no need
> for the user to check what they are pushing. As I've indicated before,
> IMO manually checking each push would not be a workable security
> measure in the long term anyway.
Then what is? Don't answer; this is a rhetorical question.
The answer is "do not push to untrustworthy place", if you are
unable to check what you are pushing.
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Jeff King @ 2016-11-14 19:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matt McCutchen, git
In-Reply-To: <xmqqbmxhyjij.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 14, 2016 at 11:00:04AM -0800, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
>
> >> Yup, and then "do not push to untrustworthy place without checking
> >> what you are pushing", too?
> >
> > If there is no private data in the repository, then there is no need
> > for the user to check what they are pushing. As I've indicated before,
> > IMO manually checking each push would not be a workable security
> > measure in the long term anyway.
>
> Then what is? Don't answer; this is a rhetorical question.
>
> The answer is "do not push to untrustworthy place", if you are
> unable to check what you are pushing.
I think "check what you are pushing" only covers one case (attacker lies
to you during a fetch, and you accidentally push that back, thinking
they already have it).
But consider the other case mentioned: the attacker lies to you while
pushing and _says_ they have X, then deduces information from the delta
you generate. The only advice there is "do not push to an untrusted
place from a repository containing private objects".
So I think the in-between answer is "it is OK to push to an
untrustworthy place, but do not do it from a repo that may contain
secret contents".
-Peff
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Matt McCutchen @ 2016-11-14 19:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqqbmxhyjij.fsf@gitster.mtv.corp.google.com>
On Mon, 2016-11-14 at 11:00 -0800, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
>
> >
> > >
> > > Yup, and then "do not push to untrustworthy place without
> > > checking
> > > what you are pushing", too?
> >
> > If there is no private data in the repository, then there is no
> > need
> > for the user to check what they are pushing. As I've indicated
> > before,
> > IMO manually checking each push would not be a workable security
> > measure in the long term anyway.
>
> Then what is?
Don't put private data in the same repository, then the whole issue
becomes moot. Am I missing something?
Matt
^ permalink raw reply
* Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Jonathan Tan @ 2016-11-14 19:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, Brandon Williams
In-Reply-To: <xmqqfumtyjoy.fsf@gitster.mtv.corp.google.com>
On 11/14/2016 10:56 AM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>>> to:
>>>> HEAD:file
>>>> HEAD:sub/file
>>>>
>>>> Signed-off-by: Brandon Williams <bmwill@google.com>
>>>> ---
>>>
>>> Unrelated tangent, but this makes readers wonder what the updated
>>> trailer code would do to the last paragraph ;-). Does it behave
>>> sensibly (with some sane definition of sensibleness)?
>>>
>>> I am guessing that it would, because neither To: or HEAD: is what we
>>> normally recognize as a known trailer block element.
>>
>> Yes, it behaves sensibly :-) because "Signed-off-by:" is preceded by a
>> blank line, so the trailer block consists only of that line.
>
> Oh, that was not what I was wondering. Imagine Brandon writing his
> message that ends in these three questionable lines and then running
> "commit -s --amend" to add his sign-off---that was the case I was
> wondering.
Ah, I see. In that case, it would consider the last block as a trailer
block and attach it directly:
to:
HEAD:file
HEAD:sub/file
Signed-off-by: ...
It is true that neither to: nor HEAD: are known trailers, but my patch
set accepts trailer blocks that are 100% well-formed regardless of
whether the trailers are known (to provide backwards compatibility with
git-interpret-trailers, and to satisfy the certain use cases that I
brought up). The "known trailer" check is used when the trailer block is
not 100% well-formed.
This issue can be avoided if those lines were indented with at least one
space or at least one tab.
^ permalink raw reply
* Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-14 19:14 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Junio C Hamano, git, sbeller
In-Reply-To: <5a60b630-15fc-6b63-fb03-25aa0d1ef081@google.com>
On 11/14, Jonathan Tan wrote:
> On 11/14/2016 10:56 AM, Junio C Hamano wrote:
> >Jonathan Tan <jonathantanmy@google.com> writes:
> >
> >>>>to:
> >>>>HEAD:file
> >>>>HEAD:sub/file
> >>>>
> >>>>Signed-off-by: Brandon Williams <bmwill@google.com>
> >>>>---
> >>>
> >>>Unrelated tangent, but this makes readers wonder what the updated
> >>>trailer code would do to the last paragraph ;-). Does it behave
> >>>sensibly (with some sane definition of sensibleness)?
> >>>
> >>>I am guessing that it would, because neither To: or HEAD: is what we
> >>>normally recognize as a known trailer block element.
> >>
> >>Yes, it behaves sensibly :-) because "Signed-off-by:" is preceded by a
> >>blank line, so the trailer block consists only of that line.
> >
> >Oh, that was not what I was wondering. Imagine Brandon writing his
> >message that ends in these three questionable lines and then running
> >"commit -s --amend" to add his sign-off---that was the case I was
> >wondering.
>
> Ah, I see. In that case, it would consider the last block as a
> trailer block and attach it directly:
>
> to:
> HEAD:file
> HEAD:sub/file
> Signed-off-by: ...
>
> It is true that neither to: nor HEAD: are known trailers, but my
> patch set accepts trailer blocks that are 100% well-formed
> regardless of whether the trailers are known (to provide backwards
> compatibility with git-interpret-trailers, and to satisfy the
> certain use cases that I brought up). The "known trailer" check is
> used when the trailer block is not 100% well-formed.
>
> This issue can be avoided if those lines were indented with at least
> one space or at least one tab.
Who would have thought my simple example would cause this kind of
discussion! I can update the commit message and indent the output so
that it looks like the following:
to:
HEAD:file
HEAD:sub/file
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] doc: mention transfer data leaks in more places
From: Junio C Hamano @ 2016-11-14 19:19 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git, Jeff King
In-Reply-To: <1479148255.2406.30.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> The "SECURITY" section of the gitnamespaces(7) man page described two
> ways for a client to steal data from a server that wasn't intended to be
> shared. Similar attacks can be performed by a server on a client, so
> adapt the section to cover both directions and add it to the
> git-fetch(1), git-pull(1), and git-push(1) man pages. Also add
> references to this section from the documentation of server
> configuration options that attempt to control data leakage but may not
> be fully effective.
This round looks OK. Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v7 16/17] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-11-14 19:23 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xr20UyKMKUZBpP-SjtEhbow2df+iT6nF67mOAZ8BAaxEg@mail.gmail.com>
Hello
On Wed, Nov 9, 2016 at 5:44 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Port branch.c to use ref-filter APIs for printing. This clears out
>> most of the code used in branch.c for printing and replaces them with
>> calls made to the ref-filter library.
>
> Nice. This looks correct based on checking against the current
> branch.c implementation by hand. There was one minor change I
> suggested but I'm not really sure it buys is that much.
>
Thanks for this review. More down.
>> + if (filter->verbose > 1)
>> + strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
>> + "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
>> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
>
> When we have extra verbose, we check whether we have an upstream, and
> if so, we print the short name of that upstream inside brackets. If we
> have tracking information, we print that without brackets, and then we
> end this section. Finally we print the subject.
>
> We could almost re-use the code for the subject bits, but I'm not sure
> it's worth it. Maybe drop the %contents:subject part and add it
> afterwards since we always want it? It would remove some duplication
> but overall not sure it's actually worth it.
>
If you see that's the last part we add to the 'local' strbuf in the
verbose case.
If we want to remove the duplication we'll end up adding one more
strbuf_addf(...).
So I guess its better this way.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-11-14 19:24 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqRjMThZF7u_W6G0sHjFP3j5PMr=TszC6UxL2XCYO+CVA@mail.gmail.com>
Hello,
On Wed, Nov 9, 2016 at 5:45 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Initially posted here: $(gmane/279226). It was decided that this series
>> would follow up after refactoring ref-filter parsing mechanism, which
>> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>>
>> v1 can be found here: $(gmane/288342)
>> v2 can be found here: $(gmane/288863)
>> v3 can be found here: $(gmane/290299)
>> v4 can be found here: $(gmane/291106)
>> v5b can be found here: $(gmane/292467)
>> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
>>
>
> I reviewed the full series. I found a few minor things I would have
> done differently, but overall I think it looks good. Thanks for the
> hard work and the time invested here. I remember seeing this on the
> list quite some time ago, so it's nice to see it finally come
> together.
>
Thanks for the review. I've gone through all the mails and will follow
up with replies.
Will post the next version as soon as possible. Thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-14 19:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqq60nqzuye.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 14, 2016 at 7:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>>> index 600b703..f4ad297 100644
>>>> --- a/Documentation/git-for-each-ref.txt
>>>> +++ b/Documentation/git-for-each-ref.txt
>>>> @@ -96,7 +96,9 @@ refname::
>>>> slash-separated path components from the front of the refname
>>>> (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>>>> `<N>` must be a positive integer. If a displayed ref has fewer
>>>> - components than `<N>`, the command aborts with an error.
>>>> + components than `<N>`, the command aborts with an error. For the base
>>>> + directory of the ref (i.e. foo in refs/foo/bar/boz) append
>>>> + `:base`. For the entire directory path append `:dir`.
>
> Sorry that I missed this so far and I do not know how many recent
> rerolls had them like this, but I am not sure about these :base and
> :dir suffixes. From their names I think readers would expect that
> they do rough equivalents to basename() and dirname() applied to the
> refname, but the example contradicts with that intuition. The
> result of applying basename() to 'refs/boo/bar/boz' would be 'boz'
> and not 'foo' as the example says.
>
True that the options ':dir' and ':base' seem to be conflicting with
the use case
of basename() and dirname(), These were names which I thought best fit
the options
with no relation to basename() and dirname(). But now that you mention
it, it would
make sense to change these names to something more suitable, any suggestions
would be welcome.
> So assuming that :base and :dir are unrelated to basename() and
> dirname():
>
> - I think calling these :base and :dir may be misleading
>
> - More importantly, what do these do? I do not think of a good
> description that generalizes "base of refs/foo/bar/boz is foo" to
> explain your :base.
$ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
refs/heads/master refs/heads
refs/heads/ref-filter refs/heads
refs/remotes/junio/va/i18n refs/remotes/junio/va
$ ./git for-each-ref refs/heads --format
"%(align:left,30)%(refname)%(end) %(refname:base)"
refs/heads/master heads
refs/heads/ref-filter heads
refs/remotes/junio/va/i18n remotes
I guess this should clear it up.
>
> - A :dir that corresponds to the :base that picks 'foo' from
> 'refs/foo/bar/boz' needs an example, too.
>
I could add in an example.
> Or is the above example simply a typo? Is refs/foo/bar/boz:base
> 'boz', not 'foo'?
>
>
Not a typo, but open to changes in name.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-14 19:40 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <20161114182431.e7jjnq422c4xobdb@sigill.intra.peff.net>
On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
> 2. Have remote-curl understand enough of the protocol that it can
> abort rather than hang.
>
> I think that's effectively the approach of your patch, but for one
> specific case. But could we, for example, make sure that everything
> we proxy is a complete set of pktlines and ends with a flush? And
> if not, then we hang up on fetch-pack.
>
> I _think_ that would work, because even the pack is always encased
> in pktlines for smart-http.
So something like this. It turned out to be a lot uglier than I had
hoped because we get fed the data from curl in odd-sized chunks, so we
need a state machine.
But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the
same way.
---
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c..605357d77 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -403,6 +403,18 @@ struct rpc_state {
struct strbuf result;
unsigned gzip_request : 1;
unsigned initial_buffer : 1;
+
+ enum {
+ RPC_PKTLINE_ERROR, /* bogus hex chars in length */
+ RPC_PKTLINE_INITIAL, /* no packets received yet */
+ RPC_PKTLINE_1, /* got one hex char */
+ RPC_PKTLINE_2, /* got two hex chars */
+ RPC_PKTLINE_3, /* got three hex chars */
+ RPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */
+ RPC_PKTLINE_END_OF_PACKET, /* last packet completed */
+ RPC_PKTLINE_FLUSH, /* last packet was flush */
+ } pktline_state;
+ size_t pktline_len;
};
static size_t rpc_out(void *ptr, size_t eltsize,
@@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
}
#endif
+static void update_pktline_state(struct rpc_state *rpc,
+ const char *buf, size_t len)
+{
+#define READ_ONE_HEX(shift) do { \
+ int val = hexval(buf[0]); \
+ if (val < 0) { \
+ warning("error on %d", *buf); \
+ rpc->pktline_state = RPC_PKTLINE_ERROR; \
+ return; \
+ } \
+ rpc->pktline_len |= val << shift; \
+ buf++; \
+ len--; \
+} while(0)
+
+ while (len > 0) {
+ switch (rpc->pktline_state) {
+ case RPC_PKTLINE_ERROR:
+ /* previous error; there is no recovery */
+ return;
+
+ /* We can start a new pktline at any of these states */
+ case RPC_PKTLINE_INITIAL:
+ case RPC_PKTLINE_FLUSH:
+ case RPC_PKTLINE_END_OF_PACKET:
+ rpc->pktline_len = 0;
+ READ_ONE_HEX(12);
+ rpc->pktline_state = RPC_PKTLINE_1;
+ break;
+
+ case RPC_PKTLINE_1:
+ READ_ONE_HEX(8);
+ rpc->pktline_state = RPC_PKTLINE_2;
+ break;
+
+ case RPC_PKTLINE_2:
+ READ_ONE_HEX(4);
+ rpc->pktline_state = RPC_PKTLINE_3;
+ break;
+
+ case RPC_PKTLINE_3:
+ READ_ONE_HEX(0);
+ if (rpc->pktline_len) {
+ rpc->pktline_state = RPC_PKTLINE_DATA;
+ rpc->pktline_len -= 4;
+ } else
+ rpc->pktline_state = RPC_PKTLINE_FLUSH;
+ break;
+
+ case RPC_PKTLINE_DATA:
+ if (len < rpc->pktline_len) {
+ rpc->pktline_len -= len;
+ len = 0;
+ } else {
+ buf += rpc->pktline_len;
+ len -= rpc->pktline_len;
+ rpc->pktline_len = 0;
+ rpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;
+ }
+ break;
+ }
+ }
+#undef READ_ONE_HEX
+}
+
static size_t rpc_in(char *ptr, size_t eltsize,
size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
struct rpc_state *rpc = buffer_;
+ update_pktline_state(rpc, ptr, size);
write_or_die(rpc->in, ptr, size);
return size;
}
@@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+ rpc->pktline_state = RPC_PKTLINE_INITIAL;
+
err = run_slot(slot, NULL);
if (err == HTTP_REAUTH && !large_request) {
credential_fill(&http_auth);
@@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)
if (err != HTTP_OK)
err = -1;
+ if (rpc->pktline_state != RPC_PKTLINE_FLUSH) {
+ error("invalid or truncated response from http server");
+ err = -1;
+ }
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
^ permalink raw reply related
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Ramsay Jones @ 2016-11-14 19:45 UTC (permalink / raw)
To: Jeff King, Torsten Bögershausen
Cc: Lars Schneider, Junio C Hamano, Johannes Schindelin,
Johannes Sixt, git, pranit.bauva
In-Reply-To: <20161114170105.btnohk2777ddaiul@sigill.intra.peff.net>
On 14/11/16 17:01, Jeff King wrote:
> On Mon, Nov 14, 2016 at 05:35:56PM +0100, Torsten Bögershausen wrote:
>
>>> Git 'pu' does not compile on macOS right now:
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized
>>> whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>
> The next step is to make sure that the topic author is aware (in this
> case, one assumes it's pb/bisect).
[+cc Pranit]
Yep, I had a quick squint, and it looks like the compiler is correct.
It should be complaining about the 'bad_syn' variable for exactly the
same reason: namely, whenever the if condition is true, the only exit
from that block is via 'goto finish' which bypasses the initialisation
of 'good_syn' and 'bad_syn'.
> Better still is to make a patch that can either be applied on top, or
> squashed as appropriate.
No patch this time, but it simply requires those variables to be
initialised to NULL in their declarations. :-D
> I know that Ramsay Jones does this, for
> example, with some of his sparse-related checks, and I'm pretty sure
> from the turnaround-time that he runs it against "pu".
Yep, the idea being to catch these simple problems before the topic
reaches 'next'.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Junio C Hamano @ 2016-11-14 19:47 UTC (permalink / raw)
To: Jeff King; +Cc: Matt McCutchen, git
In-Reply-To: <20161114190725.fxjymvztc2eiomv6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So I think the in-between answer is "it is OK to push to an
> untrustworthy place, but do not do it from a repo that may contain
> secret contents".
Yes, that sounds like a sensible piece of advice to give to the
readers.
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-14 19:51 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZSFuq2+6xsrJ=CcXuOVbTnbDirbRtu7Fonfk+9EdRpbxg@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> - More importantly, what do these do? I do not think of a good
>> description that generalizes "base of refs/foo/bar/boz is foo" to
>> explain your :base.
>
> $ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
> refs/heads/master refs/heads
> refs/heads/ref-filter refs/heads
> refs/remotes/junio/va/i18n refs/remotes/junio/va
>
> $ ./git for-each-ref refs/heads --format
> "%(align:left,30)%(refname)%(end) %(refname:base)"
> refs/heads/master heads
> refs/heads/ref-filter heads
> refs/remotes/junio/va/i18n remotes
>
> I guess this should clear it up.
Hmph.
I would guess from these examples that :dir is an equivalent to
dirname(). But it is unclear how :base is defined. Is it the path
component that comes immediately after "refs/" that appears at the
beginning?
^ permalink raw reply
* Re: [PATCH] attr: mark a file-local symbol as static
From: Stefan Beller @ 2016-11-14 20:00 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <83508d1f-e809-f6be-5afc-4c23195dbd08@ramsayjones.plus.com>
On Sun, Nov 13, 2016 at 8:42 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Stefan,
>
> If you need to re-roll your 'sb/attr' branch, could you please
> squash this into the relevant patch.
will do. I have it applied locally
>
> Alternatively, since there is only a single call site for git_attr()
> (on line #1005), you could perhaps remove git_attr() and inline that
> call. (However, that does make that line exceed 80 columns).
I'll look into that.
Thanks,
Stefan
>
> Thanks!
>
> ATB,
> Ramsay Jones
>
> attr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/attr.c b/attr.c
> index 667ba85..84c4b08 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -169,7 +169,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
> return a;
> }
>
> -struct git_attr *git_attr(const char *name)
> +static struct git_attr *git_attr(const char *name)
> {
> return git_attr_internal(name, strlen(name));
> }
> --
> 2.10.0
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Michael J Gruber @ 2016-11-14 20:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <xmqqoa1ix7nq.fsf@gitster.mtv.corp.google.com>
Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> *My* idea of --no-index was for it to behave as similar to the
>> --index-version as possible, regarding formatting etc., and to be a good
>> substitute for ordinary diff. The proposed patch achieves exactly that -
>
> Does it? It looks to me that it does a lot more.
Yes, I didn't mean to say that it achieves only that - it achieves that
one goal exactly, and more.
>> why should a *file* argument (which is not a pathspec in --no-index
>> mode) not be treated in the same way in which every other command treats
>> a file argument? The patch un-breaks the most natural expectation.
>
> I think a filename given as a command line argument, e.g. <(cmd), is
> now treated more sensibly with [2/2]. Something that is not a
> directory to be descended into and is not a regular file needs to be
> made into a form that we can use as a blob, and reading it into an
> in-core buffer is a workable way to do so.
Yes.
> However, when taken together with [1/2], doesn't the proposed patch
> "achieves" a lot more than "exactly that", namely, by not treating
> symbolic links discovered during traversals of directories given
> from the command line as such and dereferencing?
It's not clear to me what you are saying here - 1/2 makes git diff
follow symbolic links, yes, just like ordinary diff. If I 'diff' two
dirs that contain symbolic links with the same name pointing to
different files I get a diff between the contents, not between the
filenames.
I like the proposed change a lot, maybe that didn't come across clearly.
I think it makes things more "predictable" in the sense that it meets
typical expectations.
Michael
^ permalink raw reply
* RFC: Enable delayed responses to Git clean/smudge filter requests
From: Lars Schneider @ 2016-11-14 21:09 UTC (permalink / raw)
To: Git Mailing List
Hi,
Git always performs a clean/smudge filter on files in sequential order.
Sometimes a filter operation can take a noticeable amount of time.
This blocks the entire Git process.
I would like to give a filter process the possibility to answer Git with
"I got your request, I am processing it, ask me for the result later!".
I see the following way to realize this:
In unpack-trees.c:check_updates() [1] we loop through the cache
entries and "ask me later" could be an acceptable return value of the
checkout_entry() call. The loop could run until all entries returned
success or error.
The filter machinery is triggered in various other places in Git and
all places that want to support "ask me later" would need to be patched
accordingly.
--
Do you think this could be a viable approach?
Do you see a better way?
Thanks,
Lars
[1] https://github.com/git/git/blob/3ab228137f980ff72dbdf5064a877d07bec76df9/unpack-trees.c#L267
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-14 21:10 UTC (permalink / raw)
To: Michael J Gruber
Cc: Johannes Schindelin, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <a3db4c55-550c-f2e8-83b8-46c2be86f7da@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> *My* idea of --no-index was for it to behave as similar to the
>>> --index-version as possible, regarding formatting etc., and to be a good
>>> substitute for ordinary diff. The proposed patch achieves exactly that -
>> ...
> It's not clear to me what you are saying here - 1/2 makes git diff
> follow symbolic links, yes, just like ordinary diff.
Yes, which can be seen as deviating from your earlier "as similar to
the --index version as possible" goal, which I think was where Dscho's
complaint comes from.
I _think_ the no-index mode was primarily for those who want to use
our diff as a replacement for GNU and other diffs, and from that
point of view, I'd favour not doing the "comparing symbolic link?
We'll show the difference between the link contents, not target"
under no-index mode myself. That is a lot closer to the diff other
people implemented, not ours. Hence the knee-jerk reaction I gave
in
http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com
^ 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