* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 21:57 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222213542.opunuepfmj557zyr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
>>
>> Thanks for your thoughts. I'd think that we should take this change
>> and leave the optimization for later, then. It's not like the
>> change of the default is making the normal situation any worse, it
>> seems.
>
> I'm not excited that it will start making known bogus-username requests
> by default to servers which do not even support Negotiate. I guess that
> is really the server-operators problem, but it feels pretty hacky.
I guess that's another valid concern. The servers used to be able
to say "Ah, this repository needs auth and this request does not, so
reject it without asking the auth-db". Now it must say "Ah, this
repository needs auth and this request does have one, but it is
empty so let's not even bother the auth-db" in order to reject a
useless "empty-auth" request with the same efficiency.
After the first request without auth (that fails), do we learn
anything useful from the server side (like "it knows Negotiate")
that we can use to flip the "empty-auth" bit to give a better
default to people from both worlds, I wonder...?
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 21:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqqwpchanxz.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
> >>
> >> Thanks for your thoughts. I'd think that we should take this change
> >> and leave the optimization for later, then. It's not like the
> >> change of the default is making the normal situation any worse, it
> >> seems.
> >
> > I'm not excited that it will start making known bogus-username requests
> > by default to servers which do not even support Negotiate. I guess that
> > is really the server-operators problem, but it feels pretty hacky.
>
> I guess that's another valid concern. The servers used to be able
> to say "Ah, this repository needs auth and this request does not, so
> reject it without asking the auth-db". Now it must say "Ah, this
> repository needs auth and this request does have one, but it is
> empty so let's not even bother the auth-db" in order to reject a
> useless "empty-auth" request with the same efficiency.
>
> After the first request without auth (that fails), do we learn
> anything useful from the server side (like "it knows Negotiate")
> that we can use to flip the "empty-auth" bit to give a better
> default to people from both worlds, I wonder...?
Yes, that's exactly what I was trying to say in my first message.
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 22:35 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222215833.d7htyo32ptfse5l4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
>> >>
>> >> Thanks for your thoughts. I'd think that we should take this change
>> >> and leave the optimization for later, then. It's not like the
>> >> change of the default is making the normal situation any worse, it
>> >> seems.
>> >
>> > I'm not excited that it will start making known bogus-username requests
>> > by default to servers which do not even support Negotiate. I guess that
>> > is really the server-operators problem, but it feels pretty hacky.
>>
>> I guess that's another valid concern. The servers used to be able
>> to say "Ah, this repository needs auth and this request does not, so
>> reject it without asking the auth-db". Now it must say "Ah, this
>> repository needs auth and this request does have one, but it is
>> empty so let's not even bother the auth-db" in order to reject a
>> useless "empty-auth" request with the same efficiency.
>>
>> After the first request without auth (that fails), do we learn
>> anything useful from the server side (like "it knows Negotiate")
>> that we can use to flip the "empty-auth" bit to give a better
>> default to people from both worlds, I wonder...?
>
> Yes, that's exactly what I was trying to say in my first message.
I see. I am still inclined to take this as-is for now to cook in
'next', though.
A solution along your line would help Negotiate users OOB experience
without hurting the servers that do not offer Negotiate, but until
that materializes, users can set the lazier http.emptyAuth on
(without selectively setting http.<host>.emptyAuth off for sites
without Negotiate) and hurt the servers by throwing an empty auth
anyway regardless of the default, so the flipping of the default is
not fundamentally adding more harm in that sense.
^ permalink raw reply
* Re: feature request: user email config per domain
From: Andrew Ardill @ 2017-02-22 23:21 UTC (permalink / raw)
To: Tushar Kapila; +Cc: git@vger.kernel.org
In-Reply-To: <CAN0Skmmjd5Y0uWz_WC69mAStucZ6nR0mjdp4-ODJz2UnTaB-eQ@mail.gmail.com>
On 23 February 2017 at 00:12, Tushar Kapila <tgkprog@gmail.com> wrote:
> I can set my email via:
> git config --global user.email tgkprog@xyz.dom
>
> this is dangerous when I use this my office or in a multi repository
> provider environment where my email is different for a few (like
> tgkprog@search.com for github and tushar@mycompany.com for my company
> private repo).
There has been a large discussion around this idea before, see this
thread for example [0].
The proposed idea was to have something set in your global config that
would only be turned on if you were within a given directory. This
would allow you to have two root directories, one for your work
projects and one for open source projects (for example) and any git
repositories within those folders would each would have their own
config options automatically applied based on their location.
There was a patch suggested, and it worked quite well, but nothing
further has been done to my knowledge.
[0] http://public-inbox.org/git/7vvc3d1o01.fsf@alter.siamese.dyndns.org/
Regards,
Andrew Ardill
^ permalink raw reply
* Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id
From: brian m. carlson @ 2017-02-22 23:22 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Michael Haggerty, Ramsay Jones
In-Reply-To: <20170222191641.o2rtt2ymtb4h2yqe@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]
On Wed, Feb 22, 2017 at 02:16:42PM -0500, Jeff King wrote:
> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
>
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> > > Convert most leaf functions to struct object_id. Change several
> > > hardcoded numbers to uses of parse_oid_hex. In doing so, verify that we
> > > when we want two trees, we have exactly two trees.
> > >
> > > Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> > > This will be a NUL as well, since the first NUL was a newline we
> > > overwrote. However, with parse_oid_hex, we no longer need to increment
> > > the pointer directly, and can simply increment it as part of our check
> > > for the space character.
> >
> > After reading the pre- and post-image twice, I think I convinced
> > myself that this is a faithful conersion and they do the same thing.
>
> I think this is correct, too (but then, I think it largely comes from
> the patch I wrote the other night. So I did look at it carefully, but
> it's not exactly an independent review).
I did take that part, as well as other parts, from your patch.
I have a test, which I'll send in a minute, that verifies that they do
the same thing. At first I introduced a segfault, and the test caught
it, so I feel confident that the patch does indeed function as the old
code did.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 23:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqqshn5am74.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 02:35:11PM -0800, Junio C Hamano wrote:
> A solution along your line would help Negotiate users OOB experience
> without hurting the servers that do not offer Negotiate, but until
> that materializes, users can set the lazier http.emptyAuth on
> (without selectively setting http.<host>.emptyAuth off for sites
> without Negotiate) and hurt the servers by throwing an empty auth
> anyway regardless of the default, so the flipping of the default is
> not fundamentally adding more harm in that sense.
I was hoping to materialize it today. :)
Here's what I came up with. I have a lot of questions about the second
patch which I'll outline there. But I think it may be a good start.
[1/2]: http: restrict auth methods to what the server advertises
[2/2]: http: add an "auto" mode for http.emptyauth
http.c | 38 +++++++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: brian m. carlson @ 2017-02-22 23:34 UTC (permalink / raw)
To: David Turner
Cc: 'Junio C Hamano', git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <97ab9a812f7b46d7b10d4d06f73259d8@exmbdft7.ad.twosigma.com>
[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]
On Wed, Feb 22, 2017 at 09:04:14PM +0000, David Turner wrote:
> > -----Original Message-----
> > From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C
> > Hamano
> > Sent: Wednesday, February 22, 2017 3:20 PM
> > To: David Turner <David.Turner@twosigma.com>
> > Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; Johannes Schindelin
> > <johannes.schindelin@gmx.de>; Eric Sunshine
> > <sunshine@sunshineco.com>; Jeff King <peff@peff.net>
> > Subject: Re: [PATCH] http(s): automatically try NTLM authentication first
> >
> >
> > Some other possible worries we may have had I can think of are:
> >
> > - With this enabled unconditionally, would we leak some information?
>
> I think "NTLM" is actually a misnomer here (I just copied Johannes's
> commit message). The mechanism is actually SPNEGO, if I understand this
> correctly. It seems to me that this is probably secure, since it is apparently
> widely implemented in browsers.
This is SPNEGO. It will work with NTLM as well as Kerberos.
Browsers usually disable this feature by default, as it basically will
attempt to authenticate to any site that sends a 401. For Kerberos
against a malicious site, the user will either not have a valid ticket
for that domain, or the user's Kerberos server will refuse to provide a
ticket to pass to the server, so there's no security risk involved.
I'm unclear how SPNEGO works with NTLM, so I can't speak for the
security of it. From what I understand of NTLM and from RFC 4559, it
consists of a shared secret. I'm unsure what security measures are in
place to not send that to an untrusted server.
As far as Kerberos, this is a desirable feature to have enabled, with
little downside. I just don't know about the security of the NTLM part,
and I don't think we should take this patch unless we're sure we know
the consequences of it.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [RFC 03/14] upload-pack: test negotiation with changing repo
From: Junio C Hamano @ 2017-02-22 23:36 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <0c727063-0583-f713-68fb-bd284be696b2@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> This somehow looks like a good thing to do even in production. Am I
>> mistaken?
>
> Yes, that's true. If this patch set stalls (for whatever reason), I'll
> spin this off into an independent patch.
... which may be needed.
As to the main goal of this topic, I think the "ask by refname (with
glob)" is very good thing to start the "client speaks first" v2
protocol, as it would allow us to reduce the size of the initial
advertisement for common cases (i.e. remote.<name>.fetch is likely
to list only refs/heads/* on the left hand side of a refspec). And
adding this to v1 is probably a good first step to make sure the
code that is currently used by v1 protocol exchange that will be
shared with the v2 protocols are prepared to be driven by refname
without knowing the exact object name until the final round.
^ permalink raw reply
* [PATCH] t: add multi-parent test of diff-tree --stdin
From: brian m. carlson @ 2017-02-22 23:38 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Michael Haggerty, Ramsay Jones
In-Reply-To: <20170222232215.u2agozagwsyy2ooe@genre.crustytoothpaste.net>
We were lacking a test that covered the multi-parent case for commits.
Add a basic test to ensure we do not regress this behavior in the
future.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
t/t4013-diff-various.sh | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
It's a little bit ugly to me that this test hard-codes so many values,
and I'm concerned that it may be unnecessarily brittle. However, I
don't have a good idea of how to perform the kind of comprehensive test
we need otherwise.
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index d09acfe48e..e6c2a7a369 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -349,4 +349,23 @@ test_expect_success 'diff-tree --stdin with log formatting' '
test_cmp expect actual
'
+test_expect_success 'diff-tree --stdin with two parent commits' '
+ cat >expect <<-\EOF &&
+ c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+ :040000 040000 da7a33fa77d8066d6698643940ce5860fe2d7fb3 f977ed46ae6873c1c30ab878e15a4accedc3618b M dir
+ :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d f4615da674c09df322d6ba8d6b21ecfb1b1ba510 M file0
+ :000000 100644 0000000000000000000000000000000000000000 7289e35bff32727c08dda207511bec138fdb9ea5 A file3
+ 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+ :040000 040000 847b63d013de168b2de618c5ff9720d5ab27e775 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 M dir
+ :000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+ 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+ :040000 040000 da7a33fa77d8066d6698643940ce5860fe2d7fb3 847b63d013de168b2de618c5ff9720d5ab27e775 M dir
+ :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d b414108e81e5091fe0974a1858b4d0d22b107f70 M file0
+ :100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+ EOF
+ git rev-list --parents master | git diff-tree --stdin >actual &&
+ test_cmp expect actual
+'
+
+
test_done
^ permalink raw reply related
* [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: Jeff King @ 2017-02-22 23:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net>
This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.
However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.
Instead, let's introduce an automatic mode, that works like
this:
1. We won't try to send the bogus credential on the first
request. We'll wait to get an HTTP 401, as usual.
2. After seeing an HTTP 401, the empty-auth hack will kick
in only when we know there is an auth method beyond
"Basic" to be tried.
That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.
This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).
The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.
Signed-off-by: Jeff King <peff@peff.net>
---
My open questions are:
- I don't have anything but a Basic server to test against. So it's
entirely possible that this doesn't actually work in the NTLM case.
- what does a request log look like for somebody actually using NTLM?
It's possible if the initial request sends a restrict auth_avail,
that curl could get away without the extra probe request, and we'd
end up with the same number of requests for "auto" mode versus
http.emptyauth=true.
- the whole "don't use this on the initial request" flag feels really
hacky. It's a side effect of how emptyauth tries to kick in even
before we have sent any requests. Probably it should have been
handled in the 401 code path originally, but I'm hesitant to change
it now. I suspect it is eliminating a round-trip in practice when it
is enabled.
- I didn't test a server that advertises Basic and something else, but
really only takes Basic. So I'm just assuming that it incurs the
extra round-trip (actually probably two, one for curl's method
probe).
- When your curl is too old to do CURLAUTH_ANY, I just left the
default to disable emptyauth. But it could easily be "1" if people
care.
http.c | 38 ++++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index a05609766..ea70ec1ee 100644
--- a/http.c
+++ b/http.c
@@ -109,7 +109,7 @@ static int curl_save_cookies;
struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
@@ -125,6 +125,7 @@ static struct credential cert_auth = CREDENTIAL_INIT;
static int ssl_cert_password_required;
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
#endif
static struct curl_slist *pragma_header;
@@ -382,10 +383,37 @@ static int http_options(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
+static int curl_empty_auth_enabled(void)
+{
+ if (curl_empty_auth < 0) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+ /*
+ * In the automatic case, kick in the empty-auth
+ * hack as long as we would potentially try some
+ * method more exotic than "Basic".
+ *
+ * But only do so when this is _not_ our initial
+ * request, as we would not then yet know what
+ * methods are available.
+ */
+ return http_auth_methods_restricted &&
+ http_auth_methods != CURLAUTH_BASIC;
+#else
+ /*
+ * Our libcurl is too old to do AUTH_ANY in the first place;
+ * just default to turning the feature off.
+ */
+ return 0;
+#endif
+ }
+
+ return curl_empty_auth;
+}
+
static void init_curl_http_auth(CURL *result)
{
if (!http_auth.username || !*http_auth.username) {
- if (curl_empty_auth)
+ if (curl_empty_auth_enabled())
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
}
@@ -1079,7 +1107,7 @@ struct active_request_slot *get_active_slot(void)
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
#endif
- if (http_auth.password || curl_empty_auth)
+ if (http_auth.password || curl_empty_auth_enabled())
init_curl_http_auth(slot->curl);
return slot;
@@ -1347,8 +1375,10 @@ static int handle_curl_result(struct slot_results *results)
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
- if (results->auth_avail)
+ if (results->auth_avail) {
http_auth_methods &= results->auth_avail;
+ http_auth_methods_restricted = 1;
+ }
#endif
return HTTP_REAUTH;
}
--
2.12.0.rc2.597.g959f68882
^ permalink raw reply related
* [PATCH 1/2] http: restrict auth methods to what the server advertises
From: Jeff King @ 2017-02-22 23:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net>
By default, we tell curl to use CURLAUTH_ANY, which does not
limit its set of auth methods. However, this results in an
extra round-trip to the server when authentication is
required. After we've fed the credential to curl, it wants
to probe the server to find its list of available methods
before sending an Authorization header.
We can shortcut this by limiting our http_auth_methods by
what the server told us it supports. In some cases (such as
when the server only supports Basic), that lets curl skip
the extra probe request.
The end result should look the same to the user, but you can
use GIT_TRACE_CURL to verify the sequence of requests:
GIT_TRACE_CURL=1 \
git ls-remote https://example.com/repo.git \
2>&1 >/dev/null |
egrep '(Send|Recv) header: (GET|HTTP|Auth)'
Before this patch, hitting a Basic-only server like
github.com results in:
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Recv header: HTTP/1.1 401 Authorization Required
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Recv header: HTTP/1.1 401 Authorization Required
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Send header: Authorization: Basic <redacted>
Recv header: HTTP/1.1 200 OK
And after:
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Recv header: HTTP/1.1 401 Authorization Required
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Send header: Authorization: Basic <redacted>
Recv header: HTTP/1.1 200 OK
The possible downsides are:
- This only helps for a Basic-only server; for a server
with multiple auth options, curl may still send a probe
request to see which ones are available (IOW, there's no
way to say "don't probe, I already know what the server
will say").
- The http_auth_methods variable is global, so this will
apply to all further requests. That's acceptable for
Git's usage of curl, though, which also treats the
credentials as global. I.e., in any given program
invocation we hit only one conceptual server (we may be
redirected at the outset, but in that case that's whose
auth_avail field we'd see).
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index 90a1c0f11..a05609766 100644
--- a/http.c
+++ b/http.c
@@ -1347,6 +1347,8 @@ static int handle_curl_result(struct slot_results *results)
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+ if (results->auth_avail)
+ http_auth_methods &= results->auth_avail;
#endif
return HTTP_REAUTH;
}
--
2.12.0.rc2.597.g959f68882
^ permalink raw reply related
* Re: [PATCH] t: add multi-parent test of diff-tree --stdin
From: Junio C Hamano @ 2017-02-22 23:42 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King, Michael Haggerty, Ramsay Jones
In-Reply-To: <20170222233838.925157-1-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> We were lacking a test that covered the multi-parent case for commits.
> Add a basic test to ensure we do not regress this behavior in the
> future.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> t/t4013-diff-various.sh | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> It's a little bit ugly to me that this test hard-codes so many values,
> and I'm concerned that it may be unnecessarily brittle. However, I
> don't have a good idea of how to perform the kind of comprehensive test
> we need otherwise.
Hmph, perhaps the expectation can be created out of the output from
'git diff-tree master^ master' or something?
>
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index d09acfe48e..e6c2a7a369 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -349,4 +349,23 @@ test_expect_success 'diff-tree --stdin with log formatting' '
> test_cmp expect actual
> '
>
> +test_expect_success 'diff-tree --stdin with two parent commits' '
> + cat >expect <<-\EOF &&
> + c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
> + :040000 040000 da7a33fa77d8066d6698643940ce5860fe2d7fb3 f977ed46ae6873c1c30ab878e15a4accedc3618b M dir
> + :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d f4615da674c09df322d6ba8d6b21ecfb1b1ba510 M file0
> + :000000 100644 0000000000000000000000000000000000000000 7289e35bff32727c08dda207511bec138fdb9ea5 A file3
> + 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
> + :040000 040000 847b63d013de168b2de618c5ff9720d5ab27e775 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 M dir
> + :000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
> + 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
> + :040000 040000 da7a33fa77d8066d6698643940ce5860fe2d7fb3 847b63d013de168b2de618c5ff9720d5ab27e775 M dir
> + :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d b414108e81e5091fe0974a1858b4d0d22b107f70 M file0
> + :100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
> + EOF
> + git rev-list --parents master | git diff-tree --stdin >actual &&
> + test_cmp expect actual
> +'
> +
> +
> test_done
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 23:42 UTC (permalink / raw)
To: brian m. carlson, David Turner, 'Junio C Hamano',
git@vger.kernel.org, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222233419.q3fxqmrscosumbjm@genre.crustytoothpaste.net>
On Wed, Feb 22, 2017 at 11:34:19PM +0000, brian m. carlson wrote:
> Browsers usually disable this feature by default, as it basically will
> attempt to authenticate to any site that sends a 401. For Kerberos
> against a malicious site, the user will either not have a valid ticket
> for that domain, or the user's Kerberos server will refuse to provide a
> ticket to pass to the server, so there's no security risk involved.
>
> I'm unclear how SPNEGO works with NTLM, so I can't speak for the
> security of it. From what I understand of NTLM and from RFC 4559, it
> consists of a shared secret. I'm unsure what security measures are in
> place to not send that to an untrusted server.
>
> As far as Kerberos, this is a desirable feature to have enabled, with
> little downside. I just don't know about the security of the NTLM part,
> and I don't think we should take this patch unless we're sure we know
> the consequences of it.
Hmm. That would be a problem with my proposed patch 2 then, too, if only
because it turns the feature on by default in more places.
If it _is_ dangerous to turn on all the time, I'd think we should
consider warning people in the http.emptyauth documentation.
-Peff
^ permalink raw reply
* Re: [PATCH] t: add multi-parent test of diff-tree --stdin
From: Jeff King @ 2017-02-22 23:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git, Michael Haggerty, Ramsay Jones
In-Reply-To: <xmqqfuj5aj32.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 03:42:25PM -0800, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > We were lacking a test that covered the multi-parent case for commits.
> > Add a basic test to ensure we do not regress this behavior in the
> > future.
> >
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > t/t4013-diff-various.sh | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > It's a little bit ugly to me that this test hard-codes so many values,
> > and I'm concerned that it may be unnecessarily brittle. However, I
> > don't have a good idea of how to perform the kind of comprehensive test
> > we need otherwise.
>
> Hmph, perhaps the expectation can be created out of the output from
> 'git diff-tree master^ master' or something?
I had a similar thought. It may also be worth testing that:
echo "$(git rev-parse master) $(git rev-parse other)" | git diff-tree --stdin
shows the diff between "other" and "master", and not just "master^" and
"master" (i.e., it is not clear from the test expectation that the code
actually is parsing the second parent and not accidentally ignoring it).
For completeness, it would probably make sense to test the multi-parent
case, too.
-Peff
^ permalink raw reply
* Re: [PATCH] t: add multi-parent test of diff-tree --stdin
From: brian m. carlson @ 2017-02-22 23:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Michael Haggerty, Ramsay Jones
In-Reply-To: <xmqqfuj5aj32.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
On Wed, Feb 22, 2017 at 03:42:25PM -0800, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > We were lacking a test that covered the multi-parent case for commits.
> > Add a basic test to ensure we do not regress this behavior in the
> > future.
> >
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > t/t4013-diff-various.sh | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > It's a little bit ugly to me that this test hard-codes so many values,
> > and I'm concerned that it may be unnecessarily brittle. However, I
> > don't have a good idea of how to perform the kind of comprehensive test
> > we need otherwise.
>
> Hmph, perhaps the expectation can be created out of the output from
> 'git diff-tree master^ master' or something?
Okay. I'll try that in a reroll.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH] submodule init: warn about falling back to a local path
From: Stefan Beller @ 2017-02-23 0:24 UTC (permalink / raw)
To: Philip Oakley
Cc: Jonathan Nieder, git@vger.kernel.org, Junio C Hamano,
Shawn Pearce
In-Reply-To: <F50FAF9C5F724A87B5424494262E1BAD@PhilipOakley>
On Wed, Feb 22, 2017 at 7:01 AM, Philip Oakley <philipoakley@iee.org> wrote:
>
> Would a note in the user docs about the fallback you list above also be
> useful?
duh! yes it would! Will look at the documentation and reroll.
^ permalink raw reply
* Re: feature request: user email config per domain
From: Jeff King @ 2017-02-23 0:42 UTC (permalink / raw)
To: Tushar Kapila; +Cc: git
In-Reply-To: <CAN0Skmmjd5Y0uWz_WC69mAStucZ6nR0mjdp4-ODJz2UnTaB-eQ@mail.gmail.com>
On Wed, Feb 22, 2017 at 06:42:02PM +0530, Tushar Kapila wrote:
> Feature request : can we have a config for email per repo domain ?
> Something like:
>
> git config --global domain.user.email tgkprog@test.xyz.com
> testing.abc.doman:8080
>
> git config --global domain.user.email tgkprog@xyz.com abc.doman:80
>
> git config --global domain.user.email tgkprog@search.com github.com
>
> So when remote URL has github.com push as tgkprog@search.com but for
> testing.abc.doman:8080 use tgkprog@test.xyz.com ?
The idea of "the domain for this repo" doesn't really match Git's
distributed model. A repo may have no remotes at all, or multiple
remotes with different domains (or even remotes which do not have a
domain associated with them, like local files, or remote helpers which
obscure the domain name).
It sounds like you're assuming that the domain would come from looking
at remote.origin.url. Which I agree would work in the common case of
"git clone https://example.com", but I'm not comfortable with the number
of corner cases the feature has.
I'd much rather see something based on the working tree path, like Duy's
conditional config includes. Then you write something in your
~/.gitconfig like:
[include "gitdir:/home/peff/work/"]
path = .gitconfig-work
[include "gitdir:/home/peff/play/"]
path = .gitconfig-play
and set user.email as appropriate in each.
You may also set user.useConfigOnly in your ~/.gitconfig. Then you'd
have to set user.email in each individual repository, but at least Git
would complain and remind you when you forget to do so, rather than
silently using the wrong email.
-Peff
^ permalink raw reply
* RE: [PATCH] http(s): automatically try NTLM authentication first
From: David Turner @ 2017-02-23 1:03 UTC (permalink / raw)
To: 'brian m. carlson'
Cc: 'Junio C Hamano', git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <20170222233419.q3fxqmrscosumbjm@genre.crustytoothpaste.net>
> -----Original Message-----
> From: brian m. carlson [mailto:sandals@crustytoothpaste.net]
>
> This is SPNEGO. It will work with NTLM as well as Kerberos.
>
> Browsers usually disable this feature by default, as it basically will attempt to
> authenticate to any site that sends a 401. For Kerberos against a malicious
> site, the user will either not have a valid ticket for that domain, or the user's
> Kerberos server will refuse to provide a ticket to pass to the server, so
> there's no security risk involved.
>
> I'm unclear how SPNEGO works with NTLM, so I can't speak for the security
> of it. From what I understand of NTLM and from RFC 4559, it consists of a
> shared secret. I'm unsure what security measures are in place to not send
> that to an untrusted server.
>
> As far as Kerberos, this is a desirable feature to have enabled, with little
> downside. I just don't know about the security of the NTLM part, and I don't
> think we should take this patch unless we're sure we know the
> consequences of it.
NTLM on its own is bad:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa378749(v=vs.85).aspx
says:
"
1. (Interactive authentication only) A user accesses a client computer and
provides a domain name, user name, and password. The client computes a
cryptographic hash of the password and discards the actual password.
2. The client sends the user name to the server (in plaintext).
3. The server generates a 16-byte random number, called a challenge or
nonce, and sends it to the client.
4. The client encrypts this challenge with the hash of the user's password
and returns the result to the server. This is called the response.
..."
Wait, what? If I'm a malicious server, I can get access to an offline oracle
for whether I've correctly guessed the user's password? That doesn't
sound secure at all! Skimming the SPNEGO RFCs, there appears to be no
mitigation for this.
So, I guess, this patch might be considered a security risk. But on the
other hand, even *without* this patch, and without http.allowempty at
all, I think a config which simply uses a https:// url without the magic :@
would try SPNEGO. As I understand it, the http.allowempty config just
makes the traditional :@ urls work.
Actually, though, I am not sure this is as bad as it seems, because gssapi
might protect us. When I locally tried a fake server, git (libcurl) refused to
send my Kerberos credentials because "Server not found in Kerberos
database". I don't have a machine set up with NTLM authentication
(because, apparently, that would be insane), so I don't know how to
confirm that gssapi would operate off of a whitelist for NTLM as well.
^ permalink raw reply
* RE: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: David Turner @ 2017-02-23 1:16 UTC (permalink / raw)
To: 'Jeff King', Junio C Hamano
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222234059.iajn2zuwzkzjxit2@sigill.intra.peff.net>
I don't know enough about how libcurl handles authentication to know whether
these patches are a good idea, but I have a minor comment anyway.
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> +static int curl_empty_auth_enabled(void) {
> + if (curl_empty_auth < 0) {
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> + /*
> + * In the automatic case, kick in the empty-auth
> + * hack as long as we would potentially try some
> + * method more exotic than "Basic".
> + *
> + * But only do so when this is _not_ our initial
> + * request, as we would not then yet know what
> + * methods are available.
> + */
Eliminate double-negative:
"But only do this when this is our second or subsequent request,
as by then we know what methods are available."
^ permalink raw reply
* Re: feature request: user email config per domain
From: Igor Djordjevic BugA @ 2017-02-23 1:30 UTC (permalink / raw)
To: Tushar Kapila, git
In-Reply-To: <CAN0Skmmjd5Y0uWz_WC69mAStucZ6nR0mjdp4-ODJz2UnTaB-eQ@mail.gmail.com>
Hi Tushar,
On 22/02/2017 14:12, Tushar Kapila <tgkprog@gmail.com> wrote:
> So when remote URL has github.com push as tgkprog@search.com but for
> testing.abc.doman:8080 use tgkprog@test.xyz.com ?
I`m not sure if this is sensible, as authorship information is baked
into the commit at the time of committing, which (usually ;) happens
before you get to 'git push' to the other repo.
If possible, changing this info after the fact, on 'git push', would
influence the existing commit you`re trying to send over, so your
'git-push' would have a surprising consequence of not actually
pushing your desired commit at all, but creating a totally new commit
inside the other repo -- this new commit would be exactly the same
patch-wise (in regards to differences introduced), but because of the
changed user info it would be considered a different commit
nonetheless (different hash).
> ... I know I can over ride it per repository, but sometimes
> forget to do that. And even if I unset it, it inadvertantly gets set
> elsewhere when I make a repo and the site 'helps' me by showing me the
> commands to init and clone my new repo.
Otherwise, as you already stated that you find the current local (per
repo) user settings override logic inconvenient (error-prone), you
might be interested in approach described in this[1] Stack Overflow
post.
In short, it uses a template-injected 'post-checkout' hook (triggered
on 'git clone' as well) alongside '.gitconfig' (global) settings to
achieve what seems to be pretty similar to what you asked for (but
might be a bit more sensible), where you may fine-tune it further to
better suit your needs.
On 20/02/2017 21:12, Grant Humphries[2] wrote[1]:
> This answer is partially inspired by the post by @Saucier, but I was
> looking for an automated way to set user.name and user.email on a per
> repo basis, based on the remote, that was a little more light weight
> than the git-passport package that he developed. Also h/t to @John
> for the useConfigOnly setting. Here is my solution:
>
> .gitconfig changes:
>
> [github]
> name = <github username>
> email = <github email>
> [gitlab]
> name = <gitlab username>
> email = <gitlab email>
> [init]
> templatedir = ~/.git-templates
> [user]
> useConfigOnly = true
>
> post-checkout hook which should be saved to the following path:
> ~/.git-templates/hooks/post-checkout:
>
> #!/usr/bin/env bash
>
> # make regex matching below case insensitive
> shopt -s nocasematch
>
> # values in the services array should have a corresponding section in
> # .gitconfig where the 'name' and 'email' for that service are specified
> remote_url="$( git config --get --local remote.origin.url )"
> services=(
> 'github'
> 'gitlab'
> )
>
> set_local_user_config() {
> local service="${1}"
> local config="${2}"
> local service_config="$( git config --get ${service}.${config} )"
> local local_config="$( git config --get --local user.${config} )"
>
> if [[ "${local_config}" != "${service_config}" ]]; then
> git config --local "user.${config}" "${service_config}"
> echo "repo 'user.${config}' has been set to '${service_config}'"
> fi
> }
>
> # if remote_url doesn't contain the any of the values in the services
> # array the user name and email will remain unset and the
> # user.useConfigOnly = true setting in .gitconfig will prompt for those
> # credentials and prevent commits until they are defined
> for s in "${services[@]}"; do
> if [[ "${remote_url}" =~ "${s}" ]]; then
> set_local_user_config "${s}" 'name'
> set_local_user_config "${s}" 'email'
> break
> fi
> done
>
> I use different credentials for github and gitlab, but those
> references in the code above could be replaced or augmented with any
> service that you use. In order to have the post-checkout hook
> automatically set the user name and email locally for a repo after a
> checkout make sure the service name appears in the remote url, add it
> to the services array in the post-checkout script and create a
> section for it in your .gitconfig that contains your user name and
> email for that service.
>
> If none of the service names appear in the remote url or the repo
> doesn't have a remote the user name and email will not be set
> locally. In these cases the user.useConfigOnly setting will be in
> play which will not allow you to make commits until the user name and
> email are set at the repo level, and will prompt the user to
> configure that information.
Regards,
Buga
*P.S.* For the purpose of completeness and archiving I copied the
Stack Overflow post[1] here as well, but all the credits go to its
author[2] (you may upvote the linked post[1] if you find it helpful).
Please feel free let me know if this practice is otherwise to be
avoided.
[1] http://stackoverflow.com/a/42354282
[2] http://stackoverflow.com/users/2167004
^ permalink raw reply
* Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: Jeff King @ 2017-02-23 1:37 UTC (permalink / raw)
To: David Turner
Cc: Junio C Hamano, git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <b5778a7988ad4dfa9adfc8d312432189@exmbdft7.ad.twosigma.com>
On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:
> I don't know enough about how libcurl handles authentication to know whether
> these patches are a good idea, but I have a minor comment anyway.
As somebody who is using non-Basic auth, can you apply these patches and
show us the output of:
GIT_TRACE_CURL=1 \
git ls-remote https://your-server 2>&1 >/dev/null |
egrep '(Send|Recv) header: (GET|HTTP|Auth)'
(without http.emptyauth turned on, obviously).
> > + * But only do so when this is _not_ our initial
> > + * request, as we would not then yet know what
> > + * methods are available.
> > + */
>
> Eliminate double-negative:
>
> "But only do this when this is our second or subsequent request,
> as by then we know what methods are available."
Yeah, that is clearer.
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-23 2:15 UTC (permalink / raw)
To: Jeff King
Cc: brian m. carlson, David Turner, git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222234246.wjp3567vesdusiaf@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 11:34:19PM +0000, brian m. carlson wrote:
>
>> Browsers usually disable this feature by default, as it basically will
>> attempt to authenticate to any site that sends a 401. For Kerberos
>> against a malicious site, the user will either not have a valid ticket
>> for that domain, or the user's Kerberos server will refuse to provide a
>> ticket to pass to the server, so there's no security risk involved.
>>
>> I'm unclear how SPNEGO works with NTLM, so I can't speak for the
>> security of it. From what I understand of NTLM and from RFC 4559, it
>> consists of a shared secret. I'm unsure what security measures are in
>> place to not send that to an untrusted server.
>>
>> As far as Kerberos, this is a desirable feature to have enabled, with
>> little downside. I just don't know about the security of the NTLM part,
>> and I don't think we should take this patch unless we're sure we know
>> the consequences of it.
>
> Hmm. That would be a problem with my proposed patch 2 then, too, if only
> because it turns the feature on by default in more places.
>
> If it _is_ dangerous to turn on all the time, I'd think we should
> consider warning people in the http.emptyauth documentation.
Yeah, http.<url>.emptyAuth that knows where it is going may be a lot
safer but a blanket http.emptyAuth does sound bad.
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: brian m. carlson @ 2017-02-23 4:19 UTC (permalink / raw)
To: David Turner
Cc: 'Junio C Hamano', git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <b152fad7e79046c5aa6cac9e21066c1c@exmbdft7.ad.twosigma.com>
[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]
On Thu, Feb 23, 2017 at 01:03:39AM +0000, David Turner wrote:
> So, I guess, this patch might be considered a security risk. But on the
> other hand, even *without* this patch, and without http.allowempty at
> all, I think a config which simply uses a https:// url without the magic :@
> would try SPNEGO. As I understand it, the http.allowempty config just
> makes the traditional :@ urls work.
No, it's a bit different. libcurl won't try to authenticate to a server
unless it has a username (and possibly password). With the curl command
line client, you use a dummy value or -u: to force it to do auth anyway
(because you want, say, GSSAPI). http.emptyAuth just sets that option
to “:” so libcurl will auth:
if (curl_empty_auth)
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
I just use a dummy username for my URLs, but you can write :@ or any
other permutation to get it to work without emptyAuth. As a
consequence, you have to opt-in to that on a per-URL (or per-domain)
basis, which is a bit more secure.
> Actually, though, I am not sure this is as bad as it seems, because gssapi
> might protect us. When I locally tried a fake server, git (libcurl) refused to
> send my Kerberos credentials because "Server not found in Kerberos
> database". I don't have a machine set up with NTLM authentication
> (because, apparently, that would be insane), so I don't know how to
> confirm that gssapi would operate off of a whitelist for NTLM as well.
Yup. That's pretty much what I thought would happen, since the Kerberos
server has no HTTP/malicious.evil.tld@YOURREALM.TLD service ticket.
Again, I don't know how NTLM does things, or if it's wrapped in a
suitable ticket format somehow.
Last I base64-decoded an NTLM SPNEGO response, it did not contain the
OID required by GSSAPI as a prefix; it instead contained an “NTLMSSP”
header, which isn't a valid OID. I didn't delve much further, since I
was pretty sure I didn't want to know more.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* git email From: parsing (was Re: [GIT PULL] Staging/IIO driver patches for 4.11-rc1)
From: Greg KH @ 2017-02-23 6:04 UTC (permalink / raw)
To: Linus Torvalds, Simon Sandström
Cc: Andrew Morton, git, Linux Kernel Mailing List,
Linux Driver Project
In-Reply-To: <CA+55aFy1JpXmo_PpC7f0zZa0YAP6rz+bztJ+fpDUoWgCz0_FMw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]
On Wed, Feb 22, 2017 at 11:59:01AM -0800, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 6:56 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > =?UTF-8?q?Simon=20Sandstr=C3=B6m?= (1):
> > staging: vt6656: Add missing identifier names
>
> Wow, your scripts really screwed up that name.
>
> I'm assuming this is quilt not doing proper character set handling..
>
> Because if it's git, we need to get that fixed (but I'm pretty sure
> git gets this right - there are various tests for email header
> quoting).
>
> Alternatively, somebody hand-edited some email and moved the From:
> header to the body without fixing up the RFC 1342 mail header quoting
> (which is very different from how quoting works in the *body* of an
> email).
>
> Poor Simon Sandström.
>
> Funnily enough, this only exists for one commit. You've got several
> other commits from Simon that get his name right.
>
> What happened?
I don't know what happened, I used git for this, I don't use quilt for
"normal" patches accepted into my trees anymore, only for stable kernel
work.
So either the mail is malformed, or git couldn't figure it out, I've
attached the original message below, and cc:ed the git mailing list.
Also, Simon emailed me after this was committed saying something went
wrong, but I couldn't go back and rebase my tree. Simon, did you ever
figure out if something was odd on your end?
Git developers, any ideas?
thanks,
greg k-h
[-- Attachment #2: messy_email.mbox --]
[-- Type: application/mbox, Size: 6355 bytes --]
^ permalink raw reply
* Re: git email From: parsing (was Re: [GIT PULL] Staging/IIO driver patches for 4.11-rc1)
From: Jeff King @ 2017-02-23 6:17 UTC (permalink / raw)
To: Greg KH
Cc: Linus Torvalds, Simon Sandström, Andrew Morton, git,
Linux Kernel Mailing List, Linux Driver Project
In-Reply-To: <20170223060444.GA26196@kroah.com>
On Thu, Feb 23, 2017 at 07:04:44AM +0100, Greg KH wrote:
> > Poor Simon Sandström.
> >
> > Funnily enough, this only exists for one commit. You've got several
> > other commits from Simon that get his name right.
> >
> > What happened?
>
> I don't know what happened, I used git for this, I don't use quilt for
> "normal" patches accepted into my trees anymore, only for stable kernel
> work.
>
> So either the mail is malformed, or git couldn't figure it out, I've
> attached the original message below, and cc:ed the git mailing list.
>
> Also, Simon emailed me after this was committed saying something went
> wrong, but I couldn't go back and rebase my tree. Simon, did you ever
> figure out if something was odd on your end?
>
> Git developers, any ideas?
The problem isn't on the applying end, but rather on the generating end.
The From header in the attached mbox is:
From: =?us-ascii?B?PT9VVEYtOD9xP1NpbW9uPTIwU2FuZHN0cj1DMz1CNm0/PQ==?= <simon@nikanor.nu>
If you de-base64 that, you get:
=?UTF-8?q?Simon=20Sandstr=C3=B6m?=
So something double-encoded it before it got to your mbox.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox