* Re: [PATCH v2] gpg-interface: use more status letters
From: Junio C Hamano @ 2016-09-28 19:59 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Alex, Ramsay Jones
In-Reply-To: <c4777ef68059034d7ad4697a06bba3cabbdc9265.1475053649.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> - Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
If you are not using /dev/null, I expected you to do
. ./test-lib.sh
GNUPGHOME_saved=$GNPGHOME
. "$TEST_DIRECTORY/lib-gpg.sh"
and then use
GNUPGHOME="$GNUPGHOME_saved" git log -1 ...
in the test.
Otherwise, you are not futureproofing your use and only adding to
maintenance burden. The gnupg-home-not-used hack may turn out to be
a problematic and test-lib.sh may update to point to somewhere else,
which will leave your copy still pointing at the old problematic
place).
> - Do not parse for signer UID in the ERRSIG case (and test that we do not).
Good.
> - Retreat "rather" addition from the doc: good/valid are terms that we use
> differently from gpg anyways.
OK.
> + "X" for a good expired signature, or good signature made by an expired key,
As an attempt to clarify that we cover both EXPSIG and EXPKEYSIG
cases, I think this is good enough. I may have phrased the former
slightly differently, though: "a good signature that has expired".
I have no strong opinion if we want to stress that we cover both
cases, though, which is I think what Ramsay's comment was about.
Thanks.
^ permalink raw reply
* [PATCH v4 2/2] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-28 19:52 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Swift Geek, Jeff King, Kevin Daudt
In-Reply-To: <20160928194939.7706-1-me@ikke.info>
rfc2822 has provisions for quoted strings in structured header fields,
but also allows for escaping these with so-called quoted-pairs.
The only thing git currently does is removing exterior quotes, but
quotes within are left alone.
Remove exterior quotes and remove escape characters so that they don't
show up in the author field.
Signed-off-by: Kevin Daudt <me@ikke.info>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
mailinfo.c | 82 ++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 14 ++++++++
| 5 +++
| 9 +++++
t/t5100/quoted-string.expect | 5 +++
t/t5100/quoted-string.in | 9 +++++
6 files changed, 124 insertions(+)
create mode 100644 t/t5100/comment.expect
create mode 100644 t/t5100/comment.in
create mode 100644 t/t5100/quoted-string.expect
create mode 100644 t/t5100/quoted-string.in
diff --git a/mailinfo.c b/mailinfo.c
index e19abe3..b4118a0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -54,6 +54,86 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)
get_sane_name(&mi->name, &mi->name, &mi->email);
}
+static const char *unquote_comment(struct strbuf *outbuf, const char *in)
+{
+ int c;
+ int take_next_litterally = 0;
+
+ strbuf_addch(outbuf, '(');
+
+ while ((c = *in++) != 0) {
+ if (take_next_litterally == 1) {
+ take_next_litterally = 0;
+ } else {
+ switch (c) {
+ case '\\':
+ take_next_litterally = 1;
+ continue;
+ case '(':
+ in = unquote_comment(outbuf, in);
+ continue;
+ case ')':
+ strbuf_addch(outbuf, ')');
+ return in;
+ }
+ }
+
+ strbuf_addch(outbuf, c);
+ }
+
+ return in;
+}
+
+static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)
+{
+ int c;
+ int take_next_litterally = 0;
+
+ while ((c = *in++) != 0) {
+ if (take_next_litterally == 1) {
+ take_next_litterally = 0;
+ } else {
+ switch (c) {
+ case '\\':
+ take_next_litterally = 1;
+ continue;
+ case '"':
+ return in;
+ }
+ }
+
+ strbuf_addch(outbuf, c);
+ }
+
+ return in;
+}
+
+static void unquote_quoted_pair(struct strbuf *line)
+{
+ struct strbuf outbuf;
+ const char *in = line->buf;
+ int c;
+
+ strbuf_init(&outbuf, line->len);
+
+ while ((c = *in++) != 0) {
+ switch (c) {
+ case '"':
+ in = unquote_quoted_string(&outbuf, in);
+ continue;
+ case '(':
+ in = unquote_comment(&outbuf, in);
+ continue;
+ }
+
+ strbuf_addch(&outbuf, c);
+ }
+
+ strbuf_swap(&outbuf, line);
+ strbuf_release(&outbuf);
+
+}
+
static void handle_from(struct mailinfo *mi, const struct strbuf *from)
{
char *at;
@@ -63,6 +143,8 @@ static void handle_from(struct mailinfo *mi, const struct strbuf *from)
strbuf_init(&f, from->len);
strbuf_addbuf(&f, from);
+ unquote_quoted_pair(&f);
+
at = strchr(f.buf, '@');
if (!at) {
parse_bogus_from(mi, from);
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 56988b7..45d228e 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '
test_cmp expect mboxrd/msg
'
+test_expect_success 'mailinfo handles rfc2822 quoted-string' '
+ mkdir quoted-string &&
+ git mailinfo /dev/null /dev/null <"$DATA/quoted-string.in" \
+ >quoted-string/info &&
+ test_cmp "$DATA/quoted-string.expect" quoted-string/info
+'
+
+test_expect_success 'mailinfo handles rfc2822 comment' '
+ mkdir comment &&
+ git mailinfo /dev/null /dev/null <"$DATA/comment.in" \
+ >comment/info &&
+ test_cmp "$DATA/comment.expect" comment/info
+'
+
test_done
--git a/t/t5100/comment.expect b/t/t5100/comment.expect
new file mode 100644
index 0000000..7228177
--- /dev/null
+++ b/t/t5100/comment.expect
@@ -0,0 +1,5 @@
+Author: A U Thor (this is (really) a comment (honestly))
+Email: somebody@example.com
+Subject: testing comments
+Date: Sun, 25 May 2008 00:38:18 -0700
+
--git a/t/t5100/comment.in b/t/t5100/comment.in
new file mode 100644
index 0000000..c53a192
--- /dev/null
+++ b/t/t5100/comment.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "A U Thor" <somebody@example.com> (this is \(really\) a comment (honestly))
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing comments
+
+
+
+---
+patch
diff --git a/t/t5100/quoted-string.expect b/t/t5100/quoted-string.expect
new file mode 100644
index 0000000..cab1bce
--- /dev/null
+++ b/t/t5100/quoted-string.expect
@@ -0,0 +1,5 @@
+Author: Author "The Author" Name
+Email: somebody@example.com
+Subject: testing quoted-pair
+Date: Sun, 25 May 2008 00:38:18 -0700
+
diff --git a/t/t5100/quoted-string.in b/t/t5100/quoted-string.in
new file mode 100644
index 0000000..e2e627a
--- /dev/null
+++ b/t/t5100/quoted-string.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "Author \"The Author\" Name" <somebody@example.com>
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing quoted-pair
+
+
+
+---
+patch
--
2.10.0.372.g6fe1b14
^ permalink raw reply related
* [PATCH v4 1/2] t5100-mailinfo: replace common path prefix with variable
From: Kevin Daudt @ 2016-09-28 19:52 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Swift Geek, Jeff King, Kevin Daudt
In-Reply-To: <20160928194939.7706-1-me@ikke.info>
Many tests need to store data in a file, and repeat the same pattern to
refer to that path:
"$TEST_DIRECTORY"/t5100/
Create a variable that contains this path, and use that instead.
While we're making this change, make sure the quotes are not just around
the variable, but around the entire string to not give the impression
we want shell splitting to affect the other variables.
Signed-off-by: Kevin Daudt <me@ikke.info>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5100-mailinfo.sh | 68 +++++++++++++++++++++++++++--------------------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..56988b7 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'
. ./test-lib.sh
+DATA="$TEST_DIRECTORY/t5100"
+
test_expect_success 'split sample box' \
- 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
+ 'git mailsplit -o. "$DATA/sample.mbox" >last &&
last=$(cat last) &&
echo total is $last &&
test $(cat last) = 17'
@@ -16,28 +18,28 @@ test_expect_success 'split sample box' \
check_mailinfo () {
mail=$1 opt=$2
mo="$mail$opt"
- git mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/msg$mo msg$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/patch$mo patch$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/info$mo info$mo
+ git mailinfo -u $opt "msg$mo" "patch$mo" <"$mail" >"info$mo" &&
+ test_cmp "$DATA/msg$mo" "msg$mo" &&
+ test_cmp "$DATA/patch$mo" "patch$mo" &&
+ test_cmp "$DATA/info$mo" "info$mo"
}
for mail in 00*
do
test_expect_success "mailinfo $mail" '
- check_mailinfo $mail "" &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
+ check_mailinfo "$mail" "" &&
+ if test -f "$DATA/msg$mail--scissors"
then
- check_mailinfo $mail --scissors
+ check_mailinfo "$mail" --scissors
fi &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers
+ if test -f "$DATA/msg$mail--no-inbody-headers"
then
- check_mailinfo $mail --no-inbody-headers
+ check_mailinfo "$mail" --no-inbody-headers
fi &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--message-id
+ if test -f "$DATA/msg$mail--message-id"
then
- check_mailinfo $mail --message-id
+ check_mailinfo "$mail" --message-id
fi
'
done
@@ -45,7 +47,7 @@ done
test_expect_success 'split box with rfc2047 samples' \
'mkdir rfc2047 &&
- git mailsplit -orfc2047 "$TEST_DIRECTORY"/t5100/rfc2047-samples.mbox \
+ git mailsplit -orfc2047 "$DATA/rfc2047-samples.mbox" \
>rfc2047/last &&
last=$(cat rfc2047/last) &&
echo total is $last &&
@@ -54,20 +56,20 @@ test_expect_success 'split box with rfc2047 samples' \
for mail in rfc2047/00*
do
test_expect_success "mailinfo $mail" '
- git mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&
+ git mailinfo -u "$mail-msg" "$mail-patch" <"$mail" >"$mail-info" &&
echo msg &&
- test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-msg &&
+ test_cmp "$DATA/empty" "$mail-msg" &&
echo patch &&
- test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-patch &&
+ test_cmp "$DATA/empty" "$mail-patch" &&
echo info &&
- test_cmp "$TEST_DIRECTORY"/t5100/rfc2047-info-$(basename $mail) $mail-info
+ test_cmp "$DATA/rfc2047-info-$(basename $mail)" "$mail-info"
'
done
test_expect_success 'respect NULs' '
- git mailsplit -d3 -o. "$TEST_DIRECTORY"/t5100/nul-plain &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-plain 001 &&
+ git mailsplit -d3 -o. "$DATA/nul-plain" &&
+ test_cmp "$DATA/nul-plain" 001 &&
(cat 001 | git mailinfo msg patch) &&
test_line_count = 4 patch
@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '
test_expect_success 'Preserve NULs out of MIME encoded message' '
- git mailsplit -d5 -o. "$TEST_DIRECTORY"/t5100/nul-b64.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.in 00001 &&
+ git mailsplit -d5 -o. "$DATA/nul-b64.in" &&
+ test_cmp "$DATA/nul-b64.in" 00001 &&
git mailinfo msg patch <00001 &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.expect patch
+ test_cmp "$DATA/nul-b64.expect" patch
'
test_expect_success 'mailinfo on from header without name works' '
mkdir info-from &&
- git mailsplit -oinfo-from "$TEST_DIRECTORY"/t5100/info-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/info-from.in info-from/0001 &&
+ git mailsplit -oinfo-from "$DATA/info-from.in" &&
+ test_cmp "$DATA/info-from.in" info-from/0001 &&
git mailinfo info-from/msg info-from/patch \
<info-from/0001 >info-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/info-from.expect info-from/out
+ test_cmp "$DATA/info-from.expect" info-from/out
'
test_expect_success 'mailinfo finds headers after embedded From line' '
mkdir embed-from &&
- git mailsplit -oembed-from "$TEST_DIRECTORY"/t5100/embed-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/embed-from.in embed-from/0001 &&
+ git mailsplit -oembed-from "$DATA/embed-from.in" &&
+ test_cmp "$DATA/embed-from.in" embed-from/0001 &&
git mailinfo embed-from/msg embed-from/patch \
<embed-from/0001 >embed-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/embed-from.expect embed-from/out
+ test_cmp "$DATA/embed-from.expect" embed-from/out
'
test_expect_success 'mailinfo on message with quoted >From' '
mkdir quoted-from &&
- git mailsplit -oquoted-from "$TEST_DIRECTORY"/t5100/quoted-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.in quoted-from/0001 &&
+ git mailsplit -oquoted-from "$DATA/quoted-from.in" &&
+ test_cmp "$DATA/quoted-from.in" quoted-from/0001 &&
git mailinfo quoted-from/msg quoted-from/patch \
<quoted-from/0001 >quoted-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.expect quoted-from/msg
+ test_cmp "$DATA/quoted-from.expect" quoted-from/msg
'
test_expect_success 'mailinfo unescapes with --mboxrd' '
mkdir mboxrd &&
git mailsplit -omboxrd --mboxrd \
- "$TEST_DIRECTORY"/t5100/sample.mboxrd >last &&
+ "$DATA/sample.mboxrd" >last &&
test x"$(cat last)" = x2 &&
for i in 0001 0002
do
git mailinfo mboxrd/msg mboxrd/patch \
<mboxrd/$i >mboxrd/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/${i}mboxrd mboxrd/msg
+ test_cmp "$DATA/${i}mboxrd" mboxrd/msg
done &&
sp=" " &&
echo "From " >expect &&
--
2.10.0.372.g6fe1b14
^ permalink raw reply related
* [PATCH v4 0/2] Handle RFC2822 quoted-pairs in From header
From: Kevin Daudt @ 2016-09-28 19:49 UTC (permalink / raw)
To: git; +Cc: Kevin Daudt, Junio C Hamano, Swift Geek, Jeff King
In-Reply-To: <20160925210808.26424-1-me@ikke.info>
Changes since v3:
- t5100-mailinfo: Reverted back to capital $DATA
- t5100-mailinfo: Moved quotes to around the entire string, instead of the
variable, as per Junio's suggestion
Kevin Daudt (2):
t5100-mailinfo: replace common path prefix with variable
mailinfo: unescape quoted-pair in header fields
mailinfo.c | 82 ++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 82 ++++++++++++++++++++++++++------------------
t/t5100/comment.expect | 5 +++
t/t5100/comment.in | 9 +++++
t/t5100/quoted-string.expect | 5 +++
t/t5100/quoted-string.in | 9 +++++
6 files changed, 159 insertions(+), 33 deletions(-)
create mode 100644 t/t5100/comment.expect
create mode 100644 t/t5100/comment.in
create mode 100644 t/t5100/quoted-string.expect
create mode 100644 t/t5100/quoted-string.in
--
2.10.0.372.g6fe1b14
^ permalink raw reply
* Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Junio C Hamano @ 2016-09-28 19:28 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20160928114348.1470-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> The original commit d95d728aba06a34394d15466045cbdabdada58a2 was
> reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we
> were (and still are) not ready for a new world order. A lot more
> investigation must be done to see what is impacted. See the 78cc1a5 for
> details.
>
> This patch takes a smaller and safer step. The new behavior is
> controlled by shift_ita flag. We can gradually move more diff users to
> the new behavior after we are sure it's safe to do so. This flag is
> exposed to outside temporarily as "--shift-ita" for people who prefer
> "git diff [--cached] --stat" to "git status"
Let's stop advertising this as a resurrection of something else.
The original that was unconditional was simply broken.
It is very good to refer to it (and its reversion), when justifying
why this version takes the particular approach to introduce a new
optional behaviour that can be toggled on selectively, by explaining
why doing this unconditionally was a broken idea that needed to be
reverted later.
But you would need to explain what problem this patch attempts to
solve and how before even going into that. The above two paragraphs
are backwards.
As I already said, --shift-ita is not quite descriptive and I think
it should be renamed to something else, but I kept that in the
following attempt to rewrite:
Subject: diff-lib: allow ita entries treated as "not yet exist in index"
When comparing the index and the working tree to show which
paths are new, and comparing the tree recorded in the HEAD and
the index to see if committing the contents recorded in the
index would result in an empty commit, we would want the former
comparison to say "these are new paths" and the latter to say
"there is no change" for paths that are marked as intent-to-add.
We made a similar attempt at d95d728a ("diff-lib.c: adjust
position of i-t-a entries in diff", 2015-03-16), which redefined
the semantics of these two comparison modes globally, which was
a disastor and had to be reverted at 78cc1a54 ("Revert
"diff-lib.c: adjust position of i-t-a entries in diff"",
2015-06-23). To make sure we do not repeat the same mistake,
introduce a new internal diffopt option so that this different
semantics can be asked for only by callers that ask it, while
making sure other unaudited callers will get the same comparison
result. This internal option is also exposed temporarily as
"--shift-ita" to help experiment.
After reading the three patches through, however, I do not think we
use the command line option anywhere. I'm inclined to say that we
shouldn't add it at all. Or at least do so in a separate follow-up
patch "now we have an internal mechanism, let's expose it anyway" at
the end. Which means that the last sentence in my attempted rewrite
should go.
The patch to diff-lib.c machinery looks good.
Thanks.
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160928171610.pbghg4sk23vm4xnp@sigill.intra.peff.net>
[-- Attachment #1.1: Type: text/plain, Size: 1821 bytes --]
On 28.9.2016 19:16, Jeff King wrote:
> On Wed, Sep 28, 2016 at 06:05:52PM +0200, Petr Stodulka wrote:
>
>> Delegation of credentials is disabled by default in libcurl since
>> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
>> makes troubles with GSS/kerberos authentication where delegation
>> of credentials is required. This can be changed with option
>> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
>> since libcurl version 7.22.0.
>
> I don't have any real knowledge of GSSAPI, so I'll refrain from
> commenting on that aspect. But I did notice one mechanical issue:
>
Me neither. I have just basic knowledge and I am not able to configure
virtual machine, which really need set delegation in libcurl (I need
just negotiation, which is in git possible, I guess since v2.8.0).
However, I discuss it with libcurl maintainer and he confirm that this
option can be required in some cases and this is what I need to do.
this already. I tested just setting of parameter in libcurl according
to description and nothing else seems broken. So anyone else who will
be able to test complete behaviour, where delegation is needed, is welcomed.
[snip]
> We only declare the curl_deleg variable if we have a new-enough curl.
> But...
>
>> @@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb)
>> return 0;
>> }
>>
>> + if (!strcmp("http.delegation", var)) {
>> + return git_config_string(&curl_deleg, var, value);
>> + }
>> +
>
> ...here we try to use it regardless. I think you want another #ifdef,
> and probably to warn the user in the #else block (similar to what the
> http.pinnedpubkey code does).
>
> -Peff
>
You are right. Thanks. I sent new version of patch with fix.
Petr
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-28 18:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20160928172417.GA61176@google.com>
Brandon Williams <bmwill@google.com> writes:
>> I actually think it would make more sense to add
>>
>> lf_to_nul () {
>> perl -pe 'y/\012/\000/'
>> }
>>
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> Turns out this function already exists in test-lib-functions.sh
;-)
^ permalink raw reply
* Re: [PATCH 2/3] diff-lib.c: enable --shift-ita in index_differs_from()
From: Junio C Hamano @ 2016-09-28 18:49 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20160928114348.1470-3-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This function is basically "git diff --cached HEAD", It has three
> callers:
>
> - One in builtin/commit.c, which uses it to determine if the index is
> different from HEAD and go ahead making a new commit.
>
> - Two in sequencer.c, which use it to see if the index is dirty.
>
> In the first case, if ita entries are present, index_differs_from() may
> report "dirty". However at tree creation phase, ita entries are dropped
> and the result tree may look exactly the same as HEAD (assuming that
> nothing else is changed in index). This is what we need index_differs_from()
> for, to catch new empty commits. Enabling shift_ita in index_differs_from()
> fixes this.
>
> In the second case, the presence of ita entries are enough to say the
> index is dirty and not continue on. Make an explicit check for that
> before comparing index against HEAD (whether --shift-ita is present is
> irrelevant)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
There are three callers to index_differs_from(), which asks "is the
index different from the HEAD". Because you want to change the
behaviour of the function for one of these callers while not
exposing its undesirable behaviour for the other two callers, you
guard the call to it with another call to a new helper function,
which needs to scan the entire index one more time.
It somehow sounds like backwards to me.
IOW, I wonder if it makes more sense to add a new interface to tell
the index_differs_from() function "I want you to use shift-ita
semantics" bit, and pass that when calling it from builtin/commit.c
while not toggling that bit on when the other two callers call it,
without introducing the has_ita_entries() helper function.
By the way, I think "shift" is a bit unclear name for the diffopt
field. The log message of [1/3] is totally unclear (it claims
"smaller and safer" without explaining what it exactly does and why
that is safer); the documentation update in it is slightly better in
that it lets intelligent readers to guess that the option is to
declare that ita entries do not yet exist in the index (hence, "git
diff" would say "that's a new file", while "git diff --cached" says
nothing about it). From that observation, I think a descriptive
phrase that is suitable for its name than "shift" needs to be found
in a short explanation of what it does: "treat ita as missing in the
index", e.g. "rev.diffopt.ita_is_missing = 1", perhaps?
Other than these small implementation details, I think I like the
direction these two patches are taking us (I haven't checked 3/3
yet).
Thanks.
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Junio C Hamano @ 2016-09-28 18:22 UTC (permalink / raw)
To: Eric Wong; +Cc: Kevin Wern, git
In-Reply-To: <xmqqy42cj5g1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
> What "git clone" should have been was:
>
> * Parse command line arguments;
>
> * Create a new repository and go into it; this step would
> require us to have parsed the command line for --template,
> <directory>, --separate-git-dir, etc.
>
> * Talk to the remote and do get_remote_heads() aka ls-remote
> output;
>
> * Decide what fetch refspec to use, which alternate object store
> to borrow from; this step would require us to have parsed the
> command line for --reference, --mirror, --origin, etc;
>
> --- we'll insert something new here ---
>
> * Issue "git fetch" with the refspec determined above; this step
> would require us to have parsed the command line for --depth, etc.
>
> * Run "git checkout -b" to create an initial checkout; this step
> would require us to have parsed the command line for --branch,
> etc.
>
> Even though the current code conceptually does the above, these
> steps are not cleanly separated as such. I think our update to gain
> "resumable clone" feature on the client side need to start by
> refactoring the current code, before learning "resumable clone", to
> look like the above.
>
> Once we do that, we can insert an extra step before the step that
> runs "git fetch" to optionally [*1*] grab the extra piece of
> information Kevin's "prime-clone" service produces [*2*], and store
> it in the "new repository" somewhere [*3*].
>
> And then, as you suggested, an updated "git fetch" can be taught to
> notice the priming information left by the previous step, and use it
> to attempt to download the pack until success, and to index that
> pack to learn the tips that can be used as ".have" entries in the
> request. From the original server's point of view, this fetch
> request would "want" the same set of objects, but would appear as
> an incremental update.
Thinking about this even more, it probably makes even more sense to
move the new "learn prime info and store it in repository somewhere,
so that later re-invocation of 'git fetch' can take advantage of it"
step _into_ "git fetch". That would allow "git fetch" in a freshly
created empty repository take advantage of this feature for free.
The step that "git clone" internally drives "git fetch" would not
actually be done by spawning a separate process with run_command()
because we would want to reuse the connection we already have with
the server when "git clone" first talked to it to learn "ls-remote"
equivalent (i.e. transport_get_remote_refs()). I wonder if we can
do without this early "ls-remote"; that would further simplify
things by allowing us to just spawn "git fetch" internally.
^ permalink raw reply
* Re: [PATCH 10/11] run command: add RUN_COMMAND_NO_STDOUT
From: Kevin Wern @ 2016-09-28 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Wern, git
In-Reply-To: <xmqqponnkiz7.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 28, 2016 at 10:54:52AM -0700, Junio C Hamano wrote:
>
> I just got an impression that you were apologetic for having to add
> this option that is otherwise useless and tried to suggest a simpler
> solution that does not involve such an addition.
Sorry, to be clear, I meant I was ok with your suggestion. That's what I meant
by 'this change.'
^ permalink raw reply
* Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
From: Junio C Hamano @ 2016-09-28 18:02 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, David Turner, git, David Turner
In-Reply-To: <20160928085841.aoisson3fnuke47q@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> if (!dont_change_ref) {
> struct ref_transaction *transaction;
> - struct strbuf err = STRBUF_INIT;
> -
> - transaction = ref_transaction_begin(&err);
> - if (!transaction ||
> - ref_transaction_update(transaction, ref.buf,
> - sha1, forcing ? NULL : null_sha1,
> - 0, msg, &err) ||
> - ref_transaction_commit(transaction, &err))
> - die("%s", err.buf);
> +
> + transaction = ref_transaction_begin(&error_die);
> + ref_transaction_update(transaction, ref.buf,
> + sha1, forcing ? NULL : null_sha1,
> + 0, msg, &error_die);
> + ref_transaction_commit(transaction, &error_die);
> ref_transaction_free(transaction);
> - strbuf_release(&err);
> }
>
> if (real_ref && track)
>
> which is much shorter and to the point (it does rely on the called
> functions always calling report_error() and never just returning NULL or
> "-1", but that should be the already. If it isn't, we'd be printing
> "fatal: " with no message).
Yes but... grepping for die() got a lot harder, which may not be a
good thing.
I do like the flexibility such a mechanism offers, but
wrapping/hiding die in it is probably an example that the
flexibility went a bit too far.
^ permalink raw reply
* [PATCH v2] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 18:01 UTC (permalink / raw)
To: git; +Cc: pstodulk
In-Reply-To: <20160928171610.pbghg4sk23vm4xnp@sigill.intra.peff.net>
Delegation of credentials is disabled by default in libcurl since
version 7.21.7 due to security vulnerability CVE-2011-2192. Which
makes troubles with GSS/kerberos authentication when delegation
of credentials is required. This can be changed with option
CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
since libcurl version 7.22.0.
This patch provides new configuration variable http.delegation
which corresponds to curl parameter "--delegation" (see man 1 curl).
The following values are supported:
* none (default).
* policy
* always
Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
---
Documentation/config.txt | 14 ++++++++++++++
http.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e78293b..a179474 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1736,6 +1736,20 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
+http.delegation::
+ Control GSSAPI credential delegation. The delegation is disabled
+ by default in libcurl since version 7.21.7. Set parameter to tell
+ the server what it is allowed to delegate when it comes to user
+ credentials. Used with GSS/kerberos. Possible values are:
++
+--
+* `none` - Don't allow any delegation.
+* `policy` - Delegates if and only if the OK-AS-DELEGATE flag is set in the
+ Kerberos service ticket, which is a matter of realm policy.
+* `always` - Unconditionally allow the server to delegate.
+--
+
+
http.extraHeader::
Pass an additional HTTP header when communicating with a server. If
more than one such entry exists, all of them are added as extra
diff --git a/http.c b/http.c
index 82ed542..0c65639 100644
--- a/http.c
+++ b/http.c
@@ -90,6 +90,18 @@ static struct {
* here, too
*/
};
+#if LIBCURL_VERSION_NUM >= 0x071600
+static const char *curl_deleg;
+static struct {
+ const char *name;
+ long curl_deleg_param;
+} curl_deleg_levels[] = {
+ { "none", CURLGSSAPI_DELEGATION_NONE },
+ { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG },
+ { "always", CURLGSSAPI_DELEGATION_FLAG },
+};
+#endif
+
static struct credential proxy_auth = CREDENTIAL_INIT;
static const char *curl_proxyuserpwd;
static const char *curl_cookie_file;
@@ -323,6 +335,15 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp("http.delegation", var)) {
+#if LIBCURL_VERSION_NUM >= 0x071600
+ return git_config_string(&curl_deleg, var, value);
+#else
+ warning(_("Delegation control is not supported with cURL < 7.22.0"));
+ return 0;
+#endif
+ }
+
if (!strcmp("http.pinnedpubkey", var)) {
#if LIBCURL_VERSION_NUM >= 0x072c00
return git_config_pathname(&ssl_pinnedkey, var, value);
@@ -629,6 +650,22 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
#endif
+#if LIBCURL_VERSION_NUM >= 0x071600
+ if (curl_deleg) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) {
+ if (!strcmp(curl_deleg, curl_deleg_levels[i].name)) {
+ curl_easy_setopt(result, CURLOPT_GSSAPI_DELEGATION,
+ curl_deleg_levels[i].curl_deleg_param);
+ break;
+ }
+ }
+ if (i == ARRAY_SIZE(curl_deleg_levels))
+ warning("Unknown delegation method '%s': using default",
+ curl_deleg);
+ }
+#endif
+
if (http_proactive_auth)
init_curl_http_auth(result);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH 3/3] docs/cvs-migration: mention cvsimport caveats
From: Junio C Hamano @ 2016-09-28 17:59 UTC (permalink / raw)
To: Eric S. Raymond; +Cc: Jeff King, git
In-Reply-To: <20160928001108.GA9120@thyrsus.com>
"Eric S. Raymond" <esr@thyrsus.com> writes:
> Jeff King <peff@peff.net>:
>> I am not qualified to write on the current state of
>> the art in CVS importing.
>
> I *am* qualified; cvs-fast-export has had a lot of work put into it by
> myself and others over the last five years. Nobody else is really
> working this problem anymore, not much else than cvs2git is even left
> standing at this point.
It sounds like you, as a better qualified person, would be in the
best position to send an update to the documentation to tell people
not to use older and unmaintained ones and guides them instead to a
newer and better tool.
... ah, I notice that peff said the same already.
I'd be fine with reviewing and applying such a patch.
Thanks.
^ permalink raw reply
* Re: [PATCH 10/11] run command: add RUN_COMMAND_NO_STDOUT
From: Junio C Hamano @ 2016-09-28 17:54 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <20160928044622.GE3762@kwern-HP-Pavilion-dv5-Notebook-PC>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> On Fri, Sep 16, 2016 at 04:07:00PM -0700, Junio C Hamano wrote:
>> Kevin Wern <kevin.m.wern@gmail.com> writes:
>>
>> > Add option RUN_COMMAND_NO_STDOUT, which sets no_stdout on a child
>> > process.
>> >
>> > This will be used by git clone when calling index-pack on a downloaded
>> > packfile.
>>
>> If it is just one caller, would't it make more sense for that caller
>> set no_stdout explicitly itself?
>
> I based the calling code in do_index_pack on dissociate_from_references, which
> uses run_command_v_opt, so it never occured to me to do that. I thought it was
> just good, uniform style and encapsulation. Like how transport's methods and
> internals aren't really intended to be changed or accessed--unless it's through
> the APIs we create.
>
> However, I don't feel very strongly about this, so I'm okay with this change.
I am neutral and with no opinion. I may have offered a solution to
a problem that did not exist.
I just got an impression that you were apologetic for having to add
this option that is otherwise useless and tried to suggest a simpler
solution that does not involve such an addition.
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Junio C Hamano @ 2016-09-28 17:52 UTC (permalink / raw)
To: Ben Peart; +Cc: git, Ben Peart, pclouds, Jeff Hostetler, philipoakley
In-Reply-To: <004d01d219aa$0a941fa0$1fbc5ee0$@gmail.com>
"Ben Peart" <peartben@gmail.com> writes:
> The fact that "git checkout -b NEW" updates the index and as a
> result reflects any changes in the sparse-checkout and the issue
> Junio pointed out earlier about not calling show_local_changes
> at the end of merge_working_tree are the only difference in behavior
> I am aware of. Both of these are easily rectified.
>
> That said, given we are skipping huge amounts of work by no longer
> merging the commit trees, generating a new index, and merging the
> local modifications in the working tree, it is possible that there are
> other behavior changes I'm just not aware of.
That is OK. It is not ok to leave such bugs at the end of the
development before the topic is merged to 'master' to be delivered
to the end users, but you do not have to fight alone to produce a
perfect piece of code with your first attempt. That's what the
reviews and testing period are for.
If you are shooting for the same behaviour, then that is much better
than "make 'checkout -b NEW' be equivalent to a sequence of
update-ref && symbolic-ref, which is different from others", which
was the second explanation you gave earlier. I am much happier with
that goal.
But if that is the case, I really do not see any point of singling
out "-b NEW" case. The following property MUST be kept:
(1) "git checkout -b NEW", "git checkout", "git checkout HEAD^0"
and "git checkout HEAD" (no other parameters to any of them)
ought to give identical index and working tree. It is too
confusing to leave subtly different results that will lead to
hard to diagnose bugs for only one of them.
That would make the "do we skip unpack_trees() call?" decision a lot
simpler to make, I would suspect. We only need to see "are the two
trees we would fed unpack_trees() the same as HEAD's tree?" and do
not have to look at new_branch and other irrelevant things at all.
What happens in the ref namespace is immaterial, as making or
skipping an unpack_trees() call would not affect anything other than
the resulting index and the working tree. If we want to keep that
sparse-checkout wart, we would also need to see if the control file
sparse-checkout keeps in $GIT_DIR/ exists, but the result will be
much simpler set of rules, and would hopefully help remove the "the
optimization kicks in following logic that is an unreviewable-mess"
issue.
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Junio C Hamano @ 2016-09-28 17:32 UTC (permalink / raw)
To: Eric Wong; +Cc: Kevin Wern, git
In-Reply-To: <xmqqshslkndk.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>>> git clone --resume <resumable_work_or_git_dir>
>>
>> I think calling "git fetch" should resume, actually.
>> It would reduce the learning curve and seems natural to me:
>> "fetch" is jabout grabbing whatever else appeared since the
>> last clone/fetch happened.
>
> I hate say this but it sounds to me like a terrible idea. At that
> point when you need to resume, there is not even ref for "fetch" to
> base its incremental work off of. It is better to keep the knowledge
> of this "priming" dance inside "clone". Hopefully the original "clone"
> whose connection was disconnected in the middle would automatically
> attempt resuming and "clone --resume" would not be as often as needed.
After sleeping on this, I want to take the above back.
I think teaching "git fetch" about the "resume" part makes tons of
sense.
What "git clone" should have been was:
* Parse command line arguments;
* Create a new repository and go into it; this step would
require us to have parsed the command line for --template,
<directory>, --separate-git-dir, etc.
* Talk to the remote and do get_remote_heads() aka ls-remote
output;
* Decide what fetch refspec to use, which alternate object store
to borrow from; this step would require us to have parsed the
command line for --reference, --mirror, --origin, etc;
--- we'll insert something new here ---
* Issue "git fetch" with the refspec determined above; this step
would require us to have parsed the command line for --depth, etc.
* Run "git checkout -b" to create an initial checkout; this step
would require us to have parsed the command line for --branch,
etc.
Even though the current code conceptually does the above, these
steps are not cleanly separated as such. I think our update to gain
"resumable clone" feature on the client side need to start by
refactoring the current code, before learning "resumable clone", to
look like the above.
Once we do that, we can insert an extra step before the step that
runs "git fetch" to optionally [*1*] grab the extra piece of
information Kevin's "prime-clone" service produces [*2*], and store
it in the "new repository" somewhere [*3*].
And then, as you suggested, an updated "git fetch" can be taught to
notice the priming information left by the previous step, and use it
to attempt to download the pack until success, and to index that
pack to learn the tips that can be used as ".have" entries in the
request. From the original server's point of view, this fetch
request would "want" the same set of objects, but would appear as
an incremental update.
Of course, the final step that happens in "git clone", i.e. the
initial checkout, needs to be done somehow, if your user decides to
resume with "git fetch", as "git fetch" _never_ touches the working
tree. So for that purpose, the primary end-user facing interface
may still have to be "git clone --resume <dir>". That would
probably skip all four steps in the above sequence, the new
"download priming information" step and go directly to the step that
runs "git fetch".
I do agree that is a much better design, and the crucial design
decision that makes it a better design is your making "git fetch"
aware of this "ah, we have the instruction left in this repository
how to prime its object store" information.
Thanks.
[Footnotes]
*1* It is debatable if it would be an overall win to use the "first
prime by grabbing a large packfile" clone if we are doing
shallow or single-branch clone, hence "optionally". It is
important to notice that we already have enough information to
base the decision at this point in the above sequence.
*2* As I said, I do not think it needs to be a separate new service,
and I suspect it may be a better design to carry it over the
protocol extension. At this point in the above sequence, we
have done an equivalent of ls-remote and if we designed a
protocol extension to carry the information we should already
have it. If we use a separate new service, we can of course
make a separate connection to ask about "prime-clone"
information. The way this piece of information is transmitted
is of secondary importance.
*3* In addition to the "prime-clone" information, we may need to
store some information that is only known to "clone" (perhaps
because it was given from the command line) to help the final
"checkout -b" step to know what to checkout around here, in case
the next "fetch" step is interrupted and killed.
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-28 17:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwphxm7av.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it. I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> > ... digs and finds e85fe4d8 ("more tr portability test script
> > fixes", 2008-03-12)
> >
> > So use something like
> >
> > perl -pe 'y/\012/\000/' <<\-EOF
> > ...
> > EOF
> >
> > instead, perhaps?
>
> I actually think it would make more sense to add
>
> lf_to_nul () {
> perl -pe 'y/\012/\000/'
> }
>
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.
Turns out this function already exists in test-lib-functions.sh
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Jeff King @ 2016-09-28 17:16 UTC (permalink / raw)
To: Petr Stodulka; +Cc: git
In-Reply-To: <1475078752-31195-1-git-send-email-pstodulk@redhat.com>
On Wed, Sep 28, 2016 at 06:05:52PM +0200, Petr Stodulka wrote:
> Delegation of credentials is disabled by default in libcurl since
> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
> makes troubles with GSS/kerberos authentication where delegation
> of credentials is required. This can be changed with option
> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
> since libcurl version 7.22.0.
I don't have any real knowledge of GSSAPI, so I'll refrain from
commenting on that aspect. But I did notice one mechanical issue:
> +#if LIBCURL_VERSION_NUM >= 0x071600
> +static const char *curl_deleg;
> +static struct {
> + const char *name;
> + long curl_deleg_param;
> +} curl_deleg_levels[] = {
> + { "none", CURLGSSAPI_DELEGATION_NONE },
> + { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG },
> + { "always", CURLGSSAPI_DELEGATION_FLAG },
> +};
> +#endif
We only declare the curl_deleg variable if we have a new-enough curl.
But...
> @@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb)
> return 0;
> }
>
> + if (!strcmp("http.delegation", var)) {
> + return git_config_string(&curl_deleg, var, value);
> + }
> +
...here we try to use it regardless. I think you want another #ifdef,
and probably to warn the user in the #else block (similar to what the
http.pinnedpubkey code does).
-Peff
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 17:03 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <1475078752-31195-1-git-send-email-pstodulk@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 537 bytes --]
On 28.9.2016 18:05, Petr Stodulka wrote:
> Delegation of credentials is disabled by default in libcurl since
> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
> makes troubles with GSS/kerberos authentication where delegation
> of credentials is required. This can be changed with option
> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
> since libcurl version 7.22.0.
Correction:
Which makes troubles with GSS/kerberos authentication when delegation
of credentials is required.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-28 17:02 UTC (permalink / raw)
To: git
Cc: Ben Peart, pclouds, Jeff Hostetler, philipoakley,
'Junio C Hamano'
Resending
> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Philip Oakley
> Sent: Saturday, September 24, 2016 3:31 PM
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Ben Peart <Ben.Peart@microsoft.com>; pclouds@gmail.com;
> git@vger.kernel.org
> Subject: Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial
> checkout
>
> Hi Junio,
>
> From: "Junio C Hamano" <gitster@pobox.com>
> > "Philip Oakley" <philipoakley@iee.org> writes:
> >
> >>> > >"git checkout -b foo" (without -f -m or <start_point>) is defined
> >>> > >in the manual as being a shortcut for/equivalent to:
> >>> > >
> >>> > > (1a) "git branch foo"
> >>> > > (1b) "git checkout foo"
> >>> > >
> >>> > >However, it has been our experience in our observed use cases and
> >>> > >all the existing git tests, that it can be treated as equivalent
to:
> >>> > >
> >>> > > (2a) "git branch foo"
> >>> > > (2b) "git symbolic-ref HEAD refs/heads/foo"
> >>> > >...
> >>> > >
> >>> > I am still not sure if I like the change of what "checkout -b" is
> >>> > this late in the game, though.
> >>>
> >>> ...
> >>> That said, you're much more on the frontline of receiving negative
> >>> feedback about doing that than I am. :) How would you like to
> >>> proceed?
> >>
> >> I didn't see an initial confirmation as to what the issue really was.
> >> You indicated the symptom ('a long checkout time'), but then we
> >> missed out on hard facts and example repos, so that the issue was
> >> replicable.
> >
> > I took it as a given, trivial and obvious optimization opportunity,
> > that it is wasteful having to traverse two trees to consolidate and
> > reflect their differences into the working tree when we know upfront
> > that these two trees are identical, no matter what the overhead for
> > doing so is.
>
> I agree, and I believe Ben agrees.
>
Correct. In my original patch request I put more specific information on
the impact this optimization has in our specific case (reducing the cost
from 166 seconds to 16 seconds).
> >
> >> At the moment there is the simple workaround of an alias that
> >> executes that two step command dance to achieve what you needed, and
> >> Junio has outlined the issues he needed to be covered from his
> >> maintainer perspective (e.g. the detection of sparse checkouts).
> >> Confirming the root causes would help in setting a baseline.
> >>
> >> I hope that is of help - I'd seen that the discussion had gone quiet.
> >
> > Some of the problems I have are:
> >
> > (1) "git checkout -b NEW", "git checkout", "git checkout HEAD^0"
> > and "git checkout HEAD" (no other parameters to any of them)
> > ought to give identical index and working tree. It is too
> > confusing to leave subtly different results that will lead to
> > hard to diagnose bugs for only one of them.
> >
> > (2) The proposed log message talks only about "performance
> > optimization",
>
> > while the purpose of the change is more
> > about
> > changing the definition
>
> Here I think is the misunderstanding. His purpose is NOT to change the
> definition (IIUC). As I read the message you reference below (and Ben's
other
> messages), I understood that he was trying to achieve what you said (i.e.
> optimise the trivial and obvious opportunity) of selecting for the common
> case (underlying conditions) where the two command sequences are
> identical. If the selected case / conditions is not identical then it is
defined
> wrongly...
>
> I suspect that it was Ben's 'soft' explanation that allowed the discussion
to
> diverge.
>
I'm unaccustomed to doing reviews like this via email so have been
struggling with how to most effectively communicate about the proposed
change. I appreciate any help and understanding as I go through this
for the first time.
My intention was not to change the users expected results which
I believe are to "create a new branch and switch to it." We reinforce
that expectation with the output of the command which completes
with the text "Switched to a new branch 'foo'"
>
> > of what "git checkout -b
> > NEW" is from
> > "git branch NEW && git checkout NEW" to "git branch NEW && git
> > symbolic-ref HEAD refs/heads/NEW". The explanation in a Ben's
> > later message <007401d21278$445eba80$cd1c2f80$@gmail.com> does
> > a much better job contrasting the two.
> >
> > (3) I identified only one difference as an example sufficient to
> > point out why the patch provided is not a pure optimization but
> > behaviour change. Fixing that example alone to avoid change in
> > the behaviour is trivial (see if the "info/sparse-checkout"
> > file is present and refrain from skipping the proper checkout),
>
> This is probably the point Ben needs to take on board to narrow the
> conditions down. There may be others.
>
The fact that "git checkout -b NEW" updates the index and as a
result reflects any changes in the sparse-checkout and the issue
Junio pointed out earlier about not calling show_local_changes
at the end of merge_working_tree are the only difference in behavior
I am aware of. Both of these are easily rectified.
That said, given we are skipping huge amounts of work by no longer
merging the commit trees, generating a new index, and merging the
local modifications in the working tree, it is possible that there are
other behavior changes I'm just not aware of.
> > but a much larger problem is that I do not know (and Ben does
> > not, I suspect) know what other behaviour changes the patch is
> > introducing, and worse, the checks are sufficiently dense too
> > detailed and intimate to the implementation of unpack_trees()
> > that it is impossible for anybody to make sure the exceptions
> > defined in this patch and updates to other parts of the system
> > will be kept in sync.
>
> I did not believe he was proposing such a change to behaviour, hence his
> difficulty in responding (or at least that is my perception). I.e. he was
> digging a hole in the wrong place.
>
> It is possible that he had accidentally introduced a behavious change, and
> having failed to explictly say "This patch (should) produces no behavious
> change", which then continued to re-inforce the misunderstanding.
>
> >
> > So my inclination at this point, unless we see somebody invents a
> > clever way to solve (3), is that any change that violates (1),
> > i.e. as long as the patch does "Are we doing '-b NEW'? Then we do
> > something subtly different", is not acceptable, and solving (3) in a
> > maintainable way smells like quite a hard thing to do. But it would
> > be ideal if (3) is solved cleanly, as we will then not have to worry
> > about changing behaviour at all and can apply the optimization for
> > all of the four cases equally. As a side effect, that approach
> > would solve problem (2) above.
> >
> > If we were to punt on keeping the sanity (1) and introduce a subtly
> > different "create a new branch and point the HEAD at it", an easier
> > way out may be be one of
> >
> > 1. a totally new command, e.g. "git branch-switch NEW" that takes
> > only a single argument and no other "checkout" options, or
> >
> > 2. a new option to "git checkout" that takes _ONLY_ a single
> > argument and incompatible with any other option or command line
> > argument, or
> >
> > 3. an alias that does "git branch" followed by "git symbolic-ref".
> >
> > Neither of the first two sounds palatable, though.
>
> It will need Ben to come back and clarify, if he did, or did not, want any
> behaviour change (beyond speed of action;-)
>
There is a subtlety here in what is meant by "any behavior change."
I did not want to change the users expectations of what this command
is used for. The only noticeable behavior change should only be that it
sped up by an order of magnitude.
To get that speed up, there is a change in behavior from git's
perspective as it is no longer doing a bunch of work it used to do
which is what is saving the time.
I was aware that skipping the commit merge/new index/merge working
tree meant that "git checkout NEW" would no longer update these to
reflect any potential changes to the sparse-checkout file.
To determine if this would change the results the user was *expecting*,
I searched the web and found that all the instructions I could locate
that taught people how to update the index/working tree after
making changes to the sparse-checkout file instructed them to use
"git read-tree -mu HEAD." I didn't find any that told people to use
"git checkout -b NEW"
Finally, when I made the optimization to skip these steps I then
verified that the test suite still passed all tests. I realize that
there is not 100% coverage of tests but I thought it was a good
indication that none of them were impacted by this optimization.
I've tried to think of a way to solve (3) in a more maintainable way
but have not been able to come up with anything. Ultimately,
to ensure are only applying the optimization in this specific case,
we have to test to make sure other options don't require the extra
steps. I'm open to suggestions!
I'm going to be out for the next 2 weeks so will be unable to respond
to activity on the thread but a co-worker who has been involved will
be responsive to feedback and rolling any new versions of the patch.
Thanks,
Ben
^ permalink raw reply
* Re: [PATCH v2 01/11] i18n: add--interactive: mark strings for translation
From: Junio C Hamano @ 2016-09-28 16:59 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1475066620.3257.12.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> As far as I understand, %12s means that the argument printed will have
> a minimum length of 12 columns. So if the translation of 'stage' is
> longer than 12 it will be printed fully no matter what. Though in that
> case, the header will not be align correctly anymore:
Exactly. That was where my suggestion comes from. In such a case
you may want to raise these numbers so that the fixed part
(i.e. header that you are letting the translators insert their
version of these words) would fit.
As Duy points out in his response to your message, that widening
further needs to take into account how many display columns each
translated words and phrases occupies, not just its byte length.
^ permalink raw reply
* [PATCH] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 16:05 UTC (permalink / raw)
To: git; +Cc: pstodulk
Delegation of credentials is disabled by default in libcurl since
version 7.21.7 due to security vulnerability CVE-2011-2192. Which
makes troubles with GSS/kerberos authentication where delegation
of credentials is required. This can be changed with option
CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
since libcurl version 7.22.0.
This patch provides new configuration variable http.delegation
which corresponds to curl parameter "--delegation" (see man 1 curl).
The following values are supported:
* none (default).
* policy
* always
Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
---
Documentation/config.txt | 14 ++++++++++++++
http.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e78293b..a179474 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1736,6 +1736,20 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
+http.delegation::
+ Control GSSAPI credential delegation. The delegation is disabled
+ by default in libcurl since version 7.21.7. Set parameter to tell
+ the server what it is allowed to delegate when it comes to user
+ credentials. Used with GSS/kerberos. Possible values are:
++
+--
+* `none` - Don't allow any delegation.
+* `policy` - Delegates if and only if the OK-AS-DELEGATE flag is set in the
+ Kerberos service ticket, which is a matter of realm policy.
+* `always` - Unconditionally allow the server to delegate.
+--
+
+
http.extraHeader::
Pass an additional HTTP header when communicating with a server. If
more than one such entry exists, all of them are added as extra
diff --git a/http.c b/http.c
index 82ed542..5f8fab3 100644
--- a/http.c
+++ b/http.c
@@ -90,6 +90,18 @@ static struct {
* here, too
*/
};
+#if LIBCURL_VERSION_NUM >= 0x071600
+static const char *curl_deleg;
+static struct {
+ const char *name;
+ long curl_deleg_param;
+} curl_deleg_levels[] = {
+ { "none", CURLGSSAPI_DELEGATION_NONE },
+ { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG },
+ { "always", CURLGSSAPI_DELEGATION_FLAG },
+};
+#endif
+
static struct credential proxy_auth = CREDENTIAL_INIT;
static const char *curl_proxyuserpwd;
static const char *curl_cookie_file;
@@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp("http.delegation", var)) {
+ return git_config_string(&curl_deleg, var, value);
+ }
+
if (!strcmp("http.pinnedpubkey", var)) {
#if LIBCURL_VERSION_NUM >= 0x072c00
return git_config_pathname(&ssl_pinnedkey, var, value);
@@ -629,6 +645,22 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
#endif
+#if LIBCURL_VERSION_NUM >= 0x071600
+ if (curl_deleg) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) {
+ if (!strcmp(curl_deleg, curl_deleg_levels[i].name)) {
+ curl_easy_setopt(result, CURLOPT_GSSAPI_DELEGATION,
+ curl_deleg_levels[i].curl_deleg_param);
+ break;
+ }
+ }
+ if (i == ARRAY_SIZE(curl_deleg_levels))
+ warning("Unknown delegation method '%s': using default",
+ curl_deleg);
+ }
+#endif
+
if (http_proactive_auth)
init_curl_http_auth(result);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH v2] gpg-interface: use more status letters
From: Ramsay Jones @ 2016-09-28 15:10 UTC (permalink / raw)
To: Michael J Gruber, git; +Cc: Alex
In-Reply-To: <c4777ef68059034d7ad4697a06bba3cabbdc9265.1475053649.git.git@drmicha.warpmail.net>
On 28/09/16 15:24, Michael J Gruber wrote:
> According to gpg2's doc/DETAILS:
> "For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
> EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
>
> gpg1 ("classic") behaves the same (although doc/DETAILS
> differs).
>
> Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
> information and translate that into status codes G, B, U, N for the %G?
> format specifier.
>
> git-verify-* returns success in the GOODSIG case only. This is somewhat in
> disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
> but we err on the very safe side.
>
> Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
> so that a user of %G? gets more information about the absence of a 'G'
> on first glance.
>
> Requested-by: Alex <agrambot@gmail.com>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> Changes in v2:
>
> - Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
> - Do not parse for signer UID in the ERRSIG case (and test that we do not).
> - Retreat "rather" addition from the doc: good/valid are terms that we use
> differently from gpg anyways.
>
> Documentation/pretty-formats.txt | 9 +++++++--
> gpg-interface.c | 13 ++++++++++---
> pretty.c | 3 +++
> t/t7510-signed-commit.sh | 12 +++++++++++-
> 4 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index a942d57..c28ff2b 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -143,8 +143,13 @@ ifndef::git-rev-list[]
> - '%N': commit notes
> endif::git-rev-list[]
> - '%GG': raw verification message from GPG for a signed commit
> -- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
> - "U" for a good signature with unknown validity and "N" for no signature
> +- '%G?': show "G" for a good (valid) signature,
> + "B" for a bad signature,
> + "U" for a good signature with unknown validity,
> + "X" for a good expired signature, or good signature made by an expired key,
Hmm, this looks odd. Would the following:
"X" for a good signature made with an expired key,
mean something different?
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v2 01/11] i18n: add--interactive: mark strings for translation
From: Duy Nguyen @ 2016-09-28 14:29 UTC (permalink / raw)
To: Vasco Almeida
Cc: Junio C Hamano, Git Mailing List, Jiang Xin,
Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <1475066620.3257.12.camel@sapo.pt>
On Wed, Sep 28, 2016 at 7:43 PM, Vasco Almeida <vascomalmeida@sapo.pt> wrote:
> A Dom, 25-09-2016 às 15:52 -0700, Junio C Hamano escreveu:
>> > @@ -252,7 +253,7 @@ sub list_untracked {
>> > }
>> >
>> > my $status_fmt = '%12s %12s %s';
>> > -my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
>> > +my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), __('path'));
>>
>> Wouldn't it make sense to allow translators to tweak $status_fmt if
>> you are allowing the earlier elements that are formatted with %12s,
>> as their translation may not fit within that width, in which case
>> they may want to make these columns wider?
>
> As far as I understand, %12s means that the argument printed will have
> a minimum length of 12 columns. So if the translation of 'stage' is
> longer than 12 it will be printed fully no matter what. Though in that
> case, the header will not be align correctly anymore:
> for other instances of this in the present patch series.
It's 12 bytes, not columns (unless perl understands input string's
encoding, which I doubt). Think about multi-byte encodings like utf-8,
where three letters (or "columns") do not necessary mean three bytes.
The result is most likely unaligned in that case.
--
Duy
^ permalink raw reply
* [PATCH v2] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-09-28 14:24 UTC (permalink / raw)
To: git; +Cc: Alex
In-Reply-To: <xmqqk2dxp84i.fsf@gitster.mtv.corp.google.com>
According to gpg2's doc/DETAILS:
"For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
gpg1 ("classic") behaves the same (although doc/DETAILS
differs).
Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
information and translate that into status codes G, B, U, N for the %G?
format specifier.
git-verify-* returns success in the GOODSIG case only. This is somewhat in
disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
but we err on the very safe side.
Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
so that a user of %G? gets more information about the absence of a 'G'
on first glance.
Requested-by: Alex <agrambot@gmail.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Changes in v2:
- Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
- Do not parse for signer UID in the ERRSIG case (and test that we do not).
- Retreat "rather" addition from the doc: good/valid are terms that we use
differently from gpg anyways.
Documentation/pretty-formats.txt | 9 +++++++--
gpg-interface.c | 13 ++++++++++---
pretty.c | 3 +++
t/t7510-signed-commit.sh | 12 +++++++++++-
4 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..c28ff2b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,13 @@ ifndef::git-rev-list[]
- '%N': commit notes
endif::git-rev-list[]
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
- "U" for a good signature with unknown validity and "N" for no signature
+- '%G?': show "G" for a good (valid) signature,
+ "B" for a bad signature,
+ "U" for a good signature with unknown validity,
+ "X" for a good expired signature, or good signature made by an expired key,
+ "R" for a good signature made by a revoked key,
+ "E" if the signature cannot be checked (e.g. missing key)
+ and "N" for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}` or
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..6999e7b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -33,6 +33,10 @@ static struct {
{ 'B', "\n[GNUPG:] BADSIG " },
{ 'U', "\n[GNUPG:] TRUST_NEVER" },
{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
+ { 'E', "\n[GNUPG:] ERRSIG "},
+ { 'X', "\n[GNUPG:] EXPSIG "},
+ { 'X', "\n[GNUPG:] EXPKEYSIG "},
+ { 'R', "\n[GNUPG:] REVKEYSIG "},
};
void parse_gpg_output(struct signature_check *sigc)
@@ -54,9 +58,12 @@ void parse_gpg_output(struct signature_check *sigc)
/* The trust messages are not followed by key/signer information */
if (sigc->result != 'U') {
sigc->key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc->signer = xmemdupz(found, next - found);
+ /* The ERRSIG message is not followed by signer information */
+ if (sigc-> result != 'E') {
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc->signer = xmemdupz(found, next - found);
+ }
}
}
}
diff --git a/pretty.c b/pretty.c
index 493edb0..39a36cd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1232,8 +1232,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
switch (c->signature_check.result) {
case 'G':
case 'B':
+ case 'E':
case 'U':
case 'N':
+ case 'X':
+ case 'R':
strbuf_addch(sb, c->signature_check.result);
}
break;
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e839f5..9f487f9 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -190,7 +190,7 @@ test_expect_success GPG 'show bad signature with custom format' '
test_cmp expect actual
'
-test_expect_success GPG 'show unknown signature with custom format' '
+test_expect_success GPG 'show untrusted signature with custom format' '
cat >expect <<-\EOF &&
U
61092E85B7227189
@@ -200,6 +200,16 @@ test_expect_success GPG 'show unknown signature with custom format' '
test_cmp expect actual
'
+test_expect_success GPG 'show unknown signature with custom format' '
+ cat >expect <<-\EOF &&
+ E
+ 61092E85B7227189
+
+ EOF
+ GNUPGHOME="$HOME/gnupg-home-not-used" git log -1 --format="%G?%n%GK%n%GS" eighth-signed-alt >actual &&
+ test_cmp expect actual
+'
+
test_expect_success GPG 'show lack of signature with custom format' '
cat >expect <<-\EOF &&
N
--
2.10.0.527.gbcb6904
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox