* Re: [PATCH] t0021: expect more variations in the output of uniq -c
From: Johannes Sixt @ 2016-11-06 21:40 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <0B7DE8F1-34DF-4271-8936-B9B596EF5F9E@gmail.com>
Am 06.11.2016 um 16:31 schrieb Lars Schneider:
> This looks good to me. I wonder if I should post a patch
> to add the "|| return" trick to the following function
> "test_cmp_exclude_clean", too?!
The function does this:
for FILE in "$expect" "$actual"
do
grep -v "IN: clean" "$FILE" >"$FILE.tmp" &&
mv "$FILE.tmp" "$FILE"
done &&
It is an interesting case: If only matching lines are in the file,
nothing would be printed, and grep returns failure. But we shouldn't
care, because, strictly speaking, the function only wants to remove
lines with a certain pattern. Here, it actually hurts to &&-chain grep!
It wouldn't hurt to write 'mv ... || return', but at the same time, &&
after grep should be removed.
-- Hannes
^ permalink raw reply
* Re: gitk: avoid obscene memory consumption
From: Jacob Keller @ 2016-11-06 20:33 UTC (permalink / raw)
To: Markus Hitter; +Cc: Paul Mackerras, Stefan Beller, git@vger.kernel.org
In-Reply-To: <ff5bb36b-e30c-3998-100d-789b4b5e7249@jump-ing.de>
On Sun, Nov 6, 2016 at 2:28 AM, Markus Hitter <mah@jump-ing.de> wrote:
> - Storing only the actually viewed diff. It's an interactive tool, so there's no advantage in displaying the diff in 0.001 seconds over viewing it in 0.1 seconds. As far as I can see, Gitk currently stores every diff it gets a hold of forever.
>
This seems like the right solution. Store only what we need to view as
we need to view it. (IE: lazily generate the diff and don't keep it
long term, possibly by generating each file separately when that file
is viewed)?
> - View the diff sparsely. Like rendering only the actually visible portion.
>
This also would be valuable, as part of the solution above.
> - Enhancing ctext. This reference diff has 28 million characters, so there should be a way to store this with color information in, let's say, 29 MB of memory.
>
I think all three suggestions here are a better solution that what
you've outlined already as you explain the problems caused by cutting
off the diff.
Thanks,
Jake
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jacob Keller @ 2016-11-06 20:17 UTC (permalink / raw)
To: Josh Triplett
Cc: Junio C Hamano, Christian Couder, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <20161106173311.lqoxxgcklx4jlrg7@x>
On Sun, Nov 6, 2016 at 9:33 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Sun, Nov 06, 2016 at 09:14:56AM -0800, Junio C Hamano wrote:
>> Josh Triplett <josh@joshtriplett.org> writes:
>> > We could, but if we (or one of the many third-party git implementations)
>> > miss a case, gitlinks+reachability may appear to work in many cases with
>> > dataloss afterward, while gitrefs will fail early and not appear
>> > functional.
>>
>> I wonder what happens if we do not introduce the "gitref" but
>> instead change the behaviour of "gitlink" to imply an optional
>> reachability. That is, when enumerating what is reachable in your
>> repository, if you see a gitlink and if you notice that you locally
>> have the target of that gitlink, you follow, but if you know you
>> lack it, you do not error out. This may be making things too
>> complex to feasibily implement by simplify them ;-) and I see a few
>> immediate fallout that needs to be thought through (i.e. downsides)
>> and a few upsides, too. I am feeling feverish and not thinking
>> straight, so I won't try to weigh pros-and-cons.
>>
>> This would definitely need protocol extension when transferring
>> objects across repositories.
>
> It'd also need a repository format extension locally. Otherwise, if you
> ever touched that repository with an older git (or a tool built on an
> older libgit2 or JGit or other library), you could lose data.
>
> It does seem conceptually appealing, though. In an ideal world, the
> original version of gitlink would have had opt-out reachability (and
> .gitmodules with an external repository reference could count as opting
> out).
>
> But I can't think of any case where it's OK for a git implementation to
> not know about this reachability extension and still operate on the
> gitlink. And given that, it might as well use a new object type that
> the old version definitely won't think it understands.
>
> - Josh Triplett
That's still only true if the receiving end runs fsck, isn't it? I
suppose that's a large number of receivers, and at least there are
ways post-push to determine that objects don't make sense to that
version of git.
I think using a new mode is the safest way, and it allows easily
implementing RefTrees as well as other projects. Additionally, if we
*wanted* additional "opt-in / opt-out" support we could add this by
default to gitrefs,and they could (possibly) replace gitlinks in the
future?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
From: Lars Schneider @ 2016-11-06 19:35 UTC (permalink / raw)
To: Jeff King; +Cc: git, tboegi, gitster
In-Reply-To: <20161017095002.bbqyp2hkbuyau66t@sigill.intra.peff.net>
> On 17 Oct 2016, at 11:50, Jeff King <peff@peff.net> wrote:
>
> On Sun, Oct 16, 2016 at 05:25:49PM -0700, larsxschneider@gmail.com wrote:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Apple removed the OpenSSL header files in macOS 10.11 and above. OpenSSL
>> was deprecated since macOS 10.7.
>>
>> Set `NO_OPENSSL` and `APPLE_COMMON_CRYPTO` to `YesPlease` as default for
>> macOS. Make it possible to override this and use OpenSSL by defining
>> `DARWIN_OPENSSL`.
>
> I like that you gave an override, but I don't think it works in all
> cases:
>
>> diff --git a/config.mak.uname b/config.mak.uname
>> index b232908..f0c94a9 100644
>> --- a/config.mak.uname
>> +++ b/config.mak.uname
>> @@ -108,6 +108,12 @@ ifeq ($(uname_S),Darwin)
>> ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1),1)
>> HAVE_GETDELIM = YesPlease
>> endif
>> + ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 15 && echo 1),1)
>> + ifndef DARWIN_OPENSSL
>> + NO_OPENSSL = YesPlease
>> + APPLE_COMMON_CRYPTO=YesPlease
>> + endif
>> + endif
>
> This is in config.mak.uname, which gets sourced before config.mak (and
> ifndef is evaluated at the time of parsing). So it would work to do:
>
> make DARWIN_OPENSSL=Yep
>
> but not:
>
> echo DARWIN_OPENSSL=Yep >>config.mak
> make
>
> I think you'd have to set a flag in config.mak.uname, and then resolve
> it in the Makefile proper like:
>
> ifdef DARWIN_OPENSSL
> # Overrides AUTO_AVOID_OPENSSL, do nothing.
> else ifdef AUTO_AVOID_OPENSSL
> NO_OPENSSL = YesPlease
> APPLE_COMMON_CRYPTO = YesPlease
> endif
>
> but that's totally untested.
Good point. I think I found an even easier way to achieve the same.
What do you think about the patch below?
Thanks,
Lars
-- >8 --
Subject: Makefile: set NO_OPENSSL on macOS by default
Apple removed the OpenSSL header files in macOS 10.11 and above. OpenSSL
was deprecated since macOS 10.7.
Set `NO_OPENSSL` and `APPLE_COMMON_CRYPTO` to `YesPlease` as default for
macOS. It is possible to override this and use OpenSSL by defining
`NO_APPLE_COMMON_CRYPTO`.
Original-patch-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/Makefile b/Makefile
index 9d6c245..f53fcc9 100644
--- a/Makefile
+++ b/Makefile
@@ -1047,6 +1047,7 @@ ifeq ($(uname_S),Darwin)
endif
endif
ifndef NO_APPLE_COMMON_CRYPTO
+ NO_OPENSSL = YesPlease
APPLE_COMMON_CRYPTO = YesPlease
COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
endif
--
^ permalink raw reply related
* Re: [PATCH (optional)] t0021: use arithmetic expansion to trim whitespace from wc -c output
From: Johannes Sixt @ 2016-11-06 19:31 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <C61D1F80-7B7A-4111-9418-75D178AF4353@gmail.com>
Am 06.11.2016 um 16:45 schrieb Lars Schneider:
>
>> On 03 Nov 2016, at 21:22, Johannes Sixt <j6t@kdbg.org> wrote:
>> This is a pure optimization that reduces the number of forks, which
>> helps a bit on Windows.
>>
>> There would be a solution with perl that does not require trimming
>> of whitespace, but perl startup times are unbearable on Windows.
>> wc -c is better.
I was wrong here. I had looked at the perl invocations due to
git-sendemail, and they are awfully slow. A do-almost-nothing perl
invocation is in the same ballpark as wc. Therefore I changed my mind
and suggest the patch below instead.
> Since the file size function became very simple with your patch,
> shouldn't we get rid of it? If you agree, then we could squash the
> patch below into your patch.
In the new patch, the function is not that trivial (it uses perl), and
the call sites can remain as they are (simple shell variables and
substitutions).
---- 8< ----
[PATCH] t0021: compute file size with a single process instead of a pipeline
Avoid unwanted coding patterns (prodigal use of pipelines), and in
particular a useless use of cat.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
t/t0021-conversion.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index db71acacb3..cb72fa49de 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -22,7 +22,7 @@ generate_random_characters () {
}
file_size () {
- cat "$1" | wc -c | sed "s/^[ ]*//"
+ perl -e 'print -s $ARGV[0]' "$1"
}
filter_git () {
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-06 17:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Christian Couder, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <xmqqshr4cyy7.fsf@gitster.mtv.corp.google.com>
On Sun, Nov 06, 2016 at 09:14:56AM -0800, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> > We could, but if we (or one of the many third-party git implementations)
> > miss a case, gitlinks+reachability may appear to work in many cases with
> > dataloss afterward, while gitrefs will fail early and not appear
> > functional.
>
> I wonder what happens if we do not introduce the "gitref" but
> instead change the behaviour of "gitlink" to imply an optional
> reachability. That is, when enumerating what is reachable in your
> repository, if you see a gitlink and if you notice that you locally
> have the target of that gitlink, you follow, but if you know you
> lack it, you do not error out. This may be making things too
> complex to feasibily implement by simplify them ;-) and I see a few
> immediate fallout that needs to be thought through (i.e. downsides)
> and a few upsides, too. I am feeling feverish and not thinking
> straight, so I won't try to weigh pros-and-cons.
>
> This would definitely need protocol extension when transferring
> objects across repositories.
It'd also need a repository format extension locally. Otherwise, if you
ever touched that repository with an older git (or a tool built on an
older libgit2 or JGit or other library), you could lose data.
It does seem conceptually appealing, though. In an ideal world, the
original version of gitlink would have had opt-out reachability (and
.gitmodules with an external repository reference could count as opting
out).
But I can't think of any case where it's OK for a git implementation to
not know about this reachability extension and still operate on the
gitlink. And given that, it might as well use a new object type that
the old version definitely won't think it understands.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-11-06 17:16 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD2H6rZNQqg1VGNkes65KAk-4P160rjQSjFY3NjgZydZmA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> I think it is easier for user to be able to just set core.splitIndex
> to true to enable split-index.
You can have that exact benefit by making core.splitIndex to
bool-or-more. If your default is 20%, take 'true' as if the user
specified 20% and take 'false' as if the user specified 100% (or is
it 0%? I do not care about the details but you get the point).
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Junio C Hamano @ 2016-11-06 17:14 UTC (permalink / raw)
To: Josh Triplett
Cc: Jacob Keller, Christian Couder, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <20161106163410.ilysej5r6qd3744e@x>
Josh Triplett <josh@joshtriplett.org> writes:
> We could, but if we (or one of the many third-party git implementations)
> miss a case, gitlinks+reachability may appear to work in many cases with
> dataloss afterward, while gitrefs will fail early and not appear
> functional.
I wonder what happens if we do not introduce the "gitref" but
instead change the behaviour of "gitlink" to imply an optional
reachability. That is, when enumerating what is reachable in your
repository, if you see a gitlink and if you notice that you locally
have the target of that gitlink, you follow, but if you know you
lack it, you do not error out. This may be making things too
complex to feasibily implement by simplify them ;-) and I see a few
immediate fallout that needs to be thought through (i.e. downsides)
and a few upsides, too. I am feeling feverish and not thinking
straight, so I won't try to weigh pros-and-cons.
This would definitely need protocol extension when transferring
objects across repositories.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-06 16:34 UTC (permalink / raw)
To: Jacob Keller
Cc: Christian Couder, Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <CA+P7+xoG3ag8dj7s_NRoqz-EwjVENSJSzE_qj6gnW-SmWt0bgA@mail.gmail.com>
On Sat, Nov 05, 2016 at 09:50:07PM -0700, Jacob Keller wrote:
> On Sat, Nov 5, 2016 at 1:25 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Sat, Nov 05, 2016 at 09:21:58PM +0100, Christian Couder wrote:
> >> On Sat, Nov 5, 2016 at 4:18 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> >> > On Sat, Nov 05, 2016 at 01:45:27PM +0100, Christian Couder wrote:
> >> >> And with what Peff says above it looks like we will need ways
> >> >> configure and tweak commit reachability with gitlink/gitref anyway. So
> >> >> the point of gitref compared to gitlink would be that they just have a
> >> >> different reachability by default. But couldn't that be replaced by a
> >> >> default rule saying that when a gitlink is reached "this way or that
> >> >> way" then the commit reachability should be enforced, and otherwise it
> >> >> should not be?
> >> >
> >> > Any version of git unaware of that rule, though, would consider objects
> >> > only reachable by gitlink as unreachable and delete them, causing data
> >> > loss. Likewise for a server not aware of that rule. And a server
> >> > unaware of that rule would not supply those objects to a client pulling
> >> > such a branch.
> >>
> >> Yeah, so you would really need an up-to-date server and client to
> >> store the git-series data.
> >> But anyway if we create a gitref object, you would also need
> >> up-to-date servers and clients to make it work.
> >
> > Agreed, but gitrefs have the advantage of failing safe, rather than
> > failing with dataloss.
> >
> > - Josh Triplett
>
> Isn't the "failing safe" only true if the client disconnects when a
> server doesn't advertise "i understand gitrefs"? So couldn't we, as
> part of the rules for reachability advertise a capability that does a
> similar thing and fails safe as well?
We could, but if we (or one of the many third-party git implementations)
miss a case, gitlinks+reachability may appear to work in many cases with
dataloss afterward, while gitrefs will fail early and not appear
functional.
^ permalink raw reply
* Re: [PATCH (optional)] t0021: use arithmetic expansion to trim whitespace from wc -c output
From: Lars Schneider @ 2016-11-06 15:45 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <3b58b90d-5435-1503-d052-413a947a5ab5@kdbg.org>
> On 03 Nov 2016, at 21:22, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Instead of a pipeline with sed and a useless use of cat, return the
> unmodified text of wc -c from function file_size, but substitute the
> result with arithmetic expansion to get rid of the leading whitespace
> that some version of wc -c print.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> This is a pure optimization that reduces the number of forks, which
> helps a bit on Windows.
>
> There would be a solution with perl that does not require trimming
> of whitespace, but perl startup times are unbearable on Windows.
> wc -c is better.
>
> t/t0021-conversion.sh | 50 +++++++++++++++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index db71acacb3..42b529f615 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -22,7 +22,7 @@ generate_random_characters () {
> }
>
> file_size () {
> - cat "$1" | wc -c | sed "s/^[ ]*//"
> + wc -c <"$1"
> }
>
> filter_git () {
> @@ -369,10 +369,10 @@ test_expect_success PERL 'required process filter should filter data' '
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> - IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
> + IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> + IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> - IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
> + IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
> STOP
> EOF
> test_cmp_count expected.log rot13-filter.log &&
> @@ -381,14 +381,14 @@ test_expect_success PERL 'required process filter should filter data' '
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> - IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
> + IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> + IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> - IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> - IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
> + IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
> + IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> + IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
> - IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
> + IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
> STOP
> EOF
> test_cmp_count expected.log rot13-filter.log &&
> @@ -399,8 +399,8 @@ test_expect_success PERL 'required process filter should filter data' '
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
> - IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
> + IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> + IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
> STOP
> EOF
> test_cmp_exclude_clean expected.log rot13-filter.log &&
> @@ -409,7 +409,7 @@ test_expect_success PERL 'required process filter should filter data' '
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> + IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> STOP
> EOF
> test_cmp_exclude_clean expected.log rot13-filter.log &&
> @@ -418,10 +418,10 @@ test_expect_success PERL 'required process filter should filter data' '
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: smudge test.r $S [OK] -- OUT: $S . [OK]
> - IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
> + IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> + IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
> - IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
> + IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
> STOP
> EOF
> test_cmp_exclude_clean expected.log rot13-filter.log &&
> @@ -451,7 +451,7 @@ test_expect_success PERL 'required process filter takes precedence' '
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> + IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> STOP
> EOF
> test_cmp_count expected.log rot13-filter.log
> @@ -474,7 +474,7 @@ test_expect_success PERL 'required process filter should be used only for "clean
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: clean test.r $S [OK] -- OUT: $S . [OK]
> + IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> STOP
> EOF
> test_cmp_count expected.log rot13-filter.log &&
> @@ -603,11 +603,11 @@ test_expect_success PERL 'process filter should restart after unexpected write f
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF [WRITE FAIL]
> + IN: smudge smudge-write-fail.r $(($SF)) [OK] -- OUT: $(($SF)) [WRITE FAIL]
> START
> init handshake complete
> - IN: smudge test.r $S [OK] -- OUT: $S . [OK]
> - IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
> + IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> + IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> STOP
> EOF
> test_cmp_exclude_clean expected.log rot13-filter.log &&
> @@ -649,9 +649,9 @@ test_expect_success PERL 'process filter should not be restarted if it signals a
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]
> - IN: smudge test.r $S [OK] -- OUT: $S . [OK]
> - IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
> + IN: smudge error.r $(($SE)) [OK] -- OUT: 0 [ERROR]
> + IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
> + IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
> STOP
> EOF
> test_cmp_exclude_clean expected.log rot13-filter.log &&
> @@ -688,7 +688,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f
> cat >expected.log <<-EOF &&
> START
> init handshake complete
> - IN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]
> + IN: smudge abort.r $(($SA)) [OK] -- OUT: 0 [ABORT]
> STOP
> EOF
> test_cmp_exclude_clean expected.log rot13-filter.log &&
> --
> 2.11.0.rc0.55.gd967357
>
This looks good to me! As I run the Windows tests once in a while, too,
I am in full support to make them faster :-)
Since the file size function became very simple with your patch,
shouldn't we get rid of it? If you agree, then we could squash the
patch below into your patch.
I also would like to move the arithmetic expansion to the variable
to make the `expected.log` better readable. That means I would like
to have this:
S=$(($(wc -c <test.r))) &&
...
IN: clean test.r $S [OK] -- OUT: $S . [OK]
Instead of this:
S=$(wc -c <test.r) &&
...
IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
Would you agree?
Thanks,
Lars
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9968465..4454587 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -23,10 +23,6 @@ generate_random_characters () {
perl -pe "s/./chr((ord($&) % 26) + ord('a'))/sge" >"$TEST_ROOT/$NAME"
}
-file_size () {
- wc -c <"$1"
-}
-
filter_git () {
rm -f rot13-filter.log &&
git "$@" 2>git-stderr.log &&
@@ -363,9 +359,9 @@ test_expect_success PERL 'required process filter should filter data' '
cp "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r" &&
>test4-empty.r &&
- S=$(file_size test.r) &&
- S2=$(file_size test2.r) &&
- S3=$(file_size "testsubdir/test3 '\''sq'\'',\$x.r") &&
+ S=$(wc -c <test.r) &&
+ S2=$(wc -c <test2.r) &&
+ S3=$(wc -c <"testsubdir/test3 '\''sq'\'',\$x.r") &&
filter_git add . &&
cat >expected.log <<-EOF &&
@@ -446,7 +442,7 @@ test_expect_success PERL 'required process filter takes precedence' '
echo "*.r filter=protocol" >.gitattributes &&
cp "$TEST_ROOT/test.o" test.r &&
- S=$(file_size test.r) &&
+ S=$(wc -c <test.r) &&
# Check that the process filter is invoked here
filter_git add . &&
@@ -470,7 +466,7 @@ test_expect_success PERL 'required process filter should be used only for "clean
echo "*.r filter=protocol" >.gitattributes &&
cp "$TEST_ROOT/test.o" test.r &&
- S=$(file_size test.r) &&
+ S=$(wc -c <test.r) &&
filter_git add . &&
cat >expected.log <<-EOF &&
@@ -589,9 +585,9 @@ test_expect_success PERL 'process filter should restart after unexpected write f
echo "this is going to fail" >smudge-write-fail.o &&
cp smudge-write-fail.o smudge-write-fail.r &&
- S=$(file_size test.r) &&
- S2=$(file_size test2.r) &&
- SF=$(file_size smudge-write-fail.r) &&
+ S=$(wc -c <test.r) &&
+ S2=$(wc -c <test2.r) &&
+ SF=$(wc -c <smudge-write-fail.r) &&
git add . &&
rm -f *.r &&
@@ -640,9 +636,9 @@ test_expect_success PERL 'process filter should not be restarted if it signals a
echo "this will cause an error" >error.o &&
cp error.o error.r &&
- S=$(file_size test.r) &&
- S2=$(file_size test2.r) &&
- SE=$(file_size error.r) &&
+ S=$(wc -c <test.r) &&
+ S2=$(wc -c <test2.r) &&
+ SE=$(wc -c <error.r) &&
git add . &&
rm -f *.r &&
@@ -679,7 +675,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f
echo "error this blob and all future blobs" >abort.o &&
cp abort.o abort.r &&
- SA=$(file_size abort.r) &&
+ SA=$(wc -c <abort.r) &&
git add . &&
rm -f *.r &&
^ permalink raw reply related
* Re: [PATCH] t0021: expect more variations in the output of uniq -c
From: Lars Schneider @ 2016-11-06 15:31 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <fb4d62de-fbb5-a2b4-8eba-b135125dafa9@kdbg.org>
> On 03 Nov 2016, at 21:12, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Some versions of uniq -c write the count left-justified, other version
> write it right-justified. Be prepared for both kinds.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Here it is as a proper patch.
>
> Am 03.11.2016 um 01:41 schrieb Lars Schneider:
>>> On 2 Nov 2016, at 18:03, Johannes Sixt <j6t@kdbg.org> wrote:
>>> + sort "$FILE" | uniq -c |
>>> + sed -e "s/^ *[0-9][0-9]* *IN: /x IN: /" >"$FILE.tmp" &&
>>
>> This looks good (thanks for cleaning up the redundant clean/smudge
>> stuff - that was a refactoring artifact!). One minor nit: doesn't sed
>> understand '[0-9]+' ?
>
> I don't think so.
>
>>> + mv "$FILE.tmp" "$FILE" || return
>>
>> Why '|| return' here?
>
> If there is an error in the pipeline or in the mv command, the for loop
> would not exit otherwise. The subsequent test_cmp most likely fails,
> but the || return is more correct.
>
> t/t0021-conversion.sh | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index a20b9f58e3..db71acacb3 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -40,10 +40,9 @@ test_cmp_count () {
> actual=$2
> for FILE in "$expect" "$actual"
> do
> - sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
> - sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
> - sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
> - mv "$FILE.tmp" "$FILE"
> + sort "$FILE" | uniq -c |
> + sed -e "s/^ *[0-9][0-9]*[ ]*IN: /x IN: /" >"$FILE.tmp" &&
> + mv "$FILE.tmp" "$FILE" || return
> done &&
> test_cmp "$expect" "$actual"
> }
> --
> 2.11.0.rc0.55.gd967357
>
This looks good to me. I wonder if I should post a patch
to add the "|| return" trick to the following function
"test_cmp_exclude_clean", too?!
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 4/4] t0021: fix filehandle usage on older perl
From: Lars Schneider @ 2016-11-06 14:55 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, Johannes Sixt, Junio C Hamano, git,
Martin-Louis Bright
In-Reply-To: <20161102182301.hezruhtuhra6uoft@sigill.intra.peff.net>
> On 02 Nov 2016, at 19:23, Jeff King <peff@peff.net> wrote:
>
> The rot13-filter.pl script calls methods on implicitly
> defined filehandles (STDOUT, and the result of an open()
> call). Prior to perl 5.13, these methods are not
> automatically loaded, and perl will complain with:
>
> Can't locate object method "flush" via package "IO::Handle"
>
> Let's explicitly load IO::File (which inherits from
> IO::Handle). That's more than we need for just "flush", but
> matches what perl has done since:
>
> http://perl5.git.perl.org/perl.git/commit/15e6cdd91beb4cefae4b65e855d68cf64766965d
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I see J6t solved this a week ago using "FileHandle". These days that is
> basically a compatibility synonym for IO::File. I think both should be
> available pretty much everywhere, so I went with IO::File for the
> reasons above. But if that doesn't work for some reason, switching to
> "use FileHandle" should be OK, too.
>
> I don't have an old enough perl easily available to test it either way.
>
> t/t0021/rot13-filter.pl | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index e3ea58e1e..4d5697ee5 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -21,6 +21,7 @@
>
> use strict;
> use warnings;
> +use IO::File;
>
> my $MAX_PACKET_CONTENT_SIZE = 65516;
> my @capabilities = @ARGV;
> --
> 2.11.0.rc0.258.gf434c15
Looks good to me (and works flawlessly on newer macOS).
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 3/4] t0021: use $PERL_PATH for rot13-filter.pl
From: Jeff King @ 2016-11-06 14:54 UTC (permalink / raw)
To: Lars Schneider; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <E073426F-8285-4D11-91BC-E1B80F54AD32@gmail.com>
On Sun, Nov 06, 2016 at 03:52:00PM +0100, Lars Schneider wrote:
> Would it make sense to rename "rot13-filter.pl" to
> "rot13-filter.pl.template" or something because of the
> missing shebang?
I think it makes sense to keep it as ".pl". Without the "#!" things like
editors will use the extension to guess what type of file it is, for
features like syntax highlight and auto-indentation.
I was actually tempted to _keep_ the #! line for that reason, but then
the built result ends up with two such lines (which is not wrong, but is
confusing if anybody actually looks at it).
-Peff
^ permalink raw reply
* Re: [PATCH 3/4] t0021: use $PERL_PATH for rot13-filter.pl
From: Lars Schneider @ 2016-11-06 14:52 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <20161102182022.zalzmc6rcwmvrgqq@sigill.intra.peff.net>
> On 02 Nov 2016, at 19:20, Jeff King <peff@peff.net> wrote:
>
> The rot13-filter.pl script hardcodes "#!/usr/bin/perl", and
> does not respect $PERL_PATH at all. That is a problem if the
> system does not have perl at that path, or if it has a perl
> that is too old to run a complicated script like the
> rot13-filter (but PERL_PATH points to a more modern one).
>
> We can fix this by using write_script() to create a new copy
> of the script with the correct #!-line. In theory we could
> move the whole script inside t0021-conversion.sh rather than
> having it as an auxiliary file, but it's long enough that
> it just makes things harder to read.
>
> As a bonus, we can stop using the full path to the script in
> the filter-process config we add (because the trash
> directory is in our PATH). Not only is this shorter, but it
> sidesteps any shell-quoting issues. The original was broken
> when $TEST_DIRECTORY contained a space, because it was
> interpolated in the outer script.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t0021-conversion.sh | 19 +++++++++++--------
> t/t0021/rot13-filter.pl | 1 -
> 2 files changed, 11 insertions(+), 9 deletions(-)
> mode change 100755 => 100644 t/t0021/rot13-filter.pl
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index c1ad20c61..a8fa52148 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -13,6 +13,9 @@ tr \
> 'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
> EOF
>
> +write_script rot13-filter.pl "$PERL_PATH" \
> + <"$TEST_DIRECTORY"/t0021/rot13-filter.pl
> +
> generate_random_characters () {
> LEN=$1
> NAME=$2
> @@ -341,7 +344,7 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '
> '
>
> 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.process "rot13-filter.pl clean smudge" &&
> test_config_global filter.protocol.required true &&
> rm -rf repo &&
> mkdir repo &&
> @@ -434,7 +437,7 @@ test_expect_success PERL 'required process filter should filter data' '
>
> test_expect_success PERL 'required process filter takes precedence' '
> test_config_global filter.protocol.clean false &&
> - test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
> + test_config_global filter.protocol.process "rot13-filter.pl clean" &&
> test_config_global filter.protocol.required true &&
> rm -rf repo &&
> mkdir repo &&
> @@ -459,7 +462,7 @@ test_expect_success PERL 'required process filter takes precedence' '
> '
>
> test_expect_success PERL 'required process filter should be used only for "clean" operation only' '
> - test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
> + test_config_global filter.protocol.process "rot13-filter.pl clean" &&
> rm -rf repo &&
> mkdir repo &&
> (
> @@ -494,7 +497,7 @@ test_expect_success PERL 'required process filter should be used only for "clean
> '
>
> 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.process "rot13-filter.pl clean smudge" &&
> test_config_global filter.protocol.required true &&
>
> rm -rf repo &&
> @@ -554,7 +557,7 @@ test_expect_success PERL 'required process filter should process multiple packet
> '
>
> test_expect_success PERL 'required process filter with clean error should fail' '
> - test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + test_config_global filter.protocol.process "rot13-filter.pl clean smudge" &&
> test_config_global filter.protocol.required true &&
> rm -rf repo &&
> mkdir repo &&
> @@ -573,7 +576,7 @@ test_expect_success PERL 'required process filter with clean error should fail'
> '
>
> 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" &&
> + test_config_global filter.protocol.process "rot13-filter.pl clean smudge" &&
> rm -rf repo &&
> mkdir repo &&
> (
> @@ -624,7 +627,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f
> '
>
> test_expect_success PERL 'process filter should not be restarted if it signals an error' '
> - test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + test_config_global filter.protocol.process "rot13-filter.pl clean smudge" &&
> rm -rf repo &&
> mkdir repo &&
> (
> @@ -663,7 +666,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a
> '
>
> test_expect_success PERL 'process filter abort stops processing of all further files' '
> - test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
> + test_config_global filter.protocol.process "rot13-filter.pl clean smudge" &&
> rm -rf repo &&
> mkdir repo &&
> (
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> old mode 100755
> new mode 100644
> index ae4c50f5c..e3ea58e1e
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -1,4 +1,3 @@
> -#!/usr/bin/perl
> #
> # Example implementation for the Git filter protocol version 2
> # See Documentation/gitattributes.txt, section "Filter Protocol"
> --
> 2.11.0.rc0.258.gf434c15
Looks good to me!
Minor pedantic nit:
Would it make sense to rename "rot13-filter.pl" to
"rot13-filter.pl.template" or something because of the
missing shebang?
- Lars
^ permalink raw reply
* Re: [PATCH 1/4] t0021: use write_script to create rot13 shell script
From: Lars Schneider @ 2016-11-06 14:43 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <20161106142957.52wx2llanw5bis4i@sigill.intra.peff.net>
> On 06 Nov 2016, at 15:29, Jeff King <peff@peff.net> wrote:
>
> On Sun, Nov 06, 2016 at 03:25:33PM +0100, Lars Schneider wrote:
>
>> This looks good to me (and it works on my machine).
>> However, I took a look at the "write_script" function and found this,
>> added by Junio in 840c519d:
>>
>> echo "#!${2-"$SHELL_PATH"}" &&
>>
>> There is some kind of variable expansion happening with the "2-" but
>> I can't quite figure out what is going on. Plus, I can't find anything
>> about this in the sh docs.
>>
>> Can anyone help me to understand it?
>
> See the section on parameter expansion in "man bash". Basically:
>
> ${foo:-bar}
>
> expands to $foo, or "bar" if it is unset or empty. Without the colon:
>
> ${foo-bar}
>
> expands to $foo, "bar" if it unset (but not if it is empty).
Ahh! The missing colon tricked me. For some reason the version
without colon is not mentioned in my docs (GNU bash, version 3.2.57)
or I overlooked it.
Thanks for taking the time to explain it!
- Lars
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Lars Schneider @ 2016-11-06 14:29 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <20161102181824.mi6lmfnfckvrav7n@sigill.intra.peff.net>
> On 02 Nov 2016, at 19:18, Jeff King <peff@peff.net> wrote:
>
> We create a rot13.sh script in the trash directory, but need
> to call it by its full path when we have moved our cwd to
> another directory. Let's just put $TEST_ROOT in our $PATH so
> that the script is always found.
>
> This is a minor convenience for rot13.sh, but will be a
> major one when we switch rot13-filter.pl to a script in the
> same directory, as it means we will not have to deal with
> shell quoting inside the filter-process config.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t0021-conversion.sh | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index dfde22549..c1ad20c61 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes'
> . ./test-lib.sh
>
> TEST_ROOT="$(pwd)"
> +PATH=$TEST_ROOT:$PATH
>
> write_script <<\EOF "$TEST_ROOT/rot13.sh"
> tr \
> @@ -64,7 +65,7 @@ test_cmp_exclude_clean () {
> # is equal to the committed content.
> test_cmp_committed_rot13 () {
> test_cmp "$1" "$2" &&
> - "$TEST_ROOT/rot13.sh" <"$1" >expected &&
> + rot13.sh <"$1" >expected &&
> git cat-file blob :"$2" >actual &&
> test_cmp expected actual
> }
> @@ -513,7 +514,7 @@ test_expect_success PERL 'required process filter should process multiple packet
> for FILE in "$TEST_ROOT"/*.file
> do
> cp "$FILE" . &&
> - "$TEST_ROOT/rot13.sh" <"$FILE" >"$FILE.rot13"
> + rot13.sh <"$FILE" >"$FILE.rot13"
> done &&
>
> echo "*.file filter=protocol" >.gitattributes &&
> @@ -616,7 +617,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f
>
> # Smudge failed
> ! test_cmp smudge-write-fail.o smudge-write-fail.r &&
> - "$TEST_ROOT/rot13.sh" <smudge-write-fail.o >expected &&
> + rot13.sh <smudge-write-fail.o >expected &&
> git cat-file blob :smudge-write-fail.r >actual &&
> test_cmp expected actual
> )
> --
> 2.11.0.rc0.258.gf434c15
Looks good to me and the PWD Windows fix suggested by Hannes [1] works great
on macOS.
Thanks,
Lars
[1] http://public-inbox.org/git/20161103204438.zfe653c2bsv3zqkx@sigill.intra.peff.net/
^ permalink raw reply
* Re: [PATCH 1/4] t0021: use write_script to create rot13 shell script
From: Jeff King @ 2016-11-06 14:29 UTC (permalink / raw)
To: Lars Schneider; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <22DAA280-1857-4E22-914A-1208D784DA98@gmail.com>
On Sun, Nov 06, 2016 at 03:25:33PM +0100, Lars Schneider wrote:
> This looks good to me (and it works on my machine).
> However, I took a look at the "write_script" function and found this,
> added by Junio in 840c519d:
>
> echo "#!${2-"$SHELL_PATH"}" &&
>
> There is some kind of variable expansion happening with the "2-" but
> I can't quite figure out what is going on. Plus, I can't find anything
> about this in the sh docs.
>
> Can anyone help me to understand it?
See the section on parameter expansion in "man bash". Basically:
${foo:-bar}
expands to $foo, or "bar" if it is unset or empty. Without the colon:
${foo-bar}
expands to $foo, "bar" if it unset (but not if it is empty). I don't
think we really care about the distinction here, and either is fine (you
would not ever pass an empty argument).
So in this context you may pass in the interpreter:
write_script "$PERL_PATH" <<\EOF
... some perl code ...
EOF
or it defaults to shell, which is what most of the callers want:
write_script <<\EOF
... some shell code ...
EOF
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] t0021: use write_script to create rot13 shell script
From: Lars Schneider @ 2016-11-06 14:25 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Junio C Hamano, git
In-Reply-To: <20161102181751.trxsns5pxqk73ukc@sigill.intra.peff.net>
> On 02 Nov 2016, at 19:17, Jeff King <peff@peff.net> wrote:
>
> This avoids us fooling around with $SHELL_PATH and the
> executable bit ourselves.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t0021-conversion.sh | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index a20b9f58e..dfde22549 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -6,13 +6,11 @@ test_description='blob conversion via gitattributes'
>
> TEST_ROOT="$(pwd)"
>
> -cat <<EOF >"$TEST_ROOT/rot13.sh"
> -#!$SHELL_PATH
> +write_script <<\EOF "$TEST_ROOT/rot13.sh"
> tr \
> 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
> 'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
> EOF
> -chmod +x "$TEST_ROOT/rot13.sh"
>
> generate_random_characters () {
> LEN=$1
> --
> 2.11.0.rc0.258.gf434c15
>
This looks good to me (and it works on my machine).
However, I took a look at the "write_script" function and found this,
added by Junio in 840c519d:
echo "#!${2-"$SHELL_PATH"}" &&
There is some kind of variable expansion happening with the "2-" but
I can't quite figure out what is going on. Plus, I can't find anything
about this in the sh docs.
Can anyone help me to understand it?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v5 08/27] sequencer: completely revamp the "todo" script parsing
From: Lars Schneider @ 2016-11-06 14:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Jakub Narębski, Johannes Sixt,
Ramsay Jones, Torsten Bögershausen, Jeff King
In-Reply-To: <c0d106125e1b6ba107b1ef06fb60dd710d184d29.1477052405.git.johannes.schindelin@gmx.de>
> On 21 Oct 2016, at 14:24, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>
> When we came up with the "sequencer" idea, we really wanted to have
> kind of a plumbing equivalent of the interactive rebase. Hence the
> choice of words: the "todo" script, a "pick", etc.
>
> ...
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 284 ++++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 163 insertions(+), 121 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 499f5ee..145de78 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -470,7 +470,26 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
> return 1;
> }
>
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +enum todo_command {
> + TODO_PICK = 0,
> + TODO_REVERT
> +};
> +
> +static const char *todo_command_strings[] = {
> + "pick",
> + "revert"
> +};
> +
> +static const char *command_to_string(const enum todo_command command)
> +{
> + if (command < ARRAY_SIZE(todo_command_strings))
With DEVELOPER=1 I get this error on macOS when I compile current git/next (b27dc33) using clang:
sequencer.c:632:14: error: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
if (command < ARRAY_SIZE(todo_command_strings))
Torsten discovered this problem already in v3 and Peff suggested
a working solution [1]. Is there any reason not to use Peff's suggestion?
Cheers,
Lars
[1] http://public-inbox.org/git/d9f4f658-94fb-cb9e-7da8-3a2fac120a9e@web.de/
^ permalink raw reply
* Re: gitk: avoid obscene memory consumption
From: Markus Hitter @ 2016-11-06 10:28 UTC (permalink / raw)
To: Paul Mackerras, Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <20161105110845.GA4039@fergus.ozlabs.ibm.com>
Am 05.11.2016 um 12:08 schrieb Paul Mackerras:
> On Fri, Nov 04, 2016 at 03:45:09PM -0700, Stefan Beller wrote:
>> On Fri, Nov 4, 2016 at 12:49 PM, Markus Hitter <mah@jump-ing.de> wrote:
>>>
>>> Hello all,
>>
>> +cc Paul Mackeras, who maintains gitk.
>
> Thanks.
>
>>>
>>> after Gitk brought my shabby development machine (Core2Duo, 4 GB RAM, Ubuntu 16.10, no swap to save the SSD) to its knees once more than I'm comfortable with, I decided to investigate this issue.
>>>
>>> Result of this investigation is, my Git repo has a commit with a diff of some 365'000 lines and Gitk tries to display all of them, consuming more than 1.5 GB of memory.
>>>
>>> The solution is to cut off diffs at 50'000 lines for the display. This consumes about 350 MB RAM, still a lot. These first 50'000 lines are shown, followed by a copyable message on how to view the full diff on the command line. Diffs shorter than this limit are displayed as before.
>
> That sounds reasonable.
>
>>
>> Bikeshedding: I'd argue to even lower the number to 5-10k lines.
>
> I could go with 10k.
Thanks for the positive comments.
TBH, the more I think about the problem, the less I'm satisfied with the solution I provided. Including two reasons:
- The list of files affected to the right is still complete and clicking a file name further down results in nothing ... as if the file wasn't part of the diff.
- Local searches. Cutting off diffs makes them unreliable. Global searches still work, but actually viewing a search result in the skipped section is no longer possible.
So I'm watching out for better solutions. So far I can think of these:
- Storing only the actually viewed diff. It's an interactive tool, so there's no advantage in displaying the diff in 0.001 seconds over viewing it in 0.1 seconds. As far as I can see, Gitk currently stores every diff it gets a hold of forever.
- View the diff sparsely. Like rendering only the actually visible portion.
- Enhancing ctext. This reference diff has 28 million characters, so there should be a way to store this with color information in, let's say, 29 MB of memory.
Any additional ideas?
Markus
--
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Jacob Keller @ 2016-11-06 7:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kYcs0FiXdP6UZGgSmUpn_3vpnKo2RwTRJCvksP7+1o_wQ@mail.gmail.com>
On Tue, Nov 1, 2016 at 10:31 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Nov 1, 2016 at 10:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>
>> Maybe I am old fashioned, but I'd feel better to see these with
>> explicit "extern" in front (check the older header files like
>> cache.h when you are in doubt what the project convention has been).
>
> I did check the other files and saw them, so I was very unsure what to
> suggest here. I only saw the extern keyword used in headers that were
> there when Git was really young, so I assumed it's a style nit by kernel
> developers. Thanks for clarifying!
>
> I think we'll want to have some consistency though, so we
> maybe want to coordinate a cleanup of submodule.h as well as
> submodule-config.h to mark all the functions extern.
>
> This doesn't need to be a all-at-once thing, but we'd keep it in mind
> for future declarations in the header.
>
> Thanks,
> Stefan
Extern is generally used when you want to declare a header for a
function that's in a different object file. I'm not sure if we
actually need it or not though.
Thanks,
Jake
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jacob Keller @ 2016-11-06 4:50 UTC (permalink / raw)
To: Josh Triplett
Cc: Christian Couder, Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <20161105202553.migx75gfuujakqyk@x>
On Sat, Nov 5, 2016 at 1:25 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Sat, Nov 05, 2016 at 09:21:58PM +0100, Christian Couder wrote:
>> On Sat, Nov 5, 2016 at 4:18 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> > On Sat, Nov 05, 2016 at 01:45:27PM +0100, Christian Couder wrote:
>> >> And with what Peff says above it looks like we will need ways
>> >> configure and tweak commit reachability with gitlink/gitref anyway. So
>> >> the point of gitref compared to gitlink would be that they just have a
>> >> different reachability by default. But couldn't that be replaced by a
>> >> default rule saying that when a gitlink is reached "this way or that
>> >> way" then the commit reachability should be enforced, and otherwise it
>> >> should not be?
>> >
>> > Any version of git unaware of that rule, though, would consider objects
>> > only reachable by gitlink as unreachable and delete them, causing data
>> > loss. Likewise for a server not aware of that rule. And a server
>> > unaware of that rule would not supply those objects to a client pulling
>> > such a branch.
>>
>> Yeah, so you would really need an up-to-date server and client to
>> store the git-series data.
>> But anyway if we create a gitref object, you would also need
>> up-to-date servers and clients to make it work.
>
> Agreed, but gitrefs have the advantage of failing safe, rather than
> failing with dataloss.
>
> - Josh Triplett
Isn't the "failing safe" only true if the client disconnects when a
server doesn't advertise "i understand gitrefs"? So couldn't we, as
part of the rules for reachability advertise a capability that does a
similar thing and fails safe as well?
Thanks.
Jake
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 21:56 UTC (permalink / raw)
To: Josh Triplett; +Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King
In-Reply-To: <20161104211959.3532uiud27nhumt7@x>
On Fri, Nov 4, 2016 at 10:19 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Fri, Nov 04, 2016 at 09:47:41PM +0100, Christian Couder wrote:
>> On Fri, Nov 4, 2016 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> > Imagine we invent a new tree entry type, "gitref", that is similar
>> > to "gitlink" in that it can record a commit object name in a tree,
>> > but unlike "gitlink" it does imply reachability. And you do not add
>> > phony parents to your commit object. A tree that has "gitref"s in
>> > it is about annotating the commits in the same repository (e.g. the
>> > tree references two commits, "base" and "tip", to point into a slice
>> > of the main history). And it is perfectly sensible for such a
>> > pointer to imply reachability---after all it serves different
>> > purposes from "gitlink".
>>
>> The more I think about this (and also about how to limit ref
>> advertisements as recently discussed in
>> https://public-inbox.org/git/20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net/),
>> the more I think about Shawn's RefTree:
>>
>> https://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/
Just to make things clear, after reading the above link that I posted :-) ...
>> Couldn't a RefTree be used to store refs that point to the base
>> commit, the tip commit and the blob that contains the cover letter,
>> and maybe also a ref pointing to the RefTree of the previous version
>> of the series?
>
> That's really interesting! The Software Heritage project is working on
> something similar, because they want to store all the refs as part of
> their data model as well. I'll point them to the reftree work.
>
> If upstream git supported RefTree, I could potentially use that for
> git-series. However, I do want a commit message and history for the
> series itself, and using refs in the reftree to refer to the parents
> seems like abusing reftree to recreate commits, in a reversal of the
> hack of using commit parents as a reftree. :)
>
> What if, rather than storing a hash reference to a reftree as a single
> reference and replacing it with no history, a reftree could be
> referenced from a commit and have history? (That would also allow
> tagging a version of the reftree.)
... I think that indeed that's what Shawn's reftree proposal is about,
so I agree that it makes sense.
We just need to find a good way to specify object reachability.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-05 20:25 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <CAP8UFD3XFHr7POKmZr_6guapC6sme3GvWBV5vPw4XO7FE5HOPw@mail.gmail.com>
On Sat, Nov 05, 2016 at 09:21:58PM +0100, Christian Couder wrote:
> On Sat, Nov 5, 2016 at 4:18 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Sat, Nov 05, 2016 at 01:45:27PM +0100, Christian Couder wrote:
> >> And with what Peff says above it looks like we will need ways
> >> configure and tweak commit reachability with gitlink/gitref anyway. So
> >> the point of gitref compared to gitlink would be that they just have a
> >> different reachability by default. But couldn't that be replaced by a
> >> default rule saying that when a gitlink is reached "this way or that
> >> way" then the commit reachability should be enforced, and otherwise it
> >> should not be?
> >
> > Any version of git unaware of that rule, though, would consider objects
> > only reachable by gitlink as unreachable and delete them, causing data
> > loss. Likewise for a server not aware of that rule. And a server
> > unaware of that rule would not supply those objects to a client pulling
> > such a branch.
>
> Yeah, so you would really need an up-to-date server and client to
> store the git-series data.
> But anyway if we create a gitref object, you would also need
> up-to-date servers and clients to make it work.
Agreed, but gitrefs have the advantage of failing safe, rather than
failing with dataloss.
- Josh Triplett
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 20:21 UTC (permalink / raw)
To: Josh Triplett
Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <20161105151836.wztypzrdywyltvrc@x>
On Sat, Nov 5, 2016 at 4:18 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Sat, Nov 05, 2016 at 01:45:27PM +0100, Christian Couder wrote:
>> And with what Peff says above it looks like we will need ways
>> configure and tweak commit reachability with gitlink/gitref anyway. So
>> the point of gitref compared to gitlink would be that they just have a
>> different reachability by default. But couldn't that be replaced by a
>> default rule saying that when a gitlink is reached "this way or that
>> way" then the commit reachability should be enforced, and otherwise it
>> should not be?
>
> Any version of git unaware of that rule, though, would consider objects
> only reachable by gitlink as unreachable and delete them, causing data
> loss. Likewise for a server not aware of that rule. And a server
> unaware of that rule would not supply those objects to a client pulling
> such a branch.
Yeah, so you would really need an up-to-date server and client to
store the git-series data.
But anyway if we create a gitref object, you would also need
up-to-date servers and clients to make it work.
> So I don't think "gitlink defined as reachable" quite works, unless we
> make some other format-incompatible change that forces clients and
> servers touching that gitlink to know about that reachability rule. (In
> the absence of a hack such as making the same commit a parent.)
There are other tools that would like to tweak reachability rules for objects.
For example Mike Hommey's git-cinnabar:
https://public-inbox.org/git/20150331100756.GA13377@glandium.org/
and my current work on external object databases:
https://public-inbox.org/git/20160628181933.24620-1-chriscool@tuxfamily.org/
would be interested in a way to make some blobs not reachable.
So if we had default rules and a generic way to specify that some
objects are, or are not, reachable, that could be used by many tools,
and the design of these tools would be simplified.
Maybe this could be specified in the attributes files, or in a special
file like for shallow clone.
^ 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