* 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
* [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
* [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 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
* 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
* Re: [PATCH v4 1/2] t5100-mailinfo: replace common path prefix with variable
From: Junio C Hamano @ 2016-09-28 20:21 UTC (permalink / raw)
To: Kevin Daudt; +Cc: git, Swift Geek, Jeff King
In-Reply-To: <20160928195232.7843-1-me@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> 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.
Wow. I was half expecting that you'd say something like "1/2 plus
the SQUASH is OK by me", but you went extra mile to do it right.
Impressed, and very much appreciated.
^ permalink raw reply
* Re: [PATCH v4 1/2] t5100-mailinfo: replace common path prefix with variable
From: Kevin Daudt @ 2016-09-28 20:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Swift Geek, Jeff King
In-Reply-To: <xmqqoa37ixmu.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 28, 2016 at 01:21:13PM -0700, Junio C Hamano wrote:
> Kevin Daudt <me@ikke.info> writes:
>
> > 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.
>
> Wow. I was half expecting that you'd say something like "1/2 plus
> the SQUASH is OK by me", but you went extra mile to do it right.
>
> Impressed, and very much appreciated.
>
You're What's Cooking mail mentioned you expected a reroll, so I guessed
that I could just fix this part as well.
^ 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 20:33 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <xmqqzimrj03j.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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:
> ...
Please do not use that verbatim; it was full of typo and grammo.
> 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 00/11] Resumable clone
From: Eric Wong @ 2016-09-28 20:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Wern, git
In-Reply-To: <xmqqshslkndk.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
>
> >> [primeclone]
> >> url = http://location/pack-$NAME.pack
> >> filetype = pack
> >
> > If unconfigured, I wonder if a primeclone pack can be inferred by
> > the existence of a pack bitmap (or merely being the biggest+oldest
> > pack for dumb HTTP).
>
> That would probably be a nice heuristics but it is unclear who
> should find that out at runtime. The downloading side would not
> have a visiblity into directory listing.
I think making a bunch of HEAD requests based on the contents of
$GIT_DIR/objects/info/packs wouldn't be too expensive on either
end, especially when HTTP/1.1 persistent connections + pipelining
may be used.
^ permalink raw reply
* Re: [PATCH v2] gpg-interface: use more status letters
From: Ramsay Jones @ 2016-09-28 21:09 UTC (permalink / raw)
To: Junio C Hamano, Michael J Gruber; +Cc: git, Alex
In-Reply-To: <xmqqshsjiyn4.fsf@gitster.mtv.corp.google.com>
On 28/09/16 20:59, Junio C Hamano wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> + "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.
Kinda! ;-)
I'm not sure that it is a good idea to mash both EXPSIG and EXPKEYSIG
into one status letter, but I was also fishing for some information
about EXPSIG. I was only vaguely aware that a signature could expire
_independently_ of the key used to do the signing. Also, according to
https://www.gnupg.org/documentation/manuals/gnupg/Automated-signature-checking.html
for the EXPSIG case 'Note, that this case is currently not implemented.'
Hmm, I guess these are so closely related that a single status letter
is OK, but I think I would prefer your phrasing; namely:
"X" for a good signature that has expired, or a good signature made with an expired key,
[Although that is still a bit cumbersome.]
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Junio C Hamano @ 2016-09-28 21:23 UTC (permalink / raw)
To: Petr Stodulka; +Cc: Jeff King, git
In-Reply-To: <9c9e9949-cfe0-5465-6ae1-3e8254ff055b@redhat.com>
Petr Stodulka <pstodulk@redhat.com> writes:
> 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.
Thanks; let's queue this in 'pu' to make it easier for people who
would be affected to try it out.
^ permalink raw reply
* [PATCH] Replace deprecated CURLAUTH_GSSNEGOTIATE with CURLAUTH_NEGOTIATE.
From: Petr Stodulka @ 2016-09-28 21:20 UTC (permalink / raw)
To: git; +Cc: pstodulk
Macro CURLAUTH_GSSNEGOTIATE is deprecated since cURL v7.38.0 and
should be used CURLAUTH_NEGOTIATE instead. For compatibility
with older versions of cURL is CURLAUTH_NEGOTIATE set as alias
to CURLAUTH_GSSNEGOTIATE
Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
---
http.c | 4 ++--
http.h | 7 +++++++
remote-curl.c | 2 +-
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index 82ed542..7c2d9ef 100644
--- a/http.c
+++ b/http.c
@@ -79,7 +79,7 @@ static struct {
} proxy_authmethods[] = {
{ "basic", CURLAUTH_BASIC },
{ "digest", CURLAUTH_DIGEST },
- { "negotiate", CURLAUTH_GSSNEGOTIATE },
+ { "negotiate", CURLAUTH_NEGOTIATE },
{ "ntlm", CURLAUTH_NTLM },
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
{ "anyauth", CURLAUTH_ANY },
@@ -1277,7 +1277,7 @@ static int handle_curl_result(struct slot_results *results)
return HTTP_NOAUTH;
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
- http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+ http_auth_methods &= ~CURLAUTH_NEGOTIATE;
#endif
return HTTP_REAUTH;
}
diff --git a/http.h b/http.h
index 5ab9d9c..bcc7d7d 100644
--- a/http.h
+++ b/http.h
@@ -42,6 +42,13 @@
#endif
/*
+ * Keep it compatible on system with cURL < 7.38.0
+ * */
+#ifndef CURLAUTH_NEGOTIATE
+#define CURLAUTH_NEGOTIATE CURLAUTH_GSSNEGOTIATE
+#endif
+
+/*
* CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
* and the constants were known as CURLFTPSSL_*
*/
diff --git a/remote-curl.c b/remote-curl.c
index 6b83b77..d4fce63 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -542,7 +542,7 @@ static int post_rpc(struct rpc_state *rpc)
if (err != HTTP_OK)
return -1;
- if (results.auth_avail & CURLAUTH_GSSNEGOTIATE)
+ if (results.auth_avail & CURLAUTH_NEGOTIATE)
needs_100_continue = 1;
}
--
2.5.5
^ permalink raw reply related
* Re: [PATCH v8 00/11] Git filter protocol
From: Junio C Hamano @ 2016-09-28 21:49 UTC (permalink / raw)
To: larsxschneider; +Cc: git, peff, sbeller, jnareb, mlbright, tboegi, ramsay
In-Reply-To: <20160920190247.82189-1-larsxschneider@gmail.com>
I suspect that you are preparing a reroll already, but the one that
is sitting in 'pu' seems to be flaky in t/t0021 and I seem to see
occasional failures from it.
I didn't trace where the test goes wrong, but one easy mistake you
could make (I am not saying that is the reason of the failure) is to
assume your filter will not be called under certain condition (like
immediately after you checked out from the index to the working
tree), when the automated test goes fast enough and get you into a
"racy git" situation---the filter may be asked to filter the
contents from the working tree again to re-validate what's there is
still what is in the index.
^ permalink raw reply
* [PATCH v5 0/4] recursive support for ls-files
From: Brandon Williams @ 2016-09-28 21:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1474930003-83750-1-git-send-email-bmwill@google.com>
The big change in this version is the introduction of a --super-prefix option
to the top level git. After much discussion this seemed to be a better naming
scheme than 'submodule-prefix' as it could be an option other cmds could use
independent of submodules.
In 3/4 I changed the compile_submodule_options function to compile all options that can be realistically passed through and when an option that isn't supported (or rather safe) yet is provided the caller will be errored out.
Brandon Williams (4):
git: make super-prefix option
ls-files: optionally recurse into submodules
ls-files: pass through safe options for --recurse-submodules
ls-files: add pathspec matching for submodules
Documentation/git-ls-files.txt | 7 +-
Documentation/git.txt | 6 +
builtin/ls-files.c | 202 ++++++++++++++++++++++++-------
cache.h | 2 +
dir.c | 46 +++++++-
dir.h | 4 +
environment.c | 10 ++
git.c | 24 +++-
t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
9 files changed, 466 insertions(+), 44 deletions(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH v5 1/4] git: make super-prefix option
From: Brandon Williams @ 2016-09-28 21:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475099443-145608-1-git-send-email-bmwill@google.com>
Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
which can be used to specify a path from above a repository down to its
root. The immediate use of this option is by commands which have a
--recurse-submodule option in order to give context to submodules about
how they were invoked. This option is currently only allowed for
builtins which support a super-prefix.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git.txt | 6 ++++++
cache.h | 2 ++
environment.c | 10 ++++++++++
git.c | 22 ++++++++++++++++++++++
4 files changed, 40 insertions(+)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..e3309b9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
[--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
[--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+ [--super-prefix=<path>]
<command> [<args>]
DESCRIPTION
@@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
details. Equivalent to setting the `GIT_NAMESPACE` environment
variable.
+--super-prefix=<path>::
+ Set a prefix which gives a path from above a repository down to its
+ root. One use is to give submodules context about the superproject that
+ invoked it. Currently for internal use only.
+
--bare::
Treat the repository as a bare repository. If GIT_DIR
environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..01730e1 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
#define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
#define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
#define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
#define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
#define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
#define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
@@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
extern int get_common_dir(struct strbuf *sb, const char *gitdir);
extern const char *get_git_namespace(void);
extern const char *strip_namespace(const char *namespaced_ref);
+extern const char *get_super_prefix();
extern const char *get_git_work_tree(void);
/*
diff --git a/environment.c b/environment.c
index ca72464..1a2a779 100644
--- a/environment.c
+++ b/environment.c
@@ -100,6 +100,8 @@ static char *work_tree;
static const char *namespace;
static size_t namespace_len;
+static const char *super_prefix;
+
static const char *git_dir, *git_common_dir;
static char *git_object_dir, *git_index_file, *git_graft_file;
int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -120,6 +122,7 @@ const char * const local_repo_env[] = {
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
+ GIT_SUPER_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
GIT_COMMON_DIR_ENVIRONMENT,
NULL
@@ -222,6 +225,13 @@ const char *strip_namespace(const char *namespaced_ref)
return namespaced_ref + namespace_len;
}
+const char *get_super_prefix()
+{
+ if (!super_prefix)
+ super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
+ return super_prefix;
+}
+
static int git_work_tree_initialized;
/*
diff --git a/git.c b/git.c
index 1c61151..3e5cd63 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
if (envchanged)
*envchanged = 1;
+ } else if (!strcmp(cmd, "--super-prefix")) {
+ if (*argc < 2) {
+ fprintf(stderr, "No prefix given for --super-prefix.\n" );
+ usage(git_usage_string);
+ }
+ setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+ if (envchanged)
+ *envchanged = 1;
+ (*argv)++;
+ (*argc)--;
+ } else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
+ setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
+ if (envchanged)
+ *envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
char *cwd = xgetcwd();
is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
* RUN_SETUP for reading from the configuration file.
*/
#define NEED_WORK_TREE (1<<3)
+#define SUPPORT_SUPER_PREFIX (1<<4)
struct cmd_struct {
const char *cmd;
@@ -344,6 +359,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
}
commit_pager_choice();
+ if (!help && get_super_prefix()) {
+ if (!(p->option & SUPPORT_SUPER_PREFIX))
+ die("%s doesn't support --super-prefix", p->cmd);
+ if (prefix)
+ die("can't have both a prefix and super-prefix");
+ }
+
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v5 2/4] ls-files: optionally recurse into submodules
From: Brandon Williams @ 2016-09-28 21:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475099443-145608-1-git-send-email-bmwill@google.com>
Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules. This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--super-prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 7 +-
builtin/ls-files.c | 139 ++++++++++++++++++++++++---------
git.c | 2 +-
t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
4 files changed, 208 insertions(+), 40 deletions(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
[--exclude-per-directory=<file>]
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
- [--full-name] [--abbrev] [--] [<file>...]
+ [--full-name] [--recurse-submodules]
+ [--abbrev] [--] [<file>...]
DESCRIPTION
-----------
@@ -137,6 +138,10 @@ a space) at the start of each line:
option forces paths to be output relative to the project
top directory.
+--recurse-submodules::
+ Recursively calls ls-files on each submodule in the repository.
+ Currently there is only support for the --cached mode.
+
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..e0e5cf5 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "string-list.h"
#include "pathspec.h"
+#include "run-command.h"
static int abbrev;
static int show_deleted;
@@ -28,8 +29,10 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
+static int recurse_submodules;
static const char *prefix;
+static const char *super_prefix;
static int max_prefix_len;
static int prefix_len;
static struct pathspec pathspec;
@@ -68,6 +71,19 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
static void write_name(const char *name)
{
/*
+ * Prepend the super_prefix to name to construct the full_name to be
+ * written. 'full_name' gets reused across output lines to minimize the
+ * allocation churn.
+ */
+ static struct strbuf full_name = STRBUF_INIT;
+ if (super_prefix && *super_prefix) {
+ strbuf_reset(&full_name);
+ strbuf_addstr(&full_name, super_prefix);
+ strbuf_addstr(&full_name, name);
+ name = full_name.buf;
+ }
+
+ /*
* With "--full-name", prefix_len=0; this caller needs to pass
* an empty string in that case (a NULL is good for "").
*/
@@ -152,55 +168,84 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int status;
+
+ argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
+ super_prefix ? super_prefix : "",
+ ce->name);
+ argv_array_push(&cp.args, "ls-files");
+ argv_array_push(&cp.args, "--recurse-submodules");
+
+ cp.git_cmd = 1;
+ cp.dir = ce->name;
+ status = run_command(&cp);
+ if (status)
+ exit(status);
+}
+
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+ if (super_prefix)
+ strbuf_addstr(&name, super_prefix);
+ strbuf_addstr(&name, ce->name);
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
- len, ps_matched,
- S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
- return;
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ show_gitlink(ce);
+ } else if (match_pathspec(&pathspec, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ if (tag && *tag && show_valid_bit &&
+ (ce->ce_flags & CE_VALID)) {
+ static char alttag[4];
+ memcpy(alttag, tag, 3);
+ if (isalpha(tag[0]))
+ alttag[0] = tolower(tag[0]);
+ else if (tag[0] == '?')
+ alttag[0] = '!';
+ else {
+ alttag[0] = 'v';
+ alttag[1] = tag[0];
+ alttag[2] = ' ';
+ alttag[3] = 0;
+ }
+ tag = alttag;
+ }
- if (tag && *tag && show_valid_bit &&
- (ce->ce_flags & CE_VALID)) {
- static char alttag[4];
- memcpy(alttag, tag, 3);
- if (isalpha(tag[0]))
- alttag[0] = tolower(tag[0]);
- else if (tag[0] == '?')
- alttag[0] = '!';
- else {
- alttag[0] = 'v';
- alttag[1] = tag[0];
- alttag[2] = ' ';
- alttag[3] = 0;
+ if (!show_stage) {
+ fputs(tag, stdout);
+ } else {
+ printf("%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
+ }
+ write_eolinfo(ce, ce->name);
+ write_name(ce->name);
+ if (debug_mode) {
+ const struct stat_data *sd = &ce->ce_stat_data;
+
+ printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+ printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+ printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+ printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+ printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
}
- tag = alttag;
}
- if (!show_stage) {
- fputs(tag, stdout);
- } else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
- }
- write_eolinfo(ce, ce->name);
- write_name(ce->name);
- if (debug_mode) {
- const struct stat_data *sd = &ce->ce_stat_data;
-
- printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
- printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
- printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
- printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
- printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
- }
+ strbuf_release(&name);
}
static void show_ru_info(void)
@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+ OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+ N_("recurse through submodules")),
OPT_BOOL(0, "error-unmatch", &error_unmatch,
N_("if any <file> is not in the index, treat this as an error")),
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -484,6 +531,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
prefix = cmd_prefix;
if (prefix)
prefix_len = strlen(prefix);
+ super_prefix = get_super_prefix();
git_config(git_default_config, NULL);
if (read_cache() < 0)
@@ -519,6 +567,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules &&
+ (show_stage || show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo ||
+ show_valid_bit || show_tag || show_eol || with_tree ||
+ (line_terminator == '\0')))
+ die("ls-files --recurse-submodules unsupported mode");
+
+ if (recurse_submodules && error_unmatch)
+ die("ls-files --recurse-submodules does not support "
+ "--error-unmatch");
+
+ if (recurse_submodules && argc)
+ die("ls-files --recurse-submodules does not support path "
+ "arguments");
+
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index 3e5cd63..86d9be4 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@ static struct cmd_struct commands[] = {
{ "init-db", cmd_init_db },
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
- { "ls-files", cmd_ls_files, RUN_SETUP },
+ { "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..7d225ac
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+ echo a >a &&
+ mkdir b &&
+ echo b >b/b &&
+ git add a b &&
+ git commit -m "add a and b" &&
+ git init submodule &&
+ echo c >submodule/c &&
+ git -C submodule add c &&
+ git -C submodule commit -m "add c" &&
+ git submodule add ./submodule &&
+ git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ echo a >not_added &&
+ echo b >b/not_added &&
+ echo c >submodule/not_added &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/.gitmodules
+ submodule/c
+ submodule/subsub/d
+ EOF
+
+ git init submodule/subsub &&
+ echo d >submodule/subsub/d &&
+ git -C submodule/subsub add d &&
+ git -C submodule/subsub commit -m "add d" &&
+ git -C submodule submodule add ./subsub &&
+ git -C submodule commit -m "added subsub" &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+ test_must_fail git ls-files --recurse-submodules b 2>actual &&
+ test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+ test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+ test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+ test_expect_success "--recurse-submodules and $1 are incompatible" "
+ test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+ test_i18ngrep 'unsupported mode' actual
+ "
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v5 3/4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-28 21:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475099443-145608-1-git-send-email-bmwill@google.com>
Pass through some known-safe options when recursing into submodules.
(--cached, --stage, -v, -t, -z, --debug, --eol)
Other options are compiled into an argv_array but if an unsafe option is
given the caller will be errored out.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 51 ++++++++++++++++++++++++++++++++--
t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++----
2 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e0e5cf5..f377e36 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
static const char *prefix;
static const char *super_prefix;
@@ -168,6 +169,45 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+ if (line_terminator == '\0')
+ argv_array_push(&submodules_options, "-z");
+ if (show_tag)
+ argv_array_push(&submodules_options, "-t");
+ if (show_valid_bit)
+ argv_array_push(&submodules_options, "-v");
+ if (show_cached)
+ argv_array_push(&submodules_options, "--cached");
+ if (show_deleted)
+ argv_array_push(&submodules_options, "--deleted");
+ if (show_modified)
+ argv_array_push(&submodules_options, "--modified");
+ if (show_others)
+ argv_array_push(&submodules_options, "--others");
+ if (dir->flags & DIR_SHOW_IGNORED)
+ argv_array_push(&submodules_options, "--ignored");
+ if (show_stage)
+ argv_array_push(&submodules_options, "--stage");
+ if (show_killed)
+ argv_array_push(&submodules_options, "--killed");
+ if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
+ argv_array_push(&submodules_options, "--directory");
+ if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
+ argv_array_push(&submodules_options, "--empty-directory");
+ if (show_unmerged)
+ argv_array_push(&submodules_options, "--unmerged");
+ if (show_resolve_undo)
+ argv_array_push(&submodules_options, "--resolve-undo");
+ if (show_eol)
+ argv_array_push(&submodules_options, "--eol");
+ if (debug_mode)
+ argv_array_push(&submodules_options, "--debug");
+}
+
/**
* Recursively call ls-files on a submodule
*/
@@ -182,6 +222,9 @@ static void show_gitlink(const struct cache_entry *ce)
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
+ /* add supported options */
+ argv_array_pushv(&cp.args, submodules_options.argv);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -567,11 +610,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules)
+ compile_submodule_options(&dir, show_tag);
+
if (recurse_submodules &&
- (show_stage || show_deleted || show_others || show_unmerged ||
+ (show_deleted || show_others || show_unmerged ||
show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol || with_tree ||
- (line_terminator == '\0')))
+ with_tree))
die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 7d225ac..79107d8 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
test_cmp expect actual
'
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+ lf_to_nul >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules -z >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'ls-files does not output files not added to a repo' '
cat >expect <<-\EOF &&
.gitmodules
@@ -86,15 +98,10 @@ test_incompatible_with_recurse_submodules () {
"
}
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v5 4/4] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-28 21:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475099443-145608-1-git-send-email-bmwill@google.com>
Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule. The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.
This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally. If any wildcard character is encountered we'll simply punt
and produce a false positive match. More accurate matching will be done
once inside the submodule. This is due to the superproject not knowing
what files could exist in the submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 28 ++++++---
dir.c | 46 +++++++++++++-
dir.h | 4 ++
t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
4 files changed, 174 insertions(+), 12 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index f377e36..c8c5c29 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -215,6 +215,7 @@ static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+ int i;
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
@@ -225,6 +226,15 @@ static void show_gitlink(const struct cache_entry *ce)
/* add supported options */
argv_array_pushv(&cp.args, submodules_options.argv);
+ /*
+ * Pass in the original pathspec args. The submodule will be
+ * responsible for prepending the 'submodule_prefix' prior to comparing
+ * against the pathspec for matches.
+ */
+ argv_array_push(&cp.args, "--");
+ for (i = 0; i < pathspec.nr; i++)
+ argv_array_push(&cp.args, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -243,7 +253,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(&pathspec, name.buf, ps_matched)) {
show_gitlink(ce);
} else if (match_pathspec(&pathspec, name.buf, name.len,
len, ps_matched,
@@ -623,17 +634,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
die("ls-files --recurse-submodules does not support "
"--error-unmatch");
- if (recurse_submodules && argc)
- die("ls-files --recurse-submodules does not support path "
- "arguments");
-
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
- /* Find common prefix for all pathspec's */
- max_prefix = common_prefix(&pathspec);
+ /*
+ * Find common prefix for all pathspec's
+ * This is used as a performance optimization which unfortunately cannot
+ * be done when recursing into submodules
+ */
+ if (recurse_submodules)
+ max_prefix = NULL;
+ else
+ max_prefix = common_prefix(&pathspec);
max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
return 1;
}
-#define DO_MATCH_EXCLUDE 1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
/*
* Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ /* name is a literal prefix of the pathspec */
+ if ((namelen < matchlen) &&
+ (match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen))
+ return MATCHED_RECURSIVELY;
+
+ /* name" doesn't match up to the first wild character */
+ if (item->nowildcard_len < item->len &&
+ ps_strncmp(item, match, name,
+ item->nowildcard_len - prefix))
+ return 0;
+
+ /*
+ * Here is where we would perform a wildmatch to check if
+ * "name" can be matched as a directory (or a prefix) against
+ * the pathspec. Since wildmatch doesn't have this capability
+ * at the present we have to punt and say that it is a match,
+ * potentially returning a false positive
+ * The submodules themselves will be able to perform more
+ * accurate matching to determine if the pathspec matches.
+ */
+ return MATCHED_RECURSIVELY;
+ }
+
return 0;
}
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen)
+{
+ int matched = do_match_pathspec(ps, submodule_name,
+ strlen(submodule_name),
+ 0, seen,
+ DO_MATCH_DIRECTORY |
+ DO_MATCH_SUBMODULE);
+ return matched;
+}
+
int report_path_error(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix);
+extern int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen);
+
static inline int ce_path_match(const struct cache_entry *ce,
const struct pathspec *pathspec,
char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 79107d8..5475855 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
test_cmp expect actual
'
-test_expect_success '--recurse-submodules does not support using path arguments' '
- test_must_fail git ls-files --recurse-submodules b 2>actual &&
- test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+ echo e >submodule/subsub/e.txt &&
+ git -C submodule/subsub add e.txt &&
+ git -C submodule/subsub commit -m "adding e.txt" &&
+ echo f >submodule/f.TXT &&
+ echo g >submodule/g.txt &&
+ git -C submodule add f.TXT g.txt &&
+ git -C submodule commit -m "add f and g" &&
+ echo h >h.txt &&
+ mkdir sib &&
+ echo sib >sib/file &&
+ git add h.txt sib/file &&
+ git commit -m "add h and sib/file" &&
+ git init sub &&
+ echo sub >sub/file &&
+ git -C sub add file &&
+ git -C sub commit -m "add file" &&
+ git submodule add ./sub &&
+ git commit -m "added sub" &&
+
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ sib/file
+ sub/file
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual &&
+ cat actual &&
+ git ls-files --recurse-submodules "*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "sub" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su*/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su?/file" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sib/file
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "s??/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s???file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s*file" >actual &&
+ test_cmp expect actual
'
test_expect_success '--recurse-submodules does not support --error-unmatch' '
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH] blame: use DEFAULT_ABBREV macro
From: Junio C Hamano @ 2016-09-28 21:57 UTC (permalink / raw)
To: git
This does not make any practical difference in today's code, but
everybody else accesses the default abbreviation length via the
DEFAULT_ABBREV macro. Make sure this oddball codepath does not
stray from the convention.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/blame.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index a5bbf91..f618392 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2111,7 +2111,7 @@ static void find_alignment(struct scoreboard *sb, int *option)
unsigned largest_score = 0;
struct blame_entry *e;
int compute_auto_abbrev = (abbrev < 0);
- int auto_abbrev = default_abbrev;
+ int auto_abbrev = DEFAULT_ABBREV;
for (e = sb->ent; e; e = e->next) {
struct origin *suspect = e->suspect;
--
2.10.0-582-gfa57e49
^ permalink raw reply related
* Re: [PATCH v5 1/4] git: make super-prefix option
From: Stefan Beller @ 2016-09-28 22:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <1475099443-145608-2-git-send-email-bmwill@google.com>
On Wed, Sep 28, 2016 at 2:50 PM, Brandon Williams <bmwill@google.com> wrote:
>
> DESCRIPTION
> @@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
> details. Equivalent to setting the `GIT_NAMESPACE` environment
> variable.
>
> +--super-prefix=<path>::
> + Set a prefix which gives a path from above a repository down to its
> + root. One use is to give submodules context about the superproject that
> + invoked it. Currently for internal use only.
I would put the "Currently for internal use only." at the beginning, so end
users don't have to bother reading though the description, when they want to
only use kosher flags. (Well, checking `man git fetch` and searching
for 'internal',
there doesn't seem to be a consistent way how to document internal flags :(.
It doesn't however advertise the flag in the SYNOPSIS. Ok it doesn't advertise
a lot of flags in its SYNOPSIS)
> +const char *get_super_prefix()
> +{
> + if (!super_prefix)
> + super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
The getenv() function returns a pointer to the value in the
environment, or NULL if there is no match.
So in case this is not set (when e.g. the user did not specify the
super prefix),
we would probe it a couple of times.
The caching effect only occurs when the string is set. So this looks
like we save repetitive
calls, but we do not always do that.
^ permalink raw reply
* Re: [PATCH v5 2/4] ls-files: optionally recurse into submodules
From: Stefan Beller @ 2016-09-28 22:11 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <1475099443-145608-3-git-send-email-bmwill@google.com>
On Wed, Sep 28, 2016 at 2:50 PM, Brandon Williams <bmwill@google.com> wrote:
> Allow ls-files to recognize submodules in order to retrieve a list of
> files from a repository's submodules. This is done by forking off a
> process to recursively call ls-files on all submodules. Use top-level
> --super-prefix option to pass a path to the submodule which it can
> use to prepend to output or pathspec matching logic.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/git-ls-files.txt | 7 +-
> builtin/ls-files.c | 139 ++++++++++++++++++++++++---------
> git.c | 2 +-
> t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
> 4 files changed, 208 insertions(+), 40 deletions(-)
> create mode 100755 t/t3007-ls-files-recurse-submodules.sh
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 0d933ac..446209e 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -18,7 +18,8 @@ SYNOPSIS
> [--exclude-per-directory=<file>]
> [--exclude-standard]
> [--error-unmatch] [--with-tree=<tree-ish>]
> - [--full-name] [--abbrev] [--] [<file>...]
> + [--full-name] [--recurse-submodules]
> + [--abbrev] [--] [<file>...]
>
> DESCRIPTION
> -----------
> @@ -137,6 +138,10 @@ a space) at the start of each line:
> option forces paths to be output relative to the project
> top directory.
>
> +--recurse-submodules::
> + Recursively calls ls-files on each submodule in the repository.
> + Currently there is only support for the --cached mode.
> +
> --abbrev[=<n>]::
> Instead of showing the full 40-byte hexadecimal object
> lines, show only a partial prefix.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 00ea91a..e0e5cf5 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -14,6 +14,7 @@
> #include "resolve-undo.h"
> #include "string-list.h"
> #include "pathspec.h"
> +#include "run-command.h"
>
> static int abbrev;
> static int show_deleted;
> @@ -28,8 +29,10 @@ static int show_valid_bit;
> static int line_terminator = '\n';
> static int debug_mode;
> static int show_eol;
> +static int recurse_submodules;
>
> static const char *prefix;
> +static const char *super_prefix;
> static int max_prefix_len;
> static int prefix_len;
> static struct pathspec pathspec;
> @@ -68,6 +71,19 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
> static void write_name(const char *name)
> {
> /*
> + * Prepend the super_prefix to name to construct the full_name to be
> + * written. 'full_name' gets reused across output lines to minimize the
> + * allocation churn.
> + */
When doing these tricks with the allocation churn (i.e. we make it
hard to read and understand
for a reader, then we should do it completely, i.e. keep full_name in
the strbuf and
only do a strbuf_setlen to reset the buffer just a bit. With this
implementation we burden the
reader/user to understand how the memory is kept over multiple calls
to this function,
but we still do more work than expected). So either I'd not worry
about performance
and provide an 'obvious correct' implementation, with e.g. no static
here and we free the memory
correctly. Or you'd go the performance route, but then we'd usually
ask for numbers.
(How much faster is it; Does the trickyness trade off well to the
performance gain?)
> + static struct strbuf full_name = STRBUF_INIT;
> + if (super_prefix && *super_prefix) {
Why do we have to check twice here? Wouldn't just
if (super_prefix) {
...
be enough?
^ permalink raw reply
* Re: [PATCH v5 1/4] git: make super-prefix option
From: Junio C Hamano @ 2016-09-28 22:19 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, peff
In-Reply-To: <1475099443-145608-2-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> which can be used to specify a path from above a repository down to its
> root. The immediate use of this option is by commands which have a
> --recurse-submodule option in order to give context to submodules about
> how they were invoked. This option is currently only allowed for
> builtins which support a super-prefix.
Yes, it can be used to specify that, but it is unclear (1) how such
a path thusly specified is used, (2) how it affects the outcome of
the operations, and (3) what it means (the same applies to the
documentation update).
It would help future readers of "git log", "tig blame", etc. to have
a few sentences after the first sentence above. Perhaps
... specify a path from above ... down to its root. When such a
super-prefix is specified, the paths reported by Git are
prefixed with it to make them relative to that directory
"above". The paths given by the users on the command line
(e.g. "git subcmd --output-file=path/to/a/file" and pathspecs)
are taken relative to that directory "above" to match.
or something like that?
> @@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
> extern int get_common_dir(struct strbuf *sb, const char *gitdir);
> extern const char *get_git_namespace(void);
> extern const char *strip_namespace(const char *namespaced_ref);
> +extern const char *get_super_prefix();
Nice, but
extern const char *get_super_prefix(void);
> +const char *get_super_prefix()
Likewise.
> +{
> + if (!super_prefix)
> + super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> + return super_prefix;
> +}
Good.
> commit_pager_choice();
>
> + if (!help && get_super_prefix()) {
> + if (!(p->option & SUPPORT_SUPER_PREFIX))
> + die("%s doesn't support --super-prefix", p->cmd);
> + if (prefix)
> + die("can't have both a prefix and super-prefix");
> + }
Nice. I'd phrase the latter as "can't use--super-prefix from a
subdirectory", though.
^ permalink raw reply
* Re: [PATCH v5 2/4] ls-files: optionally recurse into submodules
From: Junio C Hamano @ 2016-09-28 22:22 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, peff
In-Reply-To: <1475099443-145608-3-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +--recurse-submodules::
> + Recursively calls ls-files on each submodule in the repository.
> + Currently there is only support for the --cached mode.
Good to describe it, but at this step, there is only support for the
"--cached" mode and it does not take a pathspec.
Also as the series progresses, I would expect this to be updated in
patches $n/4 (2 < $n).
> + if (recurse_submodules && argc)
> + die("ls-files --recurse-submodules does not support path "
> + "arguments");
Hmm, s/path arguments/pathspec/, perhaps, as the latter is specified
in the glossary?
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-09-28 23:14 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-12-larsxschneider@gmail.com>
Part third (and last) of the review of v8 11/11.
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com napisał:
[...]
> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
> new file mode 100755
> index 0000000..c13a631
> --- /dev/null
> +++ b/contrib/long-running-filter/example.pl
[...]
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index dc50938..210c4f6 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
One thing that could have been done as yet another preparatory
patch would be to modernize existing t/t0021-conversion.sh tests.
For example use here-doc instead of series of echo-s, use cp
to copy files and not echo, etc.
> @@ -31,7 +31,10 @@ test_expect_success setup '
> cat test >test.i &&
> git add test test.t test.i &&
> rm -f test test.t test.i &&
> - git checkout -- test test.t test.i
> + git checkout -- test test.t test.i &&
> +
> + echo "content-test2" >test2.o &&
> + echo "content-test3 - subdir" >"test3 - subdir.o"
I see that you prepare here a few uncommitted files, but both
their names and their contents leave much to be desired - you
don't know from the name and contents what they are for.
And the '"subdir"' file which is not in subdirectory is
especially egregious.
> '
>
> script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
> @@ -279,4 +282,364 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '
> test_line_count = 0 count
> '
>
A small comment on parameters of this function would be nice.
> +check_filter () {
> + rm -f rot13-filter.log actual.log &&
> + "$@" 2> git_stderr.log &&
> + test_must_be_empty git_stderr.log &&
> + cat >expected.log &&
This is too clever by half. Having a function that both tests
the behavior and prepares 'expected' file is too much.
In my opinion preparation of 'expected.log' file should be moved
to another function or functions.
Also, if we are running sort on output, I think we should also
run sort on 'expected.log', so that what we write doesn't need to
be created sorted (so we don't have to sort expected lines by hand).
Or maybe we should run the same transformation on rot13-filter.log
and on the contents of expected.log.
> + sort rot13-filter.log | uniq -c | sed "s/^[ ]*//" >actual.log &&
> + test_cmp expected.log actual.log
> +}
> +
> +check_filter_count_clean () {
> + rm -f rot13-filter.log actual.log &&
> + "$@" 2> git_stderr.log &&
> + test_must_be_empty git_stderr.log &&
All those functions (well, wait?) have common setup, which we can
extract into separate shell function, I think. IMVHO.
> + cat >expected.log &&
> + sort rot13-filter.log | uniq -c | sed "s/^[ ]*//" |
> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" >actual.log &&
> + test_cmp expected.log actual.log
> +}
> +
> +check_filter_ignore_clean () {
> + rm -f rot13-filter.log actual.log &&
> + "$@" &&
Why we don't check for stderr here?
> + cat >expected.log &&
> + grep -v "IN: clean" rot13-filter.log >actual.log &&
> + test_cmp expected.log actual.log
> +}
> +
> +check_filter_no_call () {
> + rm -f rot13-filter.log &&
> + "$@" 2> git_stderr.log &&
> + test_must_be_empty git_stderr.log &&
> + test_must_be_empty rot13-filter.log
> +}
> +
A small comment on parameters of this function would be nice.
And a comment what it does.
> +check_rot13 () {
> + test_cmp "$1" "$2" &&
> + ./../rot13.sh <"$1" >expected &&
Why there is .. in this invocation?
> + git cat-file blob :"$2" >actual &&
> + test_cmp expected actual
> +}
> +
> +test_expect_success PERL 'required process filter should filter data' '
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + test_config_global filter.protocol.required true &&
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
Don't you think that creating a fresh test repository for each
separate test is a bit too much? I guess that you want for
each and every test to be completely independent, but this setup
and teardown is a bit excessive.
Other tests in the same file (should we reuse the test, or use
new test file) do not use this method.
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> + git add . &&
> + git commit . -m "test commit" &&
> + git branch empty &&
Err... I think it would be better to name it 'empty-branch'
(or 'almost-empty-branch', as it does include .gitattributes file).
See my mistake below (marked <del>...</del>).
> +
> + cp ../test.o test.r &&
> + cp ../test2.o test2.r &&
What does this test2.o / test2.r file tests, that test.o / test.r
doesn't? The name doesn't tell us.
Why it is test.r, but test2.r? Why it isn't test1.r?
> + mkdir testsubdir &&
> + cp "../test3 - subdir.o" "testsubdir/test3 - subdir.r" &&
Why it needs to have different contents?
I guess that you test two things here: file in a subdirectory,
and file with spaces in names. Shouldn't it be better split
into two separate test files?
> + >test4-empty.r &&
You test ordinary file, file in subdirectory, file with filename
containing spaces, and an empty file.
Other tests of single file `clean`/`smudge` filters use filename
that requires mangling; maybe we should use similar file?
special="name with '\''sq'\'' and \$x" &&
echo some test text >"$special" &&
In case of `process` filter, a special filename could look like
this:
process_special="name=with equals and\nembedded newlines\n" &&
echo some test text >"$process_special" &&
> +
> + check_filter \
> + git add . \
I assume that this kind of test is here also to check that
we are not regressing / backsliding, and we do not start to
run "clean" operation more than once per file for "git add",
isn't it?
> + <<-\EOF &&
> + 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
> + 1 IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
> + 1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> + 1 IN: clean testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
> + 1 START
> + 1 STOP
> + 1 wrote filter header
> + EOF
First, this indentation level confirms that the check_filter
function is too clever by half, and that preparing expected.log
file should be a separate step.
Second, if we run "sort" on contents to be in expected.log, we
can write it in more natural, and less fragile way:
+ sort >expected.log <<-\EOF &&
+ 1 START
+ 1 wrote filter header
+ 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
+ 1 IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
+ 1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ 1 IN: clean testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
+ 1 STOP
+ EOF
Third, why the filter even writes output size? It is no longer
part of `process` filter driver protocol, and it makes test more
fragile.
If we are to keep sizes, then to make test less fragile with
respect to changes in contents of tested files, we should use
variables containing file size:
test_r_size=$(wc -c test.r)
...
sort >expected.log <<-EOF &&
...
1 IN: clean test.r $test_r_size [OK] -- OUT: $test_r_size . [OK]
> +
> + check_filter_count_clean \
> + git commit . -m "test commit" \
I guess that you use "git commit ." (not very visible this '.')
in order to force cleaning of all files, isn't it?
Use of *_count_clean function is here, from what I remember,
because 'git commit .' sometimes call `clean` multiple times
for the same file (?), and sometimes it calls `smudge` (probably
as part of some optimization?).
I guess that fixing "git commit" so that calls clean operation
at most once per file is left for a separate patch series; this
one is long enough and involved enough as it is.
> + <<-\EOF &&
> + x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
> + x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
> + x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> + x IN: clean testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
> + 1 START
> + 1 STOP
> + 1 wrote filter header
> + EOF
> +
> + rm -f test?.r "testsubdir/test3 - subdir.r" &&
Why 'test?.r' when we are removing only 'test2.r'; why not be explicit?
> +
> + check_filter_ignore_clean \
> + git checkout . \
> + <<-\EOF &&
> + START
> + wrote filter header
> + IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
> + IN: smudge testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
Ah, I see that there are no shenningans for `clean`
operation, calling op multiple time for single file.
> + STOP
> + EOF
> +
> + check_filter_ignore_clean \
> + git checkout empty \
<del>
First, isn't it test4-empty.r? Trying to check out non-existent
file should not run filter, isn't it? How the heck this passed???
There is no branch 'empty'.
Second, the one-shot filter tests have empty-in-worktree and
empty-in-repo files; why not reuse them?
</del>
My mistake, but the branch is named a bit strange.
> + <<-\EOF &&
> + START
> + wrote filter header
> + STOP
> + EOF
Why is even filter process invoked? If this is not expected, perhaps
simply ignore what checking out almost empty branch (one without any
files marked for filtering) does.
Shouldn't we test_expect_failure no-call?
> +
> + check_filter_ignore_clean \
> + git checkout master \
Does this checks different code path than 'git checkout .'? For
example, does this test increase code coverage (e.g. as measured
by gcov)? If not, then this test could be safely dropped.
> + <<-\EOF &&
> + START
> + wrote filter header
> + IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
> + IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
> + IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
> + IN: smudge testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
Can we assume that Git would pass files to filter in alphabetical
order? This assumption might make the test unnecessary fragile.
> + STOP
> + EOF
> +
> + check_rot13 ../test.o test.r &&
> + check_rot13 ../test2.o test2.r &&
> + check_rot13 "../test3 - subdir.o" "testsubdir/test3 - subdir.r"
All right.
> + )
> +'
> +
> +test_expect_success PERL 'required process filter should clean only and take precedence' '
Trying to describe it better results in overly long description,
which probably means that this test should be split into few
smaller ones:
- `process` filter takes precedence over `clean` and/or `smudge`
filters, regardless if it supports relevant ("clean" or "smudge")
capability or not
- `process` filter that includes only "clean" capability should
clean only (be used only for 'clean' operation)
- required process filter should do something (???)
> + test_config_global filter.protocol.clean ./../rot13.sh &&
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
> + test_config_global filter.protocol.required true &&
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> + git add . &&
> + git commit . -m "test commit" &&
> + git branch empty &&
> +
> + cp ../test.o test.r &&
> +
> + check_filter \
> + git add . \
> + <<-\EOF &&
> + 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
> + 1 START
> + 1 STOP
> + 1 wrote filter header
> + EOF
> +
> + check_filter_count_clean \
> + git commit . -m "test commit" \
Is this part really necessary? I think it duplicates what we
have tested earlier, and would not catch any new errors. Removing
spurious/redundant tests results in faster testsuite, which is
quite important.
> + <<-\EOF
> + x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
> + 1 START
> + 1 STOP
> + 1 wrote filter header
> + EOF
And this test checks only the first one from the list.
Well, actually the first part, without "regardless if it
supports relevant ('clean' [...]) capability or not".
> + )
> +'
> +
In my opinion all functions should be placed at beginning,
or even in separate file (if they are used in more than
one test).
> +generate_test_data () {
The name is not good, it doesn't describe what kind of data
we want to generate.
> + LEN=$1
> + NAME=$2
> + test-genrandom end $LEN |
Why do you use 'end' as <seed_string> parameter to test-genrandom?
> + perl -pe "s/./chr((ord($&) % 26) + 97)/sge" >../$NAME.file &&
Those constants (26 and 97) are a bit cryptic; magical constants.
I guess this is
+ perl -pe "s/./chr((ord($&) % (ord('z') - ord('a') + 1) + ord('a'))/sge" >../$NAME.file &&
or
+ perl -pe "s/./chr((ord($&) % 26 + ord('a'))/sge" >../$NAME.file &&
That is, convert to a-z range (why not ASCII printable characters,
that is characters from ' ' / chr(32) to '~' / chr(126), which is
95 characters instead of 26?)
I guess this is so we can be sure that rot13 filter would work
(note: the filter is defined for A-Za-z, not only a-z, never
the mind pass-through for other characters).
> + cp ../$NAME.file . &&
Do we re-generate this file each time?
> + ./../rot13.sh <../$NAME.file >../$NAME.file.rot13
Anyway, I wonder if taking the last two lines out of the function
(as they are not about _generating_ a file) would make it more
readable or not.
> +}
> +
> +test_expect_success PERL 'required process filter should process multiple packets' '
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + test_config_global filter.protocol.required true &&
> +
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + # Generate data that requires 3 packets
> + PKTLINE_DATA_MAXLEN=65516 &&
Shouldn't this be set once per whole test?
> +
> + generate_test_data $(($PKTLINE_DATA_MAXLEN )) 1pkt_1__ &&
> + generate_test_data $(($PKTLINE_DATA_MAXLEN + 1)) 2pkt_1+1 &&
> + generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 - 1)) 2pkt_2-1 &&
> + generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 )) 2pkt_2__ &&
> + generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 + 1)) 3pkt_2+1 &&
Looks good to me.
> +
> + echo "*.file filter=protocol" >.gitattributes &&
> + check_filter \
> + git add *.file .gitattributes \
Should it be shell expansion, or git expansion, that is
git add '*.file' .gitattributes
> + <<-\EOF &&
> + 1 IN: clean 1pkt_1__.file 65516 [OK] -- OUT: 65516 . [OK]
> + 1 IN: clean 2pkt_1+1.file 65517 [OK] -- OUT: 65517 .. [OK]
> + 1 IN: clean 2pkt_2-1.file 131031 [OK] -- OUT: 131031 .. [OK]
> + 1 IN: clean 2pkt_2__.file 131032 [OK] -- OUT: 131032 .. [OK]
> + 1 IN: clean 3pkt_2+1.file 131033 [OK] -- OUT: 131033 ... [OK]
I think it would be better for those sizes to be calculated,
not entered by hand. Though in this case this doesn't matter
much - it would always be this size.
> + 1 START
> + 1 STOP
> + 1 wrote filter header
> + EOF
> + git commit . -m "test commit" &&
Is this needed / necessary?
> +
> + rm -f *.file &&
> + git checkout -- *.file &&
Is this necessary? I guess this checks that it doesn't crash, but
we do not check that smudge operation works correctly, as we did
for clean.
> +
> + for f in *.file
> + do
> + git cat-file blob :$f >actual &&
> + test_cmp ../$f.rot13 actual
> + done
Wasn't there helper function for this?
> + )
> +'
> +
> +test_expect_success PERL 'required process filter should with clean error should fail' '
^^^^^^ ^^^^^^
Errr... what? You have 'should' twice here.
Also, does it matter that the error is during clean operation?
We don't test that error during smudge operation is handled in
the same way, do we?
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
Do we need to pass 'clean smudge', or does it provide both by
default?
> + test_config_global filter.protocol.required true &&
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> +
> + cp ../test.o test.r &&
> + echo "this is going to fail" >clean-write-fail.r &&
> + echo "content-test3-subdir" >test3.r &&
> +
> + # Note: There are three clean paths in convert.c we just test one here.
What does this comment is about? What 'three clean paths'?
> + test_must_fail git add .
> + )
> +'
> +
> +test_expect_success PERL 'process filter should restart after unexpected write failure' '
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> +
> + cp ../test.o test.r &&
> + cp ../test2.o test2.r &&
Note that the preparation step is almost the same, and we
repeat it over, and over, and over (no shell function for
this; and we always do full setup / teardown).
> + echo "this is going to fail" >smudge-write-fail.o &&
> + cat smudge-write-fail.o >smudge-write-fail.r &&
This cat is cp.
> + git add . &&
> + git commit . -m "test commit" &&
You don't need to commit for 'git checkout <path>' (e.g. for .)
or 'git cat-file -p :<file>' to work.
> + rm -f *.r &&
> +
> + check_filter_ignore_clean \
> + git checkout . \
> + <<-\EOF &&
> + START
> + wrote filter header
> + IN: smudge smudge-write-fail.r 22 [OK] -- OUT: 22 [WRITE FAIL]
> + START
> + wrote filter header
> + IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
> + IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
> + STOP
> + EOF
> +
> + check_rot13 ../test.o test.r &&
> + check_rot13 ../test2.o test2.r &&
Looks good.
> +
> + ! test_cmp smudge-write-fail.o smudge-write-fail.r && # Smudge failed!
> + ./../rot13.sh <smudge-write-fail.o >expected &&
> + git cat-file blob :smudge-write-fail.r >actual &&
> + test_cmp expected actual # Clean worked!
This is almost negation of check_rot13 - perhaps a helper function
would help here (check_not_rot13?).
Also, what this comment is about, and why so far to the right?
> + )
> +'
> +
> +test_expect_success PERL 'process filter should not restart in case of an error' '
Errr... what? This description is not clear. Did you mean
that filter should not be restarted if it *signals* an error
with file (either before sending anything, or after sending
partial contents)?
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> +
> + cp ../test.o test.r &&
> + cp ../test2.o test2.r &&
> + echo "this will cause an error" >error.o &&
> + cp error.o error.r &&
And here you (correctly) use cp, and not cat.
> + git add . &&
> + git commit . -m "test commit" &&
> + rm -f *.r &&
> +
> + check_filter_ignore_clean \
> + git checkout . \
> + <<-\EOF &&
> + START
> + wrote filter header
> + IN: smudge error.r 25 [OK] -- OUT: 0 [ERROR]
> + IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
> + IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
> + STOP
> + EOF
> +
> + check_rot13 ../test.o test.r &&
> + check_rot13 ../test2.o test2.r &&
> + test_cmp error.o error.r
Looks good to me.
> + )
> +'
> +
> +test_expect_success PERL 'process filter should be able to signal an error for all future files' '
Did you mean here that filter can abort processing of
all future files?
> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> +
> + cp ../test.o test.r &&
> + cp ../test2.o test2.r &&
> + echo "error this blob and all future blobs" >abort.o &&
> + cp abort.o abort.r &&
> + git add . &&
> + git commit . -m "test commit" &&
> + rm -f *.r &&
> +
> + check_filter_ignore_clean \
> + git checkout . \
> + <<-\EOF &&
> + START
> + wrote filter header
> + IN: smudge abort.r 37 [OK] -- OUT: 0 [ABORT]
> + STOP
How can we know that 'abort' file is processed first?
Though more resilent solution would be harder to create...
> + EOF
> +
> + test_cmp ../test.o test.r &&
> + test_cmp ../test2.o test2.r &&
> + test_cmp abort.o abort.r
> + )
> +'
> +
> +test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
> + test_config_global filter.protocol.process cat &&
We could use rot13.sh, that is one-shot filter here.
> + test_config_global filter.protocol.required true &&
All right, filter is required to easier distinguish it not
working from not filtered.
> + rm -rf repo &&
> + mkdir repo &&
> + (
> + cd repo &&
> + git init &&
> +
> + echo "*.r filter=protocol" >.gitattributes &&
> +
> + cp ../test.o test.r &&
> + test_must_fail git add . 2> git_stderr.log &&
> + grep "not support long running filter protocol" git_stderr.log
Shouldn't this use gettext poison (or rather C locale)?
This error message could be translated in the future.
> + )
> +'
> +
> test_done
I wonder how does the code coverage for the new v2 filter
code looks like...
Anyway, I think it would be good idea to write at the beginning
of new tests (be they in old test, or in new test) what we want
to test:
- that 'clean' and 'smudge' operations are invoked, for all
possible combinations (covering all code paths), and that
filter is invoked only once
- that special types of files work:
* empty file (in worktree, in index, in repo)
* file in subdirectory
* filename with special characters
* large file (test marked as EXPENSIVE), multiple maximum
packet size
* perhaps binary file?
- that 'process' overrides old-style 'clean' and 'smudge'
filters, regardless of the former capabilities
- that limiting capabilities works
- that requiring filter works correctly (doubles number of
tests, at least for a subset of them)
- that filter is restarted if it fails on non-required,
fails git command if required
- that filter can error out out of filtering a file,
upfront and after partial contents, without restart;
fails git command if required
- that filter can abort,
fails git command if required (?)
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> new file mode 100755
> index 0000000..8958f71
> --- /dev/null
> +++ b/t/t0021/rot13-filter.pl
> @@ -0,0 +1,191 @@
> +#!/usr/bin/perl
> +#
> +# Example implementation for the Git filter protocol version 2
> +# See Documentation/gitattributes.txt, section "Filter Protocol"
> +#
> +# The script takes the list of supported protocol capabilities as
> +# arguments ("clean", "smudge", etc).
> +#
> +# This implementation supports special test cases:
> +# (1) If data with the pathname "clean-write-fail.r" is processed with
> +# a "clean" operation then the write operation will die.
> +# (2) If data with the pathname "smudge-write-fail.r" is processed with
> +# a "smudge" operation then the write operation will die.
> +# (3) If data with the pathname "error.r" is processed with any
> +# operation then the filter signals that it cannot or does not want
> +# to process the file.
> +# (4) If data with the pathname "abort.r" is processed with any
> +# operation then the filter signals that it cannot or does not want
> +# to process the file and any file after that is processed with the
> +# same command.
Nice to have this description.
BTW. why write-fail is per operation (clean or smudge), but error and abort
is not?
> +#
> +
> +use strict;
> +use warnings;
I guess there is some duplication with the code in contrib, isn't it?
> +
> +my $MAX_PACKET_CONTENT_SIZE = 65516;
> +my @capabilities = @ARGV;
> +
> +open my $debug, ">>", "rot13-filter.log";
or die "cannot open file for appending: $!";
Good, three argument open. Bad (?), not error handling.
> +
> +sub rot13 {
> + my ($str) = @_;
^^^^
Why 4 spaces, and not TAB character?
I think
my $str = shift;
is more idiomatic Perl.
> + $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
Why not use tr/// version of this quote-like operation?
Or do you follow prior art here?
> + return $str;
> +}
> +
> +sub packet_bin_read {
> + my $buffer;
> + my $bytes_read = read STDIN, $buffer, 4;
> + if ( $bytes_read == 0 ) {
> +
> + # EOF - Git stopped talking to us!
> + print $debug "STOP\n";
> + exit();
> + }
> + elsif ( $bytes_read != 4 ) {
> + die "invalid packet size '$bytes_read' field";
Errr, $bytes_read is not packet size field. It is $buffer.
Also, error message looks strange
invalid packet size '004' field
Shouldn't it be at end?
> + }
> + my $pkt_size = hex($buffer);
$pkt_size greater than $MAX_PACKET_CONTENT_SIZE is also an error,
as is sizes 1-3 (not that it matters much, at least here).
> + if ( $pkt_size == 0 ) {
> + return ( 1, "" );
It feels a bit strange to me to return list instead of hashref,
but this is a matter of opinion.
> + }
> + elsif ( $pkt_size > 4 ) {
> + my $content_size = $pkt_size - 4;
> + $bytes_read = read STDIN, $buffer, $content_size;
> + if ( $bytes_read != $content_size ) {
> + die "invalid packet ($content_size expected; $bytes_read read)";
It would read, strangely
"invalid packet (8 expected, 7 read)"
The "size" or "bytes" is missing from this output.
> + }
> + return ( 0, $buffer );
> + }
> + else {
> + die "invalid packet size";
Is keep-alive packet valid ("0004")?
> + }
> +}
> +
> +sub packet_txt_read {
> + my ( $res, $buf ) = packet_bin_read();
> + unless ( $buf =~ /\n$/ ) {
> + die "A non-binary line SHOULD BE terminated by an LF.";
First, if SHOULD BE, then perhaps 'warn' not 'die'... though for
tests it is probably better to 'die'.
Second, we should probably print (a fragment of) this line.
> + }
> + return ( $res, substr( $buf, 0, -1 ) );
Same comment as for example file in contrib/ - use s/// and no
need for substr stuff.
> +}
> +
> +sub packet_bin_write {
> + my ($packet) = @_;
> + print STDOUT sprintf( "%04x", length($packet) + 4 );
> + print STDOUT $packet;
> + STDOUT->flush();
> +}
> +
> +sub packet_txt_write {
> + packet_bin_write( $_[0] . "\n" );
> +}
> +
> +sub packet_flush {
> + print STDOUT sprintf( "%04x", 0 );
> + STDOUT->flush();
> +}
Looks good to me (though same comments as to contrib/ file applies).
> +
> +print $debug "START\n";
> +$debug->flush();
> +
> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
> +( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
> +( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
> +
> +packet_txt_write("git-filter-server");
> +packet_txt_write("version=2");
> +
> +( packet_txt_read() eq ( 0, "clean=true" ) ) || die "bad capability";
> +( packet_txt_read() eq ( 0, "smudge=true" ) ) || die "bad capability";
> +( packet_bin_read() eq ( 1, "" ) ) || die "bad capability end";
> +
> +foreach (@capabilities) {
> + packet_txt_write( $_ . "=true" );
> +}
> +packet_flush();
> +print $debug "wrote filter header\n";
Or perhaps "handshake end"?
> +$debug->flush();
> +
> +while (1) {
> + my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
> + print $debug "IN: $command";
> + $debug->flush();
> +
> + my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
All right, here list context is necessary.
> + print $debug " $pathname";
No " pathname=$pathname" ?
> + $debug->flush();
> +
> + # Flush
> + packet_bin_read();
Same comment as earlier: read_flush, or read_varlist (till flush)
to have would be better.
> +
> + my $input = "";
> + {
> + binmode(STDIN);
> + my $buffer;
> + my $done = 0;
> + while ( !$done ) {
> + ( $done, $buffer ) = packet_bin_read();
> + $input .= $buffer;
> + }
> + print $debug " " . length($input) . " [OK] -- ";
> + $debug->flush();
> + }
> +
> + my $output;
> + if ( $pathname eq "error.r" or $pathname eq "abort.r" ) {
> + $output = "";
> + }
> + elsif ( $command eq "clean" and grep( /^clean$/, @capabilities ) ) {
> + $output = rot13($input);
> + }
> + elsif ( $command eq "smudge" and grep( /^smudge$/, @capabilities ) ) {
> + $output = rot13($input);
> + }
> + else {
> + die "bad command '$command'";
> + }
> +
> + print $debug "OUT: " . length($output) . " ";
Shouldn't we write the length of output only if we don't error out,
abort, or fail?
> + $debug->flush();
> +
> + if ( $pathname eq "error.r" ) {
> + print $debug "[ERROR]\n";
> + $debug->flush();
> + packet_txt_write("status=error");
> + packet_flush();
> + }
> + elsif ( $pathname eq "abort.r" ) {
> + print $debug "[ABORT]\n";
> + $debug->flush();
> + packet_txt_write("status=abort");
> + packet_flush();
> + }
Looks good, so this is upfront status=error or status-abort.
> + else {
> + packet_txt_write("status=success");
> + packet_flush();
> +
> + if ( $pathname eq "${command}-write-fail.r" ) {
> + print $debug "[WRITE FAIL]\n";
> + $debug->flush();
> + die "${command} write error";
> + }
> +
> + while ( length($output) > 0 ) {
> + my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
> + packet_bin_write($packet);
> + print $debug ".";
All right, so number of dots is the number of packets. This is
surprisingly opaque.
> + if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
> + $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
> + }
> + else {
> + $output = "";
> + }
> + }
> + packet_flush();
> + print $debug " [OK]\n";
> + $debug->flush();
> + packet_flush();
Should we test partial contents case? Or failure during printing?
What happens then - is file cleared by Git, or left partially converted?
> + }
> +}
>
Keep up good work. Looks quite good.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v8 01/11] pkt-line: rename packet_write() to packet_write_fmt()
From: Jakub Narębski @ 2016-09-28 23:15 UTC (permalink / raw)
To: Lars Schneider
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <13E00B0F-41FC-4FBE-9ABD-2E9B11743E2B@gmail.com>
W dniu 26.09.2016 o 20:49, Lars Schneider pisze:
> On 24 Sep 2016, at 23:14, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>>
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> packet_write() should be called packet_write_fmt() as the string
>>> parameter can be formatted.
>>
>> I would say:
>>
>> packet_write() should be called packet_write_fmt() because it
>> is printf-like function where first parameter is format string.
>>
>> Or something like that. But such minor change might be not worth
>> yet another reroll of this patch series.
>>
>> Perhaps it would be a good idea to explain the reasoning behind
>> this change:
>>
>> This is important distinction to know from the name if the
>> function accepts arbitrary binary data and/or arbitrary
>> strings to be written - packet_write[_fmt()] do not.
>
> packet_write() should be called packet_write_fmt() because it is a
> printf-like function that takes a format string as first parameter.
>
> packet_write_fmt() should be used for text strings only. Arbitrary
> binary data should use a new packet_write() function that is introduced
> in a subsequent patch.
>
> Better?
Better.
>
>>> pkt-line.h | 2 +-
>>> shallow.c | 2 +-
>>> upload-pack.c | 30 +++++++++++++++---------------
>>> 11 files changed, 29 insertions(+), 29 deletions(-)
>>
>> Diffstat looks correct. Was the patch generated by doing search
>> and replace?
>
> Yes.
Good.
--
Jakub Narębski
^ 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