* Re: [PATCH 4/5] grep: optionally recurse into submodules
From: Jonathan Tan @ 2016-11-05 5:09 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20161027223834.35312-5-bmwill@google.com>
On Thu, Oct 27, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8887b6add..f34f16df9 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -18,12 +18,20 @@
> #include "quote.h"
> #include "dir.h"
> #include "pathspec.h"
> +#include "submodule.h"
>
> static char const * const grep_usage[] = {
> N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
> NULL
> };
>
> +static const char *super_prefix;
I think that the super_prefix changes could be in its own patch.
> +static int recurse_submodules;
> +static struct argv_array submodule_options = ARGV_ARRAY_INIT;
I guess this has to be static because it is shared by multiple threads.
> +
> +static int grep_submodule_launch(struct grep_opt *opt,
> + const struct grep_source *gs);
> +
> #define GREP_NUM_THREADS_DEFAULT 8
> static int num_threads;
>
> @@ -174,7 +182,10 @@ static void *run(void *arg)
> break;
>
> opt->output_priv = w;
> - hit |= grep_source(opt, &w->source);
> + if (w->source.type == GREP_SOURCE_SUBMODULE)
> + hit |= grep_submodule_launch(opt, &w->source);
> + else
> + hit |= grep_source(opt, &w->source);
It seems to me that GREP_SOURCE_SUBMODULE is of a different nature
than the other GREP_SOURCE_.* - in struct work_item, could we instead
have another variable that distinguishes between submodules and
"native" sources? This might also assuage Junio's concerns in
<xmqq37jbqf83.fsf@gitster.mtv.corp.google.com> about the nature of the
sources.
That variable could also be the discriminant for a tagged union, such
that we have "struct grep_source" for the "native" sources and a new
struct (holding only submodule-relevant information) for the
submodule.
> +/*
> + * Prep grep structures for a submodule grep
> + * sha1: the sha1 of the submodule or NULL if using the working tree
> + * filename: name of the submodule including tree name of parent
> + * path: location of the submodule
> + */
> +static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
> + const char *filename, const char *path)
> +{
> + if (!(is_submodule_initialized(path) &&
> + is_submodule_checked_out(path))) {
> + warning("skiping submodule '%s%s' since it is not initialized and checked out",
> + super_prefix ? super_prefix: "",
> + path);
> + return 0;
> + }
> +
> +#ifndef NO_PTHREADS
> + if (num_threads) {
> + add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
> + return 0;
> + } else
> +#endif
> + {
> + struct work_item w;
> + int hit;
> +
> + grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
> + filename, path, sha1);
> + strbuf_init(&w.out, 0);
> + opt->output_priv = &w;
> + hit = grep_submodule_launch(opt, &w.source);
> +
> + write_or_die(1, w.out.buf, w.out.len);
> +
> + grep_source_clear(&w.source);
> + strbuf_release(&w.out);
> + return hit;
> + }
This is at least the third invocation of this "if pthreads, add work,
otherwise do it now" pattern - could this be extracted into its own
function (in another patch)? Ideally, there would also be exactly one
function in which the grep_source.* functions are invoked, and both
"run" and the non-pthread code path can use it.
> +}
> +
> +static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
> + int cached)
This line isn't modified other than the line break, as far as I can
tell, so I wouldn't break it.
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> new file mode 100755
> index 000000000..b670c70cb
> --- /dev/null
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -0,0 +1,99 @@
> +#!/bin/sh
> +
> +test_description='Test grep recurse-submodules feature
> +
> +This test verifies the recurse-submodules feature correctly greps across
> +submodules.
> +'
> +
> +. ./test-lib.sh
> +
Would it be possible to also test it while num_threads is zero? (Or,
if num_threads is already zero, to test it while it is not zero?)
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Torsten Bögershausen @ 2016-11-05 7:27 UTC (permalink / raw)
To: Martin-Louis Bright; +Cc: Lars Schneider, Junio C Hamano, git
In-Reply-To: <CAG2PGspq34wn2bAGyhR6B-XmmayadmL-v3_65y5LJWTWNHXkOQ@mail.gmail.com>
On Thu, Nov 03, 2016 at 09:30:44AM -0400, Martin-Louis Bright wrote:
> I will see if I can find a OSX 10.6 system to test with, and I'll try with
> perl 5.10.
>
> --Martin
No need to worry too much:
I have tested Peffs patch applied on next OK-
And the integration into pu that came the 2nd Novevember is tested OK as well.
(And please everybody: avoid top-posting here)
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Torsten Bögershausen @ 2016-11-05 7:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, santiago, gitster, peff, sunshine, walters
In-Reply-To: <xmqqwpgoqjct.fsf@gitster.mtv.corp.google.com>
> * st/verify-tag (2016-10-10) 7 commits
> - t/t7004-tag: Add --format specifier tests
> - t/t7030-verify-tag: Add --format specifier tests
> - builtin/tag: add --format argument for tag -v
> - builtin/verify-tag: add --format to verify-tag
> - tag: add format specifier to gpg_verify_tag
> - ref-filter: add function to print single ref_array_item
> - gpg-interface, tag: add GPG_VERIFY_QUIET flag
>
> "git tag" and "git verify-tag" learned to put GPG verification
> status in their "--format=<placeholders>" output format.
>
> Waiting for a reroll.
> cf. <20161007210721.20437-1-santiago@nyu.edu>
I don't know if this has been reported before:
Some tests needs to be protected by GPG:
+test_expect_success GPG 'verifying a proper tag with --format pass and format accordingly'
test_expect_success GPG 'verifying a forged tag with --format fail and format accordingly'
test_expect_success GPG 'verifying a forged tag with --format fail and format accordingly'
^ permalink raw reply
* [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Johannes Schindelin @ 2016-11-05 9:50 UTC (permalink / raw)
To: git-for-windows, git
Dear Git users,
I finally got around to rebase the Windows-specific patches (which seem to
not make it upstream as fast as we get new ones) on top of upstream Git
v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
It would be really nice if those of you who have access to Windows [*2*]
could try it out and report bugs before v2.11.0 final.
Thanks,
Johannes
Footnote *1*: https://github.com/git-for-windows/git/wiki/MinGit
Footnote *2*: You can get free (time-limited) VMs running Windows here:
http://dev.modern.ie/tools/vms/windows/
^ permalink raw reply
* Re: gitk: avoid obscene memory consumption
From: Paul Mackerras @ 2016-11-05 11:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: Markus Hitter, git@vger.kernel.org
In-Reply-To: <CAGZ79kbavzGJ2sAcz5heg+BO+tZ=TgtrhxMH1-kqeJUpNNavyw@mail.gmail.com>
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.
>
> >
> > To test the waters whether such a change is welcome, here's the patch as I currently use it. If this patch makes sense I'll happily apply change requests and bring it more in line with Git's patch submission expectations.
>
> I have never contributed to gitk myself,
> which is hosted at git://ozlabs.org/~paulus/gitk
> though I'd expect these guide lines would roughly apply:
> https://github.com/git/git/blob/master/Documentation/SubmittingPatches
Paul.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 12:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Triplett, git, Shawn O. Pierce, Jeff King
In-Reply-To: <xmqq1syqedv4.fsf@gitster.mtv.corp.google.com>
On Sat, Nov 5, 2016 at 5:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Couldn't a RefTree be used to store refs that point to the base
>> commit,
>
> I think it is the other way around. With the new "gitref" thing
> that is a pointer to an in-repository commit, RefTree can be
> naturally implemented.
Yeah, I should have read Shawn's RefTree email thread again before
posting and especially before replying to Josh.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 12:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Josh Triplett, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy
In-Reply-To: <CAP8UFD0bqNQZ3nuGUDX0qrSo44hf1NL9LeZB_FQcXg3j0mD38A@mail.gmail.com>
On Sat, Nov 5, 2016 at 1:17 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sat, Nov 5, 2016 at 5:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> Couldn't a RefTree be used to store refs that point to the base
>>> commit,
>>
>> I think it is the other way around. With the new "gitref" thing
>> that is a pointer to an in-repository commit, RefTree can be
>> naturally implemented.
>
> Yeah, I should have read Shawn's RefTree email thread again before
> posting and especially before replying to Josh.
By the way, reading the following email by Peff where gitlink
reachability was already discussed:
https://public-inbox.org/git/20151217221045.GA8150@sigill.intra.peff.net/
and where Peff wrote:
> Of course, the lack of reachability has advantages, too. You can
> drop commits pointed to by old reflogs without rewriting the ref
> history. Unfortunately you cannot expunge the reflogs at all. That's
> good if you like audit trails. Bad if you are worried that your reflogs
> will grow large. :)
I think that we may not need "gitref" at all. We perhaps could just
have more ways to configure and tweak how a repo manages commit
reachability related to gitlinks.
With shallow clones we already need ways to configure and tweak commit
reachability anyway.
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?
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-05 15:18 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy
In-Reply-To: <CAP8UFD1EZ8HBzLAeyFBFgU7n2uJpswqgEgA4XM1YJuRAG_ZAAQ@mail.gmail.com>
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.
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.)
^ 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
* 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 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: 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: [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: 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 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: [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 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 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: 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 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 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 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] 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 (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: 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
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