* [RFC] test-lib: detect common misuse of test_expect_failure
From: Junio C Hamano @ 2016-10-14 22:38 UTC (permalink / raw)
To: git
It is a very easy mistake to make to say test_expect_failure when
making sure a step in the test fails, which must be spelled
"test_must_fail". By introducing a toggle $test_in_progress that is
turned on at the beginning of test_start_() and off at the end of
test_finish_() helper, we can detect this fairly easily.
Strictly speaking, writing "test_expect_success" inside another
test_expect_success (or inside test_expect_failure for that matter)
can be detected with the same mechanism if we really wanted to, but
that is a lot less likely confusion, so let's not bother.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* It is somewhat embarrassing to admit that I had to stare at the
offending code for more than 5 minutes to notice what went wrong
to come up with <xmqqr37iy5bw.fsf@gitster.mtv.corp.google.com>;
if we had something like this, it would have helped.
t/test-lib-functions.sh | 4 ++++
t/test-lib.sh | 3 +++
2 files changed, 7 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..fc8c10a061 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -381,6 +381,10 @@ test_verify_prereq () {
}
test_expect_failure () {
+ if test "$test_in_progress" = 1
+ then
+ error "bug in the test script: did you mean test_must_fail instead of test_expect_failure?"
+ fi
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..4c360216e5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -344,6 +344,7 @@ test_count=0
test_fixed=0
test_broken=0
test_success=0
+test_in_progress=0
test_external_has_tap=0
@@ -625,6 +626,7 @@ test_run_ () {
}
test_start_ () {
+ test_in_progress=1
test_count=$(($test_count+1))
maybe_setup_verbose
maybe_setup_valgrind
@@ -634,6 +636,7 @@ test_finish_ () {
echo >&3 ""
maybe_teardown_valgrind
maybe_teardown_verbose
+ test_in_progress=0
}
test_skip () {
^ permalink raw reply related
* Re: [RFC] test-lib: detect common misuse of test_expect_failure
From: Jeff King @ 2016-10-14 23:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqk2day2ry.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 14, 2016 at 03:38:41PM -0700, Junio C Hamano wrote:
> It is a very easy mistake to make to say test_expect_failure when
> making sure a step in the test fails, which must be spelled
> "test_must_fail". By introducing a toggle $test_in_progress that is
> turned on at the beginning of test_start_() and off at the end of
> test_finish_() helper, we can detect this fairly easily.
>
> Strictly speaking, writing "test_expect_success" inside another
> test_expect_success (or inside test_expect_failure for that matter)
> can be detected with the same mechanism if we really wanted to, but
> that is a lot less likely confusion, so let's not bother.
I like the general idea, but I'm not sure how this would interact with
the tests in t0000 that test the test suite. It looks like that always
happens in a full sub-shell invocation (via run_sub_test_lib_test), so
we're OK as long as test_in_progress is not exported (and obviously the
subshell cannot accidentally overwrite our variable with a 0 when it is
finished).
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index fdaeb3a96b..fc8c10a061 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -381,6 +381,10 @@ test_verify_prereq () {
> }
>
> test_expect_failure () {
> + if test "$test_in_progress" = 1
> + then
> + error "bug in the test script: did you mean test_must_fail instead of test_expect_failure?"
> + fi
This follows existing practice for things like the &&-lint-checker, and
bails out on the whole test script. That sometimes makes it hard to find
the problematic test, especially if you're running via something like
"prove", because it doesn't make valid TAP output.
It might be nicer if we just said "this test is malformed, and therefore
fails", and then you get all the usual niceties for recording and
finding the failed test.
I don't think it would be robust enough to try to propagate the error up
to the outer test_expect_success block (and anyway, you'd also want to
know about it in a test_expect_failure block; it's a bug in the test,
not a known breakage). But perhaps error() could dump some TAP-like
output with a "virtual" failed test.
Something like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bd..dc6b1f5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -299,9 +299,8 @@ TERM=dumb
export TERM
error () {
- say_color error "error: $*"
- GIT_EXIT_OK=t
- exit 1
+ test_failure_ "$@"
+ test_done
}
say () {
@@ -600,7 +599,7 @@ test_run_ () {
# code of other programs
test_eval_ "(exit 117) && $1"
if test "$?" != 117; then
- error "bug in the test script: broken &&-chain: $1"
+ error "bug in the test script: broken &&-chain" "$1"
fi
trace=$trace_tmp
fi
which lets "make prove" collect the broken test number.
It would perhaps need to cover the case when $test_count is "0"
separately. I dunno. It would be nicer still if we could continue
running other tests in the script, but I think it's impossible to
robustly jump back to the outer script.
These kinds of "bug in the test suite" are presumably rare enough that
the niceties don't matter that much, but I trigger the &&-checker
reasonably frequently (that and test_line_count, because I can never
remember the correct invocation).
Anyway. That's all orthogonal to your patch. I just wondered if we could
do better, but AFAICT the right way to do better is to hook into
error(), which means your patch would not have to care exactly how it
fails.
-Peff
^ permalink raw reply related
* Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+
From: Ævar Arnfjörð Bjarmason @ 2016-10-15 8:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narębski, Git Mailing List
In-Reply-To: <xmqq37jyzsdk.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 14, 2016 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).
>>>
>>> I think it's fairly dubious to link to things matching [0-9a-fA-F]
>>> here as opposed to just [0-9a-f], that dates back to the initial
>>> version of gitweb from 161332a ("first working version",
>>> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
>>> them as far as I can tell.
>>
>> All right. If we decide to be more strict in what we accept, we can
>> do it in a separate commit.
>>
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>
>> Acked-by: Jakub Narębski <jnareb@gmail.com>
>
> Thanks for a review. As the topic is not yet in 'next', I'll squish
> in your Acked-by: to them. I saw them only for 1 & 2/3; would
> another for 3/3 be coming soon?
As far as I can tell the only outstanding "change this" is your
s/SHA1/SHA-1/ in <xmqq37k9jm86.fsf@gitster.mtv.corp.google.com>, do
you want to fix that up or should I submit another series?
>>
>>> ---
>>> gitweb/gitweb.perl | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index cba7405..92b5e91 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2036,7 +2036,7 @@ sub format_log_line_html {
>>> my $line = shift;
>>>
>>> $line = esc_html($line, -nbsp=>1);
>>> - $line =~ s{\b([0-9a-fA-F]{8,40})\b}{
>>> + $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>>
>> By the way, it is quite long commit message for one character change.
>> Not that it is a bad thing...
>>
>>> $cgi->a({-href => href(action=>"object", hash=>$1),
>>> -class => "text"}, $1);
>>> }eg;
>>>
^ permalink raw reply
* [PATCH] cocci: avoid self-references in object_id transformations
From: René Scharfe @ 2016-10-15 8:25 UTC (permalink / raw)
To: brian m. carlson; +Cc: Git List, Junio C Hamano
The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure
that the Coccinelle transformations for converting legacy function calls
are not applied to these wrappers themselves, which would result in
tautological declarations.
We can remove the added guards once the object_id functions stand on
their own.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/object_id.cocci | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
index 0307624..09afdbf 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -17,10 +17,13 @@ expression E1;
+ oid_to_hex(&E1)
@@
+identifier f != oid_to_hex;
expression E1;
@@
+ f(...) {...
- sha1_to_hex(E1->hash)
+ oid_to_hex(E1)
+ ...}
@@
expression E1, E2;
@@ -29,10 +32,13 @@ expression E1, E2;
+ oid_to_hex_r(E1, &E2)
@@
+identifier f != oid_to_hex_r;
expression E1, E2;
@@
+ f(...) {...
- sha1_to_hex_r(E1, E2->hash)
+ oid_to_hex_r(E1, E2)
+ ...}
@@
expression E1;
@@ -41,10 +47,13 @@ expression E1;
+ oidclr(&E1)
@@
+identifier f != oidclr;
expression E1;
@@
+ f(...) {...
- hashclr(E1->hash)
+ oidclr(E1)
+ ...}
@@
expression E1, E2;
@@ -53,10 +62,13 @@ expression E1, E2;
+ oidcmp(&E1, &E2)
@@
+identifier f != oidcmp;
expression E1, E2;
@@
+ f(...) {...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
+ ...}
@@
expression E1, E2;
@@ -77,10 +89,13 @@ expression E1, E2;
+ oidcpy(&E1, &E2)
@@
+identifier f != oidcpy;
expression E1, E2;
@@
+ f(...) {...
- hashcpy(E1->hash, E2->hash)
+ oidcpy(E1, E2)
+ ...}
@@
expression E1, E2;
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v15 14/27] t6030: no cleanup with bad merge base
From: Pranit Bauva @ 2016-10-15 8:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqr37iy5bw.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Sat, Oct 15, 2016 at 3:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> +test_expect_success 'check whether bisection cleanup is not done with bad merges' '
>> + git bisect start $HASH7 $SIDE_HASH7 &&
>> + test_expect_failure git bisect bad >out 2>out &&
>
> I think you meant "test_must_fail" here.
Oh yes! Thanks for pointing it out. I see that you have already
submitted a patch to catch this mistake which was previously ignored.
Thanks!
Regards,
Pranit Bauva
^ permalink raw reply
* [PATCH] t0040: convert all possible tests to use `test-parse-options --expect`
From: Pranit Bauva @ 2016-10-15 12:28 UTC (permalink / raw)
To: git
Use "test-parse-options --expect" to rewrite the tests to avoid checking
the whole variable dump by just testing what is required. This commit is
based on 8ca65aeb (t0040: convert a few tests to use test-parse-options;
Junio C Hamano; May 6, 2016).
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t0040-parse-options.sh | 183 ++++-------------------------------------------
1 file changed, 13 insertions(+), 170 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index db5f60d..74d2cd7 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -208,32 +208,15 @@ test_expect_success 'unambiguously abbreviated option' '
'
test_expect_success 'unambiguously abbreviated option with "="' '
- test-parse-options --int=2 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="integer: 2" --int=2
'
test_expect_success 'ambiguously abbreviated option' '
test_expect_code 129 test-parse-options --strin 123
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: 123
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'non ambiguous option (after two options it abbreviates)' '
- test-parse-options --st 123 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="string: 123" --st 123
'
cat >typo.err <<\EOF
@@ -256,24 +239,8 @@ test_expect_success 'detect possible typos' '
test_cmp typo.err output.err
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-arg 00: --quux
-EOF
-
test_expect_success 'keep some options as arguments' '
- test-parse-options --quux >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="arg 00: --quux" --quux
'
cat >expect <<\EOF
@@ -350,54 +317,20 @@ test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '
test_cmp expect output
'
-cat >expect <<\EOF
-boolean: 6
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'OPT_BIT() works' '
- test-parse-options -bb --or4 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="boolean: 6" -bb --or4
'
test_expect_success 'OPT_NEGBIT() works' '
- test-parse-options -bb --no-neg-or4 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="boolean: 6" -bb --no-neg-or4
'
test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
- test-parse-options + + + + + + >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="boolean: 6" + + + + + +
'
-cat >expect <<\EOF
-boolean: 0
-integer: 12345
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'OPT_NUMBER_CALLBACK() works' '
- test-parse-options -12345 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="integer: 12345" -12345
'
cat >expect <<\EOF
@@ -435,118 +368,28 @@ test_expect_success '--no-list resets list' '
test_cmp expect output
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 3
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'multiple quiet levels' '
- test-parse-options -q -q -q >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="quiet: 3" -q -q -q
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: 3
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'multiple verbose levels' '
- test-parse-options -v -v -v >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="verbose: 3" -v -v -v
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-quiet sets --quiet to 0' '
- test-parse-options --no-quiet >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="quiet: 0" --no-quiet
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-quiet resets multiple -q to 0' '
- test-parse-options -q -q -q --no-quiet >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="quiet: 0" -q -q -q --no-quiet
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: 0
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-verbose sets verbose to 0' '
- test-parse-options --no-verbose >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="verbose: 0" --no-verbose
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: 0
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-verbose resets multiple verbose to 0' '
- test-parse-options -v -v -v --no-verbose >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="verbose: 0" -v -v -v --no-verbose
'
test_done
--
https://github.com/git/git/pull/299
^ permalink raw reply related
* Re: [PATCH] cocci: avoid self-references in object_id transformations
From: brian m. carlson @ 2016-10-15 13:45 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <a5ed26c0-7fea-259c-74c1-0cd870a35290@web.de>
[-- Attachment #1: Type: text/plain, Size: 762 bytes --]
On Sat, Oct 15, 2016 at 10:25:34AM +0200, René Scharfe wrote:
> The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
> oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
> sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure
> that the Coccinelle transformations for converting legacy function calls
> are not applied to these wrappers themselves, which would result in
> tautological declarations.
Ah, yes, this is a good idea. I've had to hack around this, but this is
much better than having to fix it up by hand.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-10-15 14:45 UTC (permalink / raw)
To: Jakub Narębski, Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <37c12539-3edd-e04b-6e09-e977a854661c@gmail.com>
@Peff: If you have time, it would be great if you could comment on
one question below prefixed with "@Peff". Thanks!
> On 12 Oct 2016, at 03:54, Jakub Narębski <jnareb@gmail.com> wrote:
>
> W dniu 12.10.2016 o 00:26, Lars Schneider pisze:
>>> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>>>>
>>
>>>> After the filter started
>>>> Git sends a welcome message ("git-filter-client"), a list of
>>>> supported protocol version numbers, and a flush packet. Git expects
>>>> +to read a welcome response message ("git-filter-server") and exactly
>>>> +one protocol version number from the previously sent list. All further
>>>> +communication will be based on the selected version. The remaining
>>>> +protocol description below documents "version=2". Please note that
>>>> +"version=42" in the example below does not exist and is only there
>>>> +to illustrate how the protocol would look like with more than one
>>>> +version.
>>>> +
>>>> +After the version negotiation Git sends a list of all capabilities that
>>>> +it supports and a flush packet. Git expects to read a list of desired
>>>> +capabilities, which must be a subset of the supported capabilities list,
>>>> +and a flush packet as response:
>>>> +------------------------
>>>> +packet: git> git-filter-client
>>>> +packet: git> version=2
>>>> +packet: git> version=42
>>>> +packet: git> 0000
>>>> +packet: git< git-filter-server
>>>> +packet: git< version=2
>>>> +packet: git> clean=true
>>>> +packet: git> smudge=true
>>>> +packet: git> not-yet-invented=true
>>>> +packet: git> 0000
>>>> +packet: git< clean=true
>>>> +packet: git< smudge=true
>>>> +packet: git< 0000
>>>
>>> WARNING: This example is different from description!!!
>>
>> Can you try to explain the difference more clearly? I read it multiple
>> times and I think this is sound.
>
> I'm sorry it was *my mistake*. I have read the example exchange wrong.
>
> On the other hand that means that I have other comment, which I though
> was addressed already in v10, namely that not all exchanges ends with
> flush packet (inconsistency, and I think a bit of lack of extendability).
Well, this part of the protocol is not supposed to be extensible because
it is supposed to deal *only* with the version number. It needs to keep
the same structure to ensure forward and backward compatibility.
However, for consistency sake I will add a flush packet.
>>> In description above the example you have 4-part handshake, not 3-part;
>>> the filter is described to send list of supported capabilities last
>>> (a subset of what Git command supports).
>>
>> Part 1: Git sends a welcome message...
>> Part 2: Git expects to read a welcome response message...
>> Part 3: After the version negotiation Git sends a list of all capabilities...
>> Part 4: Git expects to read a list of desired capabilities...
>>
>> I think example and text match, no?
>
> Yes, it does; as I have said already, I have misread the example.
>
> Anyway, in some cases 4-way handshake, where Git sends list of
> supported capabilities first, is better. If the protocol has
> to prepare something for each of capabilities, and perhaps check
> those preparation status, it can do it after Git sends what it
> could need, and before it sends what it does support.
>
> Though it looks a bit strange that client (as Git is client here)
> sends its capabilities first...
Git tells the filter what it can do. Then the filter decides what
features it supports. I would prefer to keep it that way as I don't
see a strong advantage for the other way around.
>>> By the way, now I look at it, the argument for using the
>>> "<capability>=true" format instead of "capability=<capability>"
>>> (or "supported-command=<capability>") is weak. The argument for
>>> using "<variable>=<value>" to make it easier to implement parsing
>>> is sound, but the argument for "<capability>=true" is weak.
>>>
>>> The argument was that with "<capability>=true" one can simply
>>> parse metadata into hash / dictionary / hashmap, and choose
>>> response based on that. Hash / hashmap / associative array
>>> needs different keys, so the reasoning went for "<capability>=true"
>>> over "capability=<capability>"... but the filter process still
>>> needs to handle lines with repeating keys, namely "version=<N>"
>>> lines!
>>>
>>> So the argument doesn't hold water IMVHO, and we can choose
>>> version which reads better / is more natural.
>>
>> I have to agree that "capability=<capability>" might read a
>> little bit nicer. However, Peff suggested "<capability>=true"
>> as his preference and this is absolutely OK with me.
>
> From what I remember it was Peff stating that he thinks "<foo>=true"
> is easier for parsing (it is, but we still need to support the harder
> way parsing anyway), and offered that "<foo>" is good enough (if less
> consistent).
>
>> I am happy to change that if a second reviewer shares your
>> opinion.
>
> Also, with "capability=<foo>" we can be more self descriptive,
> for example "supported-command=<foo>"; though "capability" is good
> enough for me.
>
> For example
>
> packet: git> wants=clean
> packet: git> wants=smudge
> packet: git> wants=size
> packet: git> 0000
> packet: git< supports=clean
> packet: git< supports=smudge
> packet: git< 0000
>
> Though coming up with good names is hard; and as I said "capability"
> is good enough; OTOH with "smudge=true" etc. we don't need to come
> up with good name at all... though I wonder if it is a good thing `\_o,_/
How about this (I borrowed these terms from contract negotiation)?
packet: git> offers=clean
packet: git> offers=smudge
packet: git> offers=size
packet: git> 0000
packet: git< accepts=clean
packet: git< accepts=smudge
packet: git< 0000
@Peff: Would that be OK for you?
>>>> +------------------------
>>>> +packet: git< status=abort
>>>> +packet: git< 0000
>>>> +------------------------
>>>> +
>>>> +After the filter has processed a blob it is expected to wait for
>>>> +the next "key=value" list containing a command. Git will close
>>>> +the command pipe on exit. The filter is expected to detect EOF
>>>> +and exit gracefully on its own.
>>>
>>> Any "kill filter" solutions should probably be put here.
>>
>> Agreed.
>>
>>> I guess
>>> that filter exiting means EOF on its standard output when read
>>> by Git command, isn't it?
>>
>> Yes, but at this point Git is not listening anymore.
>
> I think it might be good idea to have here the information about
> what filter process should do if it needs maybe lengthy closing
> process, to not hold/stop Git command or to not be killed.
I've added:
"Git will wait until the filter process has stopped."
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v10 14/14] contrib/long-running-filter: add long running filter example
From: Lars Schneider @ 2016-10-15 14:47 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git, gitster, jnareb, peff
In-Reply-To: <3bdfebab-aae9-3263-218e-c8ae394084fc@web.de>
> On 08 Oct 2016, at 22:42, Torsten Bögershausen <tboegi@web.de> wrote:
>
> On 08.10.16 13:25, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Add a simple pass-thru filter as example implementation for the Git
>> filter protocol version 2. See Documentation/gitattributes.txt, section
>> "Filter Protocol" for more info.
>>
>
> Nothing wrong with code in contrib.
> I may have missed parts of the discussion, was there a good reason to
> drop the test case completely?
>
>> When adding a new feature, make sure that you have new tests to show
>> the feature triggers the new behavior when it should, and to show the
>> feature does not trigger when it shouldn't. After any code change, make
>> sure that the entire test suite passes.
>
> Or is there a plan to add them later ?
The test is part of the "main feature patch" 13/14:
http://public-inbox.org/git/20161008112530.15506-14-larsxschneider@gmail.com/
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v10 04/14] run-command: add clean_on_exit_handler
From: Lars Schneider @ 2016-10-15 15:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, jnareb, peff
In-Reply-To: <alpine.DEB.2.20.1610111407080.3492@virtualbox>
> On 11 Oct 2016, at 05:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>
> Hi Lars,
>
> On Sat, 8 Oct 2016, larsxschneider@gmail.com wrote:
>
>> @@ -31,6 +32,15 @@ static void cleanup_children(int sig, int in_signal)
>> while (children_to_clean) {
>> struct child_to_clean *p = children_to_clean;
>> children_to_clean = p->next;
>> +
>> + if (p->process && !in_signal) {
>> + struct child_process *process = p->process;
>> + if (process->clean_on_exit_handler) {
>> + trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
>
> On Windows, pid_t translates to long long int, resulting in this build
> error:
>
> -- snip --
> In file included from cache.h:10:0,
> from run-command.c:1:
> run-command.c: In function 'cleanup_children':
> run-command.c:39:18: error: format '%d' expects argument of type 'int', but argument 5 has type 'pid_t {aka long long int}' [-Werror=format=]
> trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
> ^
> trace.h:81:53: note: in definition of macro 'trace_printf'
> trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
> ^~~~~~~~~~~
> cc1.exe: all warnings being treated as errors
> make: *** [Makefile:1987: run-command.o] Error 1
> -- snap --
>
> Maybe use PRIuMAX as we do elsewhere (see output of `git grep
> printf.*pid`):
>
> trace_printf("trace: run_command: running exit handler for pid %"
> PRIuMAX, (uintmax_t)p->pid);
Thanks for hint! I'll change it!
However, I am building on Win 8.1 with your latest SDK and I cannot
reproduce the error. Any idea why that might be the case?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH] convert: mark a file-local symbol static
From: Lars Schneider @ 2016-10-15 15:05 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <b21c8a92-4dd5-56d6-ec6a-5709028eaf5f@ramsayjones.plus.com>
> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>
> If you need to re-roll your 'ls/filter-process' branch, could you
> please squash this into commit 85290197
> ("convert: add filter.<driver>.process option", 08-10-2016).
>
>
> -void stop_multi_file_filter(struct child_process *process)
> +static void stop_multi_file_filter(struct child_process *process)
Done! Do you have some kind of script to detect these things
automatically or do you read the code that carefully?
Thanks,
Lars
^ permalink raw reply
* [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: René Scharfe @ 2016-10-15 16:23 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
Calculating offsets involving a NULL pointer is undefined. It works in
practice (for now?), but we should not rely on it. Allocate first and
then simply refer to the flexible array member by its name instead of
performing pointer arithmetic up front. The resulting code is slightly
shorter, easier to read and doesn't rely on undefined behaviour.
NB: The cast to a (non-const) void pointer is necessary to keep support
for flexible array members declared as const.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
This patch allows the test suite to largely pass (t7063 still fails)
for clang 3.8 with -fsanitize=undefined and -DNO_UNALIGNED_LOADS.
git-compat-util.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718da..f964e36 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -851,8 +851,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
* times, and it must be assignable as an lvalue.
*/
#define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
- (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
- (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
+ size_t flex_array_len_ = (len); \
+ (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
+ memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
} while (0)
#define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Torsten Bögershausen @ 2016-10-15 17:03 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Jakub Narębski, Johannes Sixt
In-Reply-To: <4e73ba3e8c1700259ffcc3224d1f66e6a760142d.1476120229.git.johannes.schindelin@gmx.de>
Not sure is this has been reported before:
sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare]
if (command < ARRAY_SIZE(todo_command_strings))
~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
53f8024e (Johannes Schindelin 2016-10-10 19:25:07 +0200 633) if (command < ARRAY_SIZE(todo_command_strings))
^ permalink raw reply
* Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: Jeff King @ 2016-10-15 17:13 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <ccb15072-d949-fc84-ee45-45ba013f53c4@web.de>
On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:
> Calculating offsets involving a NULL pointer is undefined. It works in
> practice (for now?), but we should not rely on it. Allocate first and
> then simply refer to the flexible array member by its name instead of
> performing pointer arithmetic up front. The resulting code is slightly
> shorter, easier to read and doesn't rely on undefined behaviour.
Yeah, this NULL computation is pretty nasty. I recall trying to get rid
of it, but I think it is impossible to do so portably while still using
the generic xalloc_flex() helper. I'm not sure why I didn't think of
just inlining it as you do here. It's not that many lines of code, and I
I agree the result is conceptually simpler.
> #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
> - (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
> - (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
> + size_t flex_array_len_ = (len); \
> + (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
> + memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
This looks correct. I wondered at first why you bothered with
flex_array_len, but it is to avoid evaluating the "len" parameter
multiple times.
> } while (0)
> #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
> (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
Now that xalloc_flex() has only this one caller remaining, perhaps it
should just be inlined here, too, for simplicity.
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Jeff King @ 2016-10-15 17:19 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Johannes Schindelin, git, Jakub Narębski, Johannes Sixt
In-Reply-To: <933b13d6-5f24-c03a-a1a0-712ceb8ddcc8@web.de>
On Sat, Oct 15, 2016 at 07:03:46PM +0200, Torsten Bögershausen wrote:
> sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare]
> if (command < ARRAY_SIZE(todo_command_strings))
> ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
> 53f8024e (Johannes Schindelin 2016-10-10 19:25:07 +0200 633) if (command < ARRAY_SIZE(todo_command_strings))
>
Interesting. The compiler is right that this _should_ never happen, but
I think the patch is quite reasonable to be defensive in case the enum
happens to get a value outside of its acceptable range (which is
probably undefined behavior, but...).
I wonder if:
if ((int)command < ARRAY_SIZE(todo_command_strings))
silences the warning (I suppose size_t is probably an even better type,
though obviously it does not matter in practice).
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Torsten Bögershausen @ 2016-10-15 17:40 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Jakub Narębski, Johannes Sixt
In-Reply-To: <20161015171926.qgtvrjcaqwb436hx@sigill.intra.peff.net>
On 15.10.16 19:19, Jeff King wrote:
> On Sat, Oct 15, 2016 at 07:03:46PM +0200, Torsten Bögershausen wrote:
>
>> sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare]
>> if (command < ARRAY_SIZE(todo_command_strings))
>> ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 warning generated.
>>
>> 53f8024e (Johannes Schindelin 2016-10-10 19:25:07 +0200 633) if (command < ARRAY_SIZE(todo_command_strings))
>>
>
> Interesting. The compiler is right that this _should_ never happen, but
> I think the patch is quite reasonable to be defensive in case the enum
> happens to get a value outside of its acceptable range (which is
> probably undefined behavior, but...).
>
> I wonder if:
>
> if ((int)command < ARRAY_SIZE(todo_command_strings))
>
> silences the warning (I suppose size_t is probably an even better type,
> though obviously it does not matter in practice).
>
> -Peff
>
Both do (silence the warning)
enum may be signed or unsigned, right ?
So the size_t variant seams to be a better choice
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Jeff King @ 2016-10-15 17:41 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jakub Narębski, git, Junio C Hamano
In-Reply-To: <3732E902-2FE9-4C99-B27D-69B9A3FF8639@gmail.com>
On Sat, Oct 15, 2016 at 07:45:48AM -0700, Lars Schneider wrote:
> >> I have to agree that "capability=<capability>" might read a
> >> little bit nicer. However, Peff suggested "<capability>=true"
> >> as his preference and this is absolutely OK with me.
> >
> > From what I remember it was Peff stating that he thinks "<foo>=true"
> > is easier for parsing (it is, but we still need to support the harder
> > way parsing anyway), and offered that "<foo>" is good enough (if less
> > consistent).
I don't mind that much if you want to do it the other way. You are the
one writing the parsing/use code.
> > Also, with "capability=<foo>" we can be more self descriptive,
> > for example "supported-command=<foo>"; though "capability" is good
> > enough for me.
> >
> > For example
> >
> > packet: git> wants=clean
> > packet: git> wants=smudge
> > packet: git> wants=size
> > packet: git> 0000
> > packet: git< supports=clean
> > packet: git< supports=smudge
> > packet: git< 0000
> >
> > Though coming up with good names is hard; and as I said "capability"
> > is good enough; OTOH with "smudge=true" etc. we don't need to come
> > up with good name at all... though I wonder if it is a good thing `\_o,_/
>
> How about this (I borrowed these terms from contract negotiation)?
>
> packet: git> offers=clean
> packet: git> offers=smudge
> packet: git> offers=size
> packet: git> 0000
> packet: git< accepts=clean
> packet: git< accepts=smudge
> packet: git< 0000
>
> @Peff: Would that be OK for you?
Is it always an offers/accepts relationship? Can the response say "you
did not ask about <foo>, but just so you know I support it"?
I cannot think offhand of an example, but at the same time, if you leave
the terms as generic as possible, you do not end up later with words
that do not make sense. It is trading off one problem now (vagueness of
the protocol terms) for a potential one later (words that have a
specific meaning, but one that is not accurate).
I don't have a strong preference, though.
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Jeff King @ 2016-10-15 17:46 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Johannes Schindelin, git, Jakub Narębski, Johannes Sixt
In-Reply-To: <d9f4f658-94fb-cb9e-7da8-3a2fac120a9e@web.de>
On Sat, Oct 15, 2016 at 07:40:15PM +0200, Torsten Bögershausen wrote:
> > I wonder if:
> >
> > if ((int)command < ARRAY_SIZE(todo_command_strings))
> >
> > silences the warning (I suppose size_t is probably an even better type,
> > though obviously it does not matter in practice).
> >
> Both do (silence the warning)
>
> enum may be signed or unsigned, right ?
> So the size_t variant seams to be a better choice
Good catch. It technically needs to check the lower bound, too. In
theory, if somebody wanted to add an enum value that is negative, you'd
use a signed cast and check against both 0 and ARRAY_SIZE(). In
practice, that is nonsense for this case, and using an unsigned type
means that any negative values become large, and the check catches them.
-Peff
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-15 19:42 UTC (permalink / raw)
To: Lars Schneider, Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <3732E902-2FE9-4C99-B27D-69B9A3FF8639@gmail.com>
W dniu 15.10.2016 o 16:45, Lars Schneider pisze:
>> On 12 Oct 2016, at 03:54, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 12.10.2016 o 00:26, Lars Schneider pisze:
>>>> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>>>>>
>>>
>>>>> After the filter started
>>>>> Git sends a welcome message ("git-filter-client"), a list of
>>>>> supported protocol version numbers, and a flush packet. Git expects
>>>>> +to read a welcome response message ("git-filter-server") and exactly
>>>>> +one protocol version number from the previously sent list. All further
>>>>> +communication will be based on the selected version. The remaining
>>>>> +protocol description below documents "version=2". Please note that
>>>>> +"version=42" in the example below does not exist and is only there
>>>>> +to illustrate how the protocol would look like with more than one
>>>>> +version.
>>>>> +
>>>>> +After the version negotiation Git sends a list of all capabilities that
>>>>> +it supports and a flush packet. Git expects to read a list of desired
>>>>> +capabilities, which must be a subset of the supported capabilities list,
>>>>> +and a flush packet as response:
>>>>> +------------------------
>>>>> +packet: git> git-filter-client
>>>>> +packet: git> version=2
>>>>> +packet: git> version=42
>>>>> +packet: git> 0000
>>>>> +packet: git< git-filter-server
>>>>> +packet: git< version=2
>>>>> +packet: git> clean=true
>>>>> +packet: git> smudge=true
>>>>> +packet: git> not-yet-invented=true
>>>>> +packet: git> 0000
>>>>> +packet: git< clean=true
>>>>> +packet: git< smudge=true
>>>>> +packet: git< 0000
>>>>
>>>> WARNING: This example is different from description!!!
>>>
>>> Can you try to explain the difference more clearly? I read it multiple
>>> times and I think this is sound.
>>
>> I'm sorry it was *my mistake*. I have read the example exchange wrong.
>>
>> On the other hand that means that I have other comment, which I though
>> was addressed already in v10, namely that not all exchanges ends with
>> flush packet (inconsistency, and I think a bit of lack of extendability).
>
> Well, this part of the protocol is not supposed to be extensible because
> it is supposed to deal *only* with the version number. It needs to keep
> the same structure to ensure forward and backward compatibility.
>
> However, for consistency sake I will add a flush packet.
Thanks. That is one thing I feel quite strongly about.
I can agree that extendability does not matter much here: we can always
change the version number. But there might be some additional information
that filter process wants to send to Git in first exchange, and using
flush-terminated list here means that we don't need to change version
number, assuming that this additional information is advisory.
The consistency means in my opinion that it should be easier to implement
filter scripts.
>>>> In description above the example you have 4-part handshake, not 3-part;
>>>> the filter is described to send list of supported capabilities last
>>>> (a subset of what Git command supports).
>>>
>>> Part 1: Git sends a welcome message...
>>> Part 2: Git expects to read a welcome response message...
>>> Part 3: After the version negotiation Git sends a list of all capabilities...
>>> Part 4: Git expects to read a list of desired capabilities...
>>>
>>> I think example and text match, no?
>>
>> Yes, it does; as I have said already, I have misread the example.
>>
>> Anyway, in some cases 4-way handshake, where Git sends list of
>> supported capabilities first, is better. If the protocol has
>> to prepare something for each of capabilities, and perhaps check
>> those preparation status, it can do it after Git sends what it
>> could need, and before it sends what it does support.
>>
>> Though it looks a bit strange that client (as Git is client here)
>> sends its capabilities first...
>
> Git tells the filter what it can do. Then the filter decides what
> features it supports. I would prefer to keep it that way as I don't
> see a strong advantage for the other way around.
I think the current order is good, no need to change it.
As I said it is better for Git to send capabilities first.
>>>> By the way, now I look at it, the argument for using the
>>>> "<capability>=true" format instead of "capability=<capability>"
>>>> (or "supported-command=<capability>") is weak. The argument for
>>>> using "<variable>=<value>" to make it easier to implement parsing
>>>> is sound, but the argument for "<capability>=true" is weak.
>>>>
>>>> The argument was that with "<capability>=true" one can simply
>>>> parse metadata into hash / dictionary / hashmap, and choose
>>>> response based on that. Hash / hashmap / associative array
>>>> needs different keys, so the reasoning went for "<capability>=true"
>>>> over "capability=<capability>"... but the filter process still
>>>> needs to handle lines with repeating keys, namely "version=<N>"
>>>> lines!
>>>>
>>>> So the argument doesn't hold water IMVHO, and we can choose
>>>> version which reads better / is more natural.
>>>
>>> I have to agree that "capability=<capability>" might read a
>>> little bit nicer. However, Peff suggested "<capability>=true"
>>> as his preference and this is absolutely OK with me.
>>
>> From what I remember it was Peff stating that he thinks "<foo>=true"
>> is easier for parsing (it is, but we still need to support the harder
>> way parsing anyway), and offered that "<foo>" is good enough (if less
>> consistent).
>>
>>> I am happy to change that if a second reviewer shares your
>>> opinion.
>>
>> Also, with "capability=<foo>" we can be more self descriptive,
>> for example "supported-command=<foo>"; though "capability" is good
>> enough for me.
>>
>> For example
>>
>> packet: git> wants=clean
>> packet: git> wants=smudge
>> packet: git> wants=size
>> packet: git> 0000
>> packet: git< supports=clean
>> packet: git< supports=smudge
>> packet: git< 0000
>>
>> Though coming up with good names is hard; and as I said "capability"
>> is good enough; OTOH with "smudge=true" etc. we don't need to come
>> up with good name at all... though I wonder if it is a good thing `\_o,_/
>
> How about this (I borrowed these terms from contract negotiation)?
>
> packet: git> offers=clean
> packet: git> offers=smudge
> packet: git> offers=size
> packet: git> 0000
> packet: git< accepts=clean
> packet: git< accepts=smudge
> packet: git< 0000
>
> @Peff: Would that be OK for you?
I don't feel strongly about it. It could be "<capability>=true", it could
be "<capability>", it could be "capability=<capability>", it could be
something more descriptive.
I guess "<capability>=true" looks a bit strange (would it ever be there
"<capability>=false"?), but it is good enough for me.
One think we can all agree on is that each capability is to be send as
separate packets, and not as space or comma separated list in a single
packet (like for fetch / push).
>>>>> +------------------------
>>>>> +packet: git< status=abort
>>>>> +packet: git< 0000
>>>>> +------------------------
>>>>> +
>>>>> +After the filter has processed a blob it is expected to wait for
>>>>> +the next "key=value" list containing a command. Git will close
>>>>> +the command pipe on exit. The filter is expected to detect EOF
>>>>> +and exit gracefully on its own.
>>>>
>>>> Any "kill filter" solutions should probably be put here.
>>>
>>> Agreed.
>>>
>>>> I guess
>>>> that filter exiting means EOF on its standard output when read
>>>> by Git command, isn't it?
>>>
>>> Yes, but at this point Git is not listening anymore.
>>
>> I think it might be good idea to have here the information about
>> what filter process should do if it needs maybe lengthy closing
>> process, to not hold/stop Git command or to not be killed.
>
> I've added:
>
> "Git will wait until the filter process has stopped."
Thanks. Looks good for me.
I think any advices (like how to handle shutdown in filter without
blocking Git) could be added later, when we have some experience
making and using long-running multi-file filter drivers.
Thank you for your work on this series.
--
Jakub Narębski
^ permalink raw reply
* [PATCH] trailer: mark file-local symbol 'conf_head' static
From: Ramsay Jones @ 2016-10-15 20:57 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Jonathan,
If you need to re-roll your 'jt/trailer-with-cruft' branch, could you
please squash this into the relevant patch. [commit 3fb120de ("trailer:
use list.h for doubly-linked list", 14-10-2016)]
Thanks!
ATB,
Ramsay Jones
trailer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/trailer.c b/trailer.c
index 1be6ec8..2862a48 100644
--- a/trailer.c
+++ b/trailer.c
@@ -42,7 +42,7 @@ struct arg_item {
struct conf_info conf;
};
-LIST_HEAD(conf_head);
+static LIST_HEAD(conf_head);
static char *separators = ":";
--
2.10.0
^ permalink raw reply related
* Re: [PATCH] convert: mark a file-local symbol static
From: Ramsay Jones @ 2016-10-15 21:01 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <A430A9E0-B2A2-4857-8DEA-EBD7AA2C9E29@gmail.com>
On 15/10/16 16:05, Lars Schneider wrote:
>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
[snip]
>> -void stop_multi_file_filter(struct child_process *process)
>> +static void stop_multi_file_filter(struct child_process *process)
>
> Done! Do you have some kind of script to detect these things
> automatically or do you read the code that carefully?
Heh, I'm _far_ too lazy to read the code that carefully. :-D
A combination of 'make sparse' and a perl script (originally
posted to the list by Junio) find all of these for me.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] convert: mark a file-local symbol static
From: Lars Schneider @ 2016-10-16 0:15 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <de24ed05-2857-9c17-920f-66770f898f80@ramsayjones.plus.com>
> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>
>
>
> On 15/10/16 16:05, Lars Schneider wrote:
>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> [snip]
>>> -void stop_multi_file_filter(struct child_process *process)
>>> +static void stop_multi_file_filter(struct child_process *process)
>>
>> Done! Do you have some kind of script to detect these things
>> automatically or do you read the code that carefully?
>
> Heh, I'm _far_ too lazy to read the code that carefully. :-D
>
> A combination of 'make sparse' and a perl script (originally
> posted to the list by Junio) find all of these for me.
Interesting. Do you have a link to this script?
Does it generate false positives?
Maybe I can add `make sparse` to the TravisCI build?
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v10 04/14] run-command: add clean_on_exit_handler
From: Johannes Schindelin @ 2016-10-16 8:03 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff
In-Reply-To: <9C686FFB-CB07-445E-B812-97781CAB113D@gmail.com>
Hi Lars,
On Sat, 15 Oct 2016, Lars Schneider wrote:
>
> > On 11 Oct 2016, at 05:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> >
> > Hi Lars,
> >
> > On Sat, 8 Oct 2016, larsxschneider@gmail.com wrote:
> >
> >> @@ -31,6 +32,15 @@ static void cleanup_children(int sig, int in_signal)
> >> while (children_to_clean) {
> >> struct child_to_clean *p = children_to_clean;
> >> children_to_clean = p->next;
> >> +
> >> + if (p->process && !in_signal) {
> >> + struct child_process *process = p->process;
> >> + if (process->clean_on_exit_handler) {
> >> + trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
> >
> > On Windows, pid_t translates to long long int, resulting in this build
> > error:
> >
> > -- snip --
> > In file included from cache.h:10:0,
> > from run-command.c:1:
> > run-command.c: In function 'cleanup_children':
> > run-command.c:39:18: error: format '%d' expects argument of type 'int', but argument 5 has type 'pid_t {aka long long int}' [-Werror=format=]
> > trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
> > ^
> > trace.h:81:53: note: in definition of macro 'trace_printf'
> > trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
> > ^~~~~~~~~~~
> > cc1.exe: all warnings being treated as errors
> > make: *** [Makefile:1987: run-command.o] Error 1
> > -- snap --
> >
> > Maybe use PRIuMAX as we do elsewhere (see output of `git grep
> > printf.*pid`):
> >
> > trace_printf("trace: run_command: running exit handler for pid %"
> > PRIuMAX, (uintmax_t)p->pid);
>
> Thanks for hint! I'll change it!
>
> However, I am building on Win 8.1 with your latest SDK and I cannot
> reproduce the error. Any idea why that might be the case?
Are you building with DEVELOPER=1?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-16 8:09 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, git, Jakub Narębski,
Johannes Sixt
In-Reply-To: <20161015174656.fmgk3le2b34nnjpx@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]
Hi,
On Sat, 15 Oct 2016, Jeff King wrote:
> On Sat, Oct 15, 2016 at 07:40:15PM +0200, Torsten Bögershausen wrote:
>
> > > I wonder if:
> > >
> > > if ((int)command < ARRAY_SIZE(todo_command_strings))
> > >
> > > silences the warning (I suppose size_t is probably an even better type,
> > > though obviously it does not matter in practice).
> > >
> > Both do (silence the warning)
> >
> > enum may be signed or unsigned, right ?
> > So the size_t variant seams to be a better choice
>
> Good catch. It technically needs to check the lower bound, too. In
> theory, if somebody wanted to add an enum value that is negative, you'd
> use a signed cast and check against both 0 and ARRAY_SIZE(). In
> practice, that is nonsense for this case, and using an unsigned type
> means that any negative values become large, and the check catches them.
I am pretty certain that I disagree with that warning: enums have been
used as equivalents of ints for a long time, and will be, for a long time
to come.
Given that this test is modified to `if (command < TODO_NOOP)` later, I
hope that you agree that it is not worth the trouble to appease that
compiler overreaction?
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Johannes Schindelin @ 2016-10-16 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqqzim6zzc7.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 14 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> >> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
> >> >> (merged to 'next' on 2016-10-11 at e37425ed17)
> >> >> + submodule: ignore trailing slash in relative url
> >> >> + submodule: ignore trailing slash on superproject URL
> >> >>
> >> >> A minor regression fix for "git submodule".
> >> >>
> >> >> Will merge to 'master'.
> >> >
> >> > Going by the bug report, this *may* be more than
> >> > minor and worth merging down to maint as well, eventually.
> >>
> >> The topic was forked at a reasonably old commit so that it can be
> >> merged as far down to maint-2.9 if we wanted to. Which means the
> >> regression was fairly old and fix is not all that urgent as well.
> >
> > And if you merge it to `master` and `maint`,...
>
> I'll mark it as "wait for follow-up fix" in whats-cooking.txt (on
> 'todo' branch) to remind myself not to merge it yet.
May I request your guidance as to your preference how to proceed? Here is
the problem: Stefan's fix uncovered a bug in the way Git for Windows' Bash
hands off "Unix" paths to real Windows programs, such as `git
submodule-helper`: trailing `/.` gets truncated to `/`.
That is, when passing `/c/test/.` to the helper, what arrives is
actually `C:/test/`. So the helper, being expected to cut off trailing
slashes, cuts it off because it does not see the dot.
Note: when passing URLs (`https://repos.com/mine/.`) or when passing
Windows paths (`C:/test/.`), the helper *will* see the dot, and all is
fine.
Unfortunately the behavior of the MSYS2 Bash cannot be altered in that
respect because existing build workflows *depend* on the current behavior.
Please note we did not have that problem previously, as the helper was a
shell script and therefore stayed in the POSIX-emulated realm, no
POSIX->Windows path translation necessary.
Now, we have two tests in t0060 that try to catch changes in this
particular scenario, verifying that only the dots are cut off of paths
like `/a/b/c/.`.
Given that we cannot modify MSYS2 to appease those tests (because we would
break many more important things), and given that you can easily work
around this by using paths native to Windows when using Git interactively
(which the tests cannot do because they target Linux), I am inclined to
change the tests.
Here are the options I see:
A) remove the tests in question
B) mark them as !MINGW instead
C) change just those two tests from using `$PWD` (pseudo-Unix path) to
`$(pwd)` (native path)
I would like to hear your feedback about your preference, but not without
priming you a little bit by detailing my current opinion on the matter:
While I think B) would be the easiest to read, C) would document the
expected behavior better. A) would feel to me like shrugging, i.e. the
lazy, wrong thing to do.
What do you think?
Dscho
^ 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