* [PATCH v3 05/27] attr.c: complete a sentence in a comment
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 6b55a57ef..9bdf87a6f 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* directory (again, reading the file from top to bottom) down to the
* current directory, and then scan the list backwards to find the first match.
* This is exactly the same as what is_excluded() does in dir.c to deal with
- * .gitignore
+ * .gitignore file and info/excludes file as a fallback.
*/
static struct attr_stack {
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 02/27] attr.c: use strchrnul() to scan for one line
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 1fcf042b8..04d24334e 100644
--- a/attr.c
+++ b/attr.c
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
for (sp = buf; *sp; ) {
char *ep;
int more;
- for (ep = sp; *ep && *ep != '\n'; ep++)
- ;
+
+ ep = strchrnul(sp, '\n');
more = (*ep == '\n');
*ep = '\0';
handle_attr_line(res, sp, path, ++lineno, macro_ok);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 01/27] commit.c: use strchrnul() to scan for one line
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
commit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 2cf85158b..0c4ee3de4 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
p++;
if (*p) {
p = skip_blank_lines(p + 2);
- for (eol = p; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
+ eol = strchrnul(p, '\n');
} else
eol = p;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 00/27] Revamp the attribute system; another round
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
Per some of the discussion online and off I locally broke up up the question
and answer and I wasn't very thrilled with the outcome for a number of reasons.
1. The API is more complex. Callers needs to have two structures allocated
instead of one, one can be shared read-only while the other can't. While this
many not be that big of a deal, it was more confusing to me.
2. Performance hit. The allocation churn with creating/freeing a
scoreboard and the results struct adds up. It even looks like the
cost of looking up a stack frame in a hashmap isn't very cheap.
Here are some very rough performance measurements I made on my machine
on linux.git by: `perf stat -r 50 git grep "asdfghjkl"`
master: 0.302176063 seconds
v1: 0.324243806 seconds
v2: 0.304339636 seconds
split: 0.349892023 seconds (hashtable of stacks, all_attr scoreboard
allocated per git_attr_check() call, split
question/answer)
After looking at this, I'm of the opinion that the API in v2 is the best route
to take. Its a step-up from what it is currently (at master) and there isn't a
performance degradation (ok there's a small bit but it seems within the margin
of error). It also allows for easier adaptation of the API if we wanted to do
a change in the future since the primary functionality remains intact, or to do
optimizations like stack pruning (if we decided to go down that route).
Given the above, v3 is a reroll of the same design as in v2. This is a good
milestone in improving the attribute system as it achieves the goal of making
the attribute subsystem thread-safe (ie multiple callers can be executing
inside the attribute system at the same time) and will enable a future series
to allow pathspec code to call into the attribute system.
Most of the changes in this revision are cosmetic (variable renames, code
movement, etc) but there was a memory leak that was also fixed.
Brandon Williams (8):
attr: pass struct attr_check to collect_some_attrs
attr: use hashmap for attribute dictionary
attr: eliminate global check_all_attr array
attr: remove maybe-real, maybe-macro from git_attr
attr: tighten const correctness with git_attr and match_attr
attr: store attribute stack in attr_check structure
attr: push the bare repo check into read_attr()
attr: reformat git_attr_set_direction() function
Junio C Hamano (17):
commit.c: use strchrnul() to scan for one line
attr.c: use strchrnul() to scan for one line
attr.c: update a stale comment on "struct match_attr"
attr.c: explain the lack of attr-name syntax check in parse_attr()
attr.c: complete a sentence in a comment
attr.c: mark where #if DEBUG ends more clearly
attr.c: simplify macroexpand_one()
attr.c: tighten constness around "git_attr" structure
attr.c: plug small leak in parse_attr_line()
attr.c: add push_stack() helper
attr.c: outline the future plans by heavily commenting
attr: rename function and struct related to checking attributes
attr: (re)introduce git_check_attr() and struct attr_check
attr: convert git_all_attrs() to use "struct attr_check"
attr: convert git_check_attrs() callers to use the new API
attr: retire git_check_attrs() API
attr: change validity check for attribute names to use positive logic
Nguyễn Thái Ngọc Duy (1):
attr: support quoting pathname patterns in C style
Stefan Beller (1):
Documentation: fix a typo
Documentation/gitattributes.txt | 10 +-
Documentation/technical/api-gitattributes.txt | 86 ++-
archive.c | 24 +-
attr.c | 878 ++++++++++++++++++--------
attr.h | 49 +-
builtin/check-attr.c | 66 +-
builtin/pack-objects.c | 19 +-
commit.c | 3 +-
common-main.c | 3 +
convert.c | 25 +-
ll-merge.c | 33 +-
t/t0003-attributes.sh | 26 +
userdiff.c | 19 +-
ws.c | 19 +-
14 files changed, 816 insertions(+), 444 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* Re: [PATCH 1/2] pack-objects: enforce --depth limit in reused deltas
From: Junio C Hamano @ 2017-01-28 0:27 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20170128000959.l7aztgu46ytkhmk3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> > +# On the receiving end, "index-pack --fix-thin" will
>> > +# complete the pack with a base copy of tree X-1.
>>
>> blob? tree? I think the argument would work the same way for either
>> type of objects, but the previous paragraph is using blob as the
>> example, so s/tree/blob/ here?
>
> Oops, yes. I had originally sketched out the example to use trees, but
> it was easier to assemble with blobs so I switched (I never actually dug
> into my "wild" case to see what it was segfaulting on, but I agree it
> applies equally well to either).
OK, then I'll squash in s/tree/blob/ here, in addition to the "my"
thing. Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] reset: support the --stdin option
From: Junio C Hamano @ 2017-01-28 0:20 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <20170127221221.d53icsq7mdkbqm23@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think a lot of the documentation uses <paths> to refer to pathspecs
> (e.g., git-log(1), git-diff(1), etc). As long as we're consistent with
> that convention, I don't think it's that big a deal.
>
> This spot needs a specific mention because it violates the convention.
Yup, I think we are in agreement.
> I don't know if the are other spots where it might be unclear, but I
> think we're probably better to tighten those as they come up, rather
> than switch to saying "<pathspecs>" everywhere.
Oh, I do not think I would disagree. As I think this change is an
instancethat makes the resulting text unclear, it would set a good
example to tighten existing one as part of its clean-up.
It can be done as a follow-up bugfix patch (i.e. "previous one made
the resulting document uncleasr and here is to fix it"), but that
would not serve as good ra ole model to mentor newer contributor as
doing the other way around.
^ permalink raw reply
* Re: [PATCH 1/2] pack-objects: enforce --depth limit in reused deltas
From: Jeff King @ 2017-01-28 0:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <xmqqinp0xep3.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 27, 2017 at 03:31:36PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Since 898b14c (pack-objects: rework check_delta_limit usage,
> > 2007-04-16), we check the delta depth limit only when
> > figuring out whether we should make a new delta. We don't
> > consider it at all when reusing deltas, which means that
> > packing once with --depth=250, and then again with
> > --depth=50, the second pack my still contain chains larger
> > than 50.
>
> "may still contain", methinks.
Oops, yes. There was another typo there that I fixed while proofreading,
and clearly I made it worse. :-/
> > This patch bounds the length of delta chains in the output
> > pack based on --depth, regardless of whether they are caused
> > by cross-pack deltas or existed in the input packs. This
> > fixes the problem, but does have two possible downsides:
> >
> > 1. High-depth aggressive repacks followed by "normal"
> > repacks will throw away the high-depth chains.
>
> I actually think it is a feature that the normal one that runs later
> is allowed to fix an over-deep delta chain.
Yeah, I saw you expressing that sentiment in the earlier thread, and I
think I agree with it.
The big problem to me is mostly that people may be surprised by the
change of behavior if they have a complicated setup. But those people
read the release notes, right? ;)
Arguably setting gc.aggressiveDepth is a mistake after this patch (you
should just set pack.depth). Possibly we should ignore it with a
warning.
> > +# On the receiving end, "index-pack --fix-thin" will
> > +# complete the pack with a base copy of tree X-1.
>
> blob? tree? I think the argument would work the same way for either
> type of objects, but the previous paragraph is using blob as the
> example, so s/tree/blob/ here?
Oops, yes. I had originally sketched out the example to use trees, but
it was easier to assemble with blobs so I switched (I never actually dug
into my "wild" case to see what it was segfaulting on, but I agree it
applies equally well to either).
> > +# Note that this whole setup is pretty reliant on the current
> > +# packing heuristics. We double-check that our test case
> > +# actually produces a long chain. If it doesn't, it should be
> > +# adjusted (or scrapped if the heuristics have become too unreliable)
>
> IOW, we want something that says "we first check X and if X still
> holds, then we expect Y to also hold; if X no longer hold, do not
> bother to test that Y holds". Nice food for thought. Perhaps we
> want a way to express that in our test framework, or is X in the
> above merely another way to say "prerequisite"?
It _is_ a prerequisite, but I think unlike our normal prerequisites, we
wouldn't want to just quietly skip the test if it fails. Because it's
not "oops, this system doesn't support this test" so much as "something
in Git changed, and a human needs to evaluate whether this test can
still be performed".
So I hoped that if that prerequisite test ever broke due to a change in
pack-objects, the person making the change would find the comment and
decide the appropriate next step.
I'll include a version below fixing the typos you found, in case you did
not just mark them up already.
-- >8 --
Subject: [PATCH] pack-objects: enforce --depth limit in reused deltas
Since 898b14c (pack-objects: rework check_delta_limit usage,
2007-04-16), we check the delta depth limit only when
figuring out whether we should make a new delta. We don't
consider it at all when reusing deltas, which means that
packing once with --depth=250, and then again with
--depth=50, the second pack may still contain chains larger
than 50.
This is generally considered a feature, as the results of
earlier high-depth repacks are carried forward, used for
serving fetches, etc. However, since we started using
cross-pack deltas in c9af708b1 (pack-objects: use mru list
when iterating over packs, 2016-08-11), we are no longer
bounded by the length of an existing delta chain in a single
pack.
Here's one particular pathological case: a sequence of N
packs, each with 2 objects, the base of which is stored as a
delta in a previous pack. If we chain all the deltas
together, we have a cycle of length N. We break the cycle,
but the tip delta is still at depth N-1.
This is less unlikely than it might sound. See the included
test for a reconstruction based on real-world actions. I
ran into such a case in the wild, where a client was rapidly
sending packs, and we had accumulated 10,000 before doing a
server-side repack. The pack that "git repack" tried to
generate had a very deep chain, which caused pack-objects to
run out of stack space in the recursive write_one().
This patch bounds the length of delta chains in the output
pack based on --depth, regardless of whether they are caused
by cross-pack deltas or existed in the input packs. This
fixes the problem, but does have two possible downsides:
1. High-depth aggressive repacks followed by "normal"
repacks will throw away the high-depth chains.
In the long run this is probably OK; investigation
showed that high-depth repacks aren't actually
beneficial, and we dropped the aggressive depth default
to match the normal case in 07e7dbf0d (gc: default
aggressive depth to 50, 2016-08-11).
2. If you really do want to store high-depth deltas on
disk, they may be discarded and new delta computed when
serving a fetch, unless you set pack.depth to match
your high-depth size.
The implementation uses the existing search for delta
cycles. That lets us compute the depth of any node based on
the depth of its base, because we know the base is DFS_DONE
by the time we look at it (modulo any cycles in the graph,
but we know there cannot be any because we break them as we
see them).
There is some subtlety worth mentioning, though. We record
the depth of each object as we compute it. It might seem
like we could save the per-object storage space by just
keeping track of the depth of our traversal (i.e., have
break_delta_chains() report how deep it went). But we may
visit an object through multiple delta paths, and on
subsequent paths we want to know its depth immediately,
without having to walk back down to its final base (doing so
would make our graph walk quadratic rather than linear).
Likewise, one could try to record the depth not from the
base, but from our starting point (i.e., start
recursion_depth at 0, and pass "recursion_depth + 1" to each
invocation of break_delta_chains()). And then when
recursion_depth gets too big, we know that we must cut the
delta chain. But that technique is wrong if we do not visit
the nodes in topological order. In a chain A->B->C, it
if we visit "C", then "B", then "A", we will never recurse
deeper than 1 link (because we see at each node that we have
already visited it).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/pack-objects.c | 18 +++++++++
pack-objects.h | 4 ++
t/t5316-pack-delta-depth.sh | 93 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 115 insertions(+)
create mode 100755 t/t5316-pack-delta-depth.sh
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8841f8b36..2b08c8121 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1539,6 +1539,8 @@ static int pack_offset_sort(const void *_a, const void *_b)
* 2. Updating our size/type to the non-delta representation. These were
* either not recorded initially (size) or overwritten with the delta type
* (type) when check_object() decided to reuse the delta.
+ *
+ * 3. Resetting our delta depth, as we are now a base object.
*/
static void drop_reused_delta(struct object_entry *entry)
{
@@ -1552,6 +1554,7 @@ static void drop_reused_delta(struct object_entry *entry)
p = &(*p)->delta_sibling;
}
entry->delta = NULL;
+ entry->depth = 0;
oi.sizep = &entry->size;
oi.typep = &entry->type;
@@ -1570,6 +1573,9 @@ static void drop_reused_delta(struct object_entry *entry)
* Follow the chain of deltas from this entry onward, throwing away any links
* that cause us to hit a cycle (as determined by the DFS state flags in
* the entries).
+ *
+ * We also detect too-long reused chains that would violate our --depth
+ * limit.
*/
static void break_delta_chains(struct object_entry *entry)
{
@@ -1587,6 +1593,18 @@ static void break_delta_chains(struct object_entry *entry)
*/
entry->dfs_state = DFS_ACTIVE;
break_delta_chains(entry->delta);
+
+ /*
+ * Once we've recursed, our base (if we still have one) knows
+ * its depth, so we can compute ours (and check it against
+ * the limit).
+ */
+ if (entry->delta) {
+ entry->depth = entry->delta->depth + 1;
+ if (entry->depth > depth)
+ drop_reused_delta(entry);
+ }
+
entry->dfs_state = DFS_DONE;
break;
diff --git a/pack-objects.h b/pack-objects.h
index cc9b9a9b9..03f119165 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -30,12 +30,16 @@ struct object_entry {
/*
* State flags for depth-first search used for analyzing delta cycles.
+ *
+ * The depth is measured in delta-links to the base (so if A is a delta
+ * against B, then A has a depth of 1, and B a depth of 0).
*/
enum {
DFS_NONE = 0,
DFS_ACTIVE,
DFS_DONE
} dfs_state;
+ int depth;
};
struct packing_data {
diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
new file mode 100755
index 000000000..236d60fe6
--- /dev/null
+++ b/t/t5316-pack-delta-depth.sh
@@ -0,0 +1,93 @@
+#!/bin/sh
+
+test_description='pack-objects breaks long cross-pack delta chains'
+. ./test-lib.sh
+
+# This mirrors a repeated push setup:
+#
+# 1. A client repeatedly modifies some files, makes a
+# commit, and pushes the result. It does this N times
+# before we get around to repacking.
+#
+# 2. Each push generates a thin pack with the new version of
+# various objects. Let's consider some file in the root tree
+# which is updated in each commit.
+#
+# When generating push number X, we feed commit X-1 (and
+# thus blob X-1) as a preferred base. The resulting pack has
+# blob X as a thin delta against blob X-1.
+#
+# On the receiving end, "index-pack --fix-thin" will
+# complete the pack with a base copy of tree X-1.
+#
+# 3. In older versions of git, if we used the delta from
+# pack X, then we'd always find blob X-1 as a base in the
+# same pack (and generate a fresh delta).
+#
+# But with the pack mru, we jump from delta to delta
+# following the traversal order:
+#
+# a. We grab blob X from pack X as a delta, putting it at
+# the tip of our mru list.
+#
+# b. Eventually we move onto commit X-1. We need other
+# objects which are only in pack X-1 (in the test code
+# below, it's the containing tree). That puts pack X-1
+# at the tip of our mru list.
+#
+# c. Eventually we look for blob X-1, and we find the
+# version in pack X-1 (because it's the mru tip).
+#
+# Now we have blob X as a delta against X-1, which is a delta
+# against X-2, and so forth.
+#
+# In the real world, these small pushes would get exploded by
+# unpack-objects rather than "index-pack --fix-thin", but the
+# same principle applies to larger pushes (they only need one
+# repeatedly-modified file to generate the delta chain).
+
+test_expect_success 'create series of packs' '
+ test-genrandom foo 4096 >content &&
+ prev= &&
+ for i in $(test_seq 1 10)
+ do
+ cat content >file &&
+ echo $i >>file &&
+ git add file &&
+ git commit -m $i &&
+ cur=$(git rev-parse HEAD^{tree}) &&
+ {
+ test -n "$prev" && echo "-$prev"
+ echo $cur
+ echo "$(git rev-parse :file) file"
+ } | git pack-objects --stdout >tmp &&
+ git index-pack --stdin --fix-thin <tmp || return 1
+ prev=$cur
+ done
+'
+
+max_chain() {
+ git index-pack --verify-stat-only "$1" >output &&
+ perl -lne '
+ /chain length = (\d+)/ and $len = $1;
+ END { print $len }
+ ' output
+}
+
+# Note that this whole setup is pretty reliant on the current
+# packing heuristics. We double-check that our test case
+# actually produces a long chain. If it doesn't, it should be
+# adjusted (or scrapped if the heuristics have become too unreliable)
+test_expect_success 'packing produces a long delta' '
+ # Use --window=0 to make sure we are seeing reused deltas,
+ # not computing a new long chain.
+ pack=$(git pack-objects --all --window=0 </dev/null pack) &&
+ test 9 = "$(max_chain pack-$pack.pack)"
+'
+
+test_expect_success '--depth limits depth' '
+ pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
+ test 5 = "$(max_chain pack-$pack.pack)"
+'
+
+test_done
--
2.11.0.914.gb3b960f50
^ permalink raw reply related
* Re: Deadlock between git-remote-http and git fetch-pack
From: Junio C Hamano @ 2017-01-27 23:34 UTC (permalink / raw)
To: tsuna; +Cc: git
In-Reply-To: <CAFKYj4cMSK5nQ1nS66c4Opz8y7x+xQH+OdW8PTi7LmCiGBP1ZA@mail.gmail.com>
tsuna <tsunanet@gmail.com> writes:
> While investigating a hung job in our CI system today, I think I found
> a deadlock in git-remote-http
> ...
> Here PID 27319 (git fetch-pack) is stuck reading on stdin, while its
> parent, PID 27317 (git-remote-http) is stuck reading on its child’s
> stdout. Nothing has moved for like 2h, it’s deadlocked.
Hmph, would this be related to 296b847c0d ("remote-curl: don't hang
when a server dies before any output", 2016-11-18) I wonder...
^ permalink raw reply
* Re: [PATCH 1/2] pack-objects: enforce --depth limit in reused deltas
From: Junio C Hamano @ 2017-01-27 23:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20170127220233.mwg36mgxdklwz7lr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Since 898b14c (pack-objects: rework check_delta_limit usage,
> 2007-04-16), we check the delta depth limit only when
> figuring out whether we should make a new delta. We don't
> consider it at all when reusing deltas, which means that
> packing once with --depth=250, and then again with
> --depth=50, the second pack my still contain chains larger
> than 50.
"may still contain", methinks.
> ...
>
> This patch bounds the length of delta chains in the output
> pack based on --depth, regardless of whether they are caused
> by cross-pack deltas or existed in the input packs. This
> fixes the problem, but does have two possible downsides:
>
> 1. High-depth aggressive repacks followed by "normal"
> repacks will throw away the high-depth chains.
I actually think it is a feature that the normal one that runs later
is allowed to fix an over-deep delta chain.
> 2. If you really do want to store high-depth deltas on
> disk, they may be discarded and new delta computed when
> serving a fetch, unless you set pack.depth to match
> your high-depth size.
Likewise.
> ... But we may
> visit an object through multiple delta paths, ...
Yes, good thinking.
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 8841f8b36..2b08c8121 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1539,6 +1539,8 @@ static int pack_offset_sort(const void *_a, const void *_b)
> * 2. Updating our size/type to the non-delta representation. These were
> * either not recorded initially (size) or overwritten with the delta type
> * (type) when check_object() decided to reuse the delta.
> + *
> + * 3. Resetting our delta depth, as we are now a base object.
> */
> static void drop_reused_delta(struct object_entry *entry)
> {
> @@ -1552,6 +1554,7 @@ static void drop_reused_delta(struct object_entry *entry)
> p = &(*p)->delta_sibling;
> }
> entry->delta = NULL;
> + entry->depth = 0;
>
> oi.sizep = &entry->size;
> oi.typep = &entry->type;
Makes sense.
> static void break_delta_chains(struct object_entry *entry)
> {
> @@ -1587,6 +1593,18 @@ static void break_delta_chains(struct object_entry *entry)
> */
> entry->dfs_state = DFS_ACTIVE;
> break_delta_chains(entry->delta);
> +
> + /*
> + * Once we've recursed, our base (if we still have one) knows
> + * its depth, so we can compute ours (and check it against
> + * the limit).
> + */
> + if (entry->delta) {
> + entry->depth = entry->delta->depth + 1;
> + if (entry->depth > depth)
> + drop_reused_delta(entry);
> + }
;-) Surprisingly simpple and effective.
> diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
> new file mode 100755
> index 000000000..236d60fe6
> --- /dev/null
> +++ b/t/t5316-pack-delta-depth.sh
> @@ -0,0 +1,93 @@
> +#!/bin/sh
> +
> +test_description='pack-objects breaks long cross-pack delta chains'
> +. ./test-lib.sh
> +
> +# This mirrors a repeated push setup:
> +#
> +# 1. A client repeatedly modifies some files, makes a
> +# commit, and pushes the result. It does this N times
> +# before we get around to repacking.
> +#
> +# 2. Each push generates a thin pack with the new version of
> +# various objects. Let's consider some file in the root tree
> +# which is updated in each commit.
> +#
> +# When generating push number X, we feed commit X-1 (and
> +# thus blob X-1) as a preferred base. The resulting pack has
> +# blob X as a thin delta against blob X-1.
> +#
> +# On the receiving end, "index-pack --fix-thin" will
> +# complete the pack with a base copy of tree X-1.
blob? tree? I think the argument would work the same way for either
type of objects, but the previous paragraph is using blob as the
example, so s/tree/blob/ here?
> +# 3. In older versions of git, if we used the delta from
> +# pack X, then we'd always find blob X-1 as a base in the
> +# same pack (and generate a fresh delta).
> +#
> +# But with the pack mru, we jump from delta to delta
> +# following the traversal order:
> +# ...
> +# Now we have blob X as a delta against X-1, which is a delta
> +# against X-2, and so forth.
> +# Note that this whole setup is pretty reliant on the current
> +# packing heuristics. We double-check that our test case
> +# actually produces a long chain. If it doesn't, it should be
> +# adjusted (or scrapped if the heuristics have become too unreliable)
IOW, we want something that says "we first check X and if X still
holds, then we expect Y to also hold; if X no longer hold, do not
bother to test that Y holds". Nice food for thought. Perhaps we
want a way to express that in our test framework, or is X in the
above merely another way to say "prerequisite"?
> +test_expect_success 'packing produces a long delta' '
> + # Use --window=0 to make sure we are seeing reused deltas,
> + # not computing a new long chain.
> + pack=$(git pack-objects --all --window=0 </dev/null pack) &&
> + test 9 = "$(max_chain pack-$pack.pack)"
> +'
> +
> +test_expect_success '--depth limits depth' '
> + pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
> + test 5 = "$(max_chain pack-$pack.pack)"
> +'
> +
> +test_done
^ permalink raw reply
* Re: Deadlock between git-remote-http and git fetch-pack
From: Jonathan Tan @ 2017-01-27 23:19 UTC (permalink / raw)
To: tsuna, git
In-Reply-To: <CAFKYj4cMSK5nQ1nS66c4Opz8y7x+xQH+OdW8PTi7LmCiGBP1ZA@mail.gmail.com>
On 01/27/2017 02:31 PM, tsuna wrote:
> Hi there,
> While investigating a hung job in our CI system today, I think I found
> a deadlock in git-remote-http
>
> Git version: 2.9.3
> Linux (amd64) kernel 4.9.0
>
> Excerpt from the process list:
>
> jenkins 27316 0.0 0.0 18508 6024 ? S 19:30 0:00 |
> \_ git -C ../../../arista fetch --unshallow
> jenkins 27317 0.0 0.0 169608 10916 ? S 19:30 0:00 |
> \_ git-remote-http origin http://gerrit/arista
> jenkins 27319 0.0 0.0 24160 8260 ? S 19:30 0:00 |
> \_ git fetch-pack --stateless-rpc --stdin
> --lock-pack --include-tag --thin --no-progress --depth=2147483647
> http://gerrit/arista/
>
> Here PID 27319 (git fetch-pack) is stuck reading on stdin, while its
> parent, PID 27317 (git-remote-http) is stuck reading on its child’s
> stdout. Nothing has moved for like 2h, it’s deadlocked.
>
>> strace -fp 27319
> strace: Process 27319 attached
> read(0,
>
> Here FD 0 is a pipe:
>
> ~ @8a33a534e2f7> lsof -np 27319 | grep 0r
> git 27319 jenkins 0r FIFO 0,10 0t0 354519158 pipe
>
> The writing end of which is owned by the parent process:
>
> ~ @8a33a534e2f7> lsof -n 2>/dev/null | fgrep 354519158
> git-remot 27317 jenkins 4w FIFO 0,10 0t0
> 354519158 pipe
> git 27319 jenkins 0r FIFO 0,10 0t0
> 354519158 pipe
>
> And the parent process (git-remote-http) is stuck reading from another FD:
>
>> strace -fp 27317
> strace: Process 27317 attached
> read(5,
>
> And here FD 5 is another pipe:
>
> ~ @8a33a534e2f7> lsof -np 27317 | grep 5r
> git-remot 27317 jenkins 5r FIFO 0,10 0t0 354519159 pipe
>
> Which is the child’s stdout:
>
>> lsof -n 2>/dev/null | fgrep 354519159
> git-remot 27317 jenkins 5r FIFO 0,10 0t0
> 354519159 pipe
> git 27319 jenkins 1w FIFO 0,10 0t0
> 354519159 pipe
>
> Hence the deadlock.
>
> Stack trace in git-remote-http:
>
> (gdb) bt
> #0 0x00007f04f1e1363d in read () from target:/lib64/libpthread.so.0
> #1 0x0000562417472d73 in xread ()
> #2 0x0000562417472f2b in read_in_full ()
> #3 0x0000562417438a6e in get_packet_data ()
> #4 0x0000562417439129 in packet_read ()
> #5 0x00005624174245e0 in rpc_service ()
> #6 0x0000562417424f10 in fetch_git ()
> #7 0x00005624174233fd in main ()
>
> Stack trace in git fetch-pack:
>
> (gdb) bt
> #0 0x00007fb3ab478620 in __read_nocancel () from target:/lib64/libpthread.so.0
> #1 0x000055f688827283 in xread ()
> #2 0x000055f68882743b in read_in_full ()
> #3 0x000055f6887ce35e in get_packet_data ()
> #4 0x000055f6887cea19 in packet_read ()
> #5 0x000055f6887ceb90 in packet_read_line ()
> #6 0x000055f68879dd05 in get_ack ()
> #7 0x000055f68879f6b4 in fetch_pack ()
> #8 0x000055f688710619 in cmd_fetch_pack ()
> #9 0x000055f6886dff7b in handle_builtin ()
> #10 0x000055f6886df026 in main ()
>
> I looked at the diff between v2.9.3 and HEAD on fetch-pack.c and
> remote-curl.c and didn’t see anything noteworthy in that area of the
> code, so I presume the bug is still there in master.
>
I haven't looked into this in detail, but this might be related to
something I discovered while writing my patch set. I noticed that
upload-pack (the process on the "other side" of fetch-pack) can die
without first writing any notification, causing fetch-pack to block
forever on a read. A fix would probably look like that patch [1].
[1]
<afe5d7d3f876893fdad318665805df1e056717c6.1485381677.git.jonathantanmy@google.com>
^ permalink raw reply
* Deadlock between git-remote-http and git fetch-pack
From: tsuna @ 2017-01-27 22:31 UTC (permalink / raw)
To: git
Hi there,
While investigating a hung job in our CI system today, I think I found
a deadlock in git-remote-http
Git version: 2.9.3
Linux (amd64) kernel 4.9.0
Excerpt from the process list:
jenkins 27316 0.0 0.0 18508 6024 ? S 19:30 0:00 |
\_ git -C ../../../arista fetch --unshallow
jenkins 27317 0.0 0.0 169608 10916 ? S 19:30 0:00 |
\_ git-remote-http origin http://gerrit/arista
jenkins 27319 0.0 0.0 24160 8260 ? S 19:30 0:00 |
\_ git fetch-pack --stateless-rpc --stdin
--lock-pack --include-tag --thin --no-progress --depth=2147483647
http://gerrit/arista/
Here PID 27319 (git fetch-pack) is stuck reading on stdin, while its
parent, PID 27317 (git-remote-http) is stuck reading on its child’s
stdout. Nothing has moved for like 2h, it’s deadlocked.
> strace -fp 27319
strace: Process 27319 attached
read(0,
Here FD 0 is a pipe:
~ @8a33a534e2f7> lsof -np 27319 | grep 0r
git 27319 jenkins 0r FIFO 0,10 0t0 354519158 pipe
The writing end of which is owned by the parent process:
~ @8a33a534e2f7> lsof -n 2>/dev/null | fgrep 354519158
git-remot 27317 jenkins 4w FIFO 0,10 0t0
354519158 pipe
git 27319 jenkins 0r FIFO 0,10 0t0
354519158 pipe
And the parent process (git-remote-http) is stuck reading from another FD:
> strace -fp 27317
strace: Process 27317 attached
read(5,
And here FD 5 is another pipe:
~ @8a33a534e2f7> lsof -np 27317 | grep 5r
git-remot 27317 jenkins 5r FIFO 0,10 0t0 354519159 pipe
Which is the child’s stdout:
> lsof -n 2>/dev/null | fgrep 354519159
git-remot 27317 jenkins 5r FIFO 0,10 0t0
354519159 pipe
git 27319 jenkins 1w FIFO 0,10 0t0
354519159 pipe
Hence the deadlock.
Stack trace in git-remote-http:
(gdb) bt
#0 0x00007f04f1e1363d in read () from target:/lib64/libpthread.so.0
#1 0x0000562417472d73 in xread ()
#2 0x0000562417472f2b in read_in_full ()
#3 0x0000562417438a6e in get_packet_data ()
#4 0x0000562417439129 in packet_read ()
#5 0x00005624174245e0 in rpc_service ()
#6 0x0000562417424f10 in fetch_git ()
#7 0x00005624174233fd in main ()
Stack trace in git fetch-pack:
(gdb) bt
#0 0x00007fb3ab478620 in __read_nocancel () from target:/lib64/libpthread.so.0
#1 0x000055f688827283 in xread ()
#2 0x000055f68882743b in read_in_full ()
#3 0x000055f6887ce35e in get_packet_data ()
#4 0x000055f6887cea19 in packet_read ()
#5 0x000055f6887ceb90 in packet_read_line ()
#6 0x000055f68879dd05 in get_ack ()
#7 0x000055f68879f6b4 in fetch_pack ()
#8 0x000055f688710619 in cmd_fetch_pack ()
#9 0x000055f6886dff7b in handle_builtin ()
#10 0x000055f6886df026 in main ()
I looked at the diff between v2.9.3 and HEAD on fetch-pack.c and
remote-curl.c and didn’t see anything noteworthy in that area of the
code, so I presume the bug is still there in master.
--
Benoit "tsuna" Sigoure
^ permalink raw reply
* Re: [PATCH v2 1/1] reset: support the --stdin option
From: Jeff King @ 2017-01-27 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <xmqqh94kz76v.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 27, 2017 at 10:30:48AM -0800, Junio C Hamano wrote:
> > Is it worth clarifying that these are paths, not pathspecs? The word
> > "paths" is used to refer to the pathspecs on the command-line elsewhere
> > in the document.
>
> If the code forces literal pathspecs, then what the user feeds to
> the command is no longer pathspecs from the user's point of view,
> and I agree that the distinction is important.
>
> If the remainder of the documentation is loose in terminology and
> the reason why we were able to get away with it was because we
> consistently used "paths" when we meant "pathspec", these existing
> mention of "paths" have to be tightened, either in this patch or a
> preparatory step patch before this one, because the addition of
> "this thing reads paths not pathspecs" is what makes them ambiguous.
I think a lot of the documentation uses <paths> to refer to pathspecs
(e.g., git-log(1), git-diff(1), etc). As long as we're consistent with
that convention, I don't think it's that big a deal.
This spot needs a specific mention because it violates the convention.
I don't know if the are other spots where it might be unclear, but I
think we're probably better to tighten those as they come up, rather
than switch to saying "<pathspecs>" everywhere.
That's outside the scope of this series, though, I would think.
-Peff
^ permalink raw reply
* [PATCH 2/2] pack-objects: convert recursion to iteration in break_delta_chain()
From: Jeff King @ 2017-01-27 22:05 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <20170127220143.boo5phhgogqlucsj@sigill.intra.peff.net>
The break_delta_chain() function is recursive over the depth
of a given delta chain, which can lead to possibly running
out of stack space. Normally delta depth is quite small, but
if there _is_ a pathological case, this is where we would
find and fix it, so we should be more careful.
We can do it without recursion at all, but there's a little
bit of cleverness needed to do so. It's easiest to explain
by covering the less-clever strategies first.
The obvious thing to try is just keeping our own stack on
the heap. Whenever we would recurse, push the new entry onto
the stack and loop instead. But this gets tricky; when we
see an ACTIVE entry, we need to care if we just pushed it
(in which case it's a cycle) or if we just popped it (in
which case we dealt with its bases, and no we need to clear
the ACTIVE flag and compute its depth).
You can hack around that in various ways, like keeping a
"just pushed" flag, but the logic gets muddled. However, we
can observe that we do all of our pushes first, and then all
of our pops afterwards. In other words, we can do this in
two passes. First dig down to the base, stopping when we see
a cycle, and pushing each item onto our stack. Then pop the
stack elements, clearing the ACTIVE flag and computing the
depth for each.
This works, and is reasonably elegant. However, why do we
need the stack for the second pass? We can just walk the
delta pointers again. There's one complication. Popping the
stack went over our list in reverse, so we could compute the
depth of each entry by incrementing the depth of its base,
which we will have just computed. To go forward in the
second pass, we have to compute the total depth on the way
down, and then assign it as we go.
This patch implements this final strategy, because it not
only keeps the memory off the stack, but it eliminates it
entirely. Credit for the cleverness in that approach goes to
Michael Haggerty; bugs are mine.
Signed-off-by: Jeff King <peff@peff.net>
---
The diff is nearly impossible to read, so I'd recommend just looking at
the end result. I tried to document the tricky parts with comments.
There are a few parts that could be made more terse, but I screwed it up
so many times while writing it that I decided to do it in a way that
carefully documents all of the assumptions.
builtin/pack-objects.c | 129 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 99 insertions(+), 30 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 2b08c8121..c7af47548 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1579,48 +1579,117 @@ static void drop_reused_delta(struct object_entry *entry)
*/
static void break_delta_chains(struct object_entry *entry)
{
- /* If it's not a delta, it can't be part of a cycle. */
- if (!entry->delta) {
- entry->dfs_state = DFS_DONE;
- return;
- }
+ /*
+ * The actual depth of each object we will write is stored as an int,
+ * as it cannot exceed our int "depth" limit. But before we break
+ * changes based no that limit, we may potentially go as deep as the
+ * number of objects, which is elsewhere bounded to a uint32_t.
+ */
+ uint32_t total_depth;
+ struct object_entry *cur, *next;
+
+ for (cur = entry, total_depth = 0;
+ cur;
+ cur = cur->delta, total_depth++) {
+ if (cur->dfs_state == DFS_DONE) {
+ /*
+ * We've already seen this object and know it isn't
+ * part of a cycle. We do need to append its depth
+ * to our count.
+ */
+ total_depth += cur->depth;
+ break;
+ }
- switch (entry->dfs_state) {
- case DFS_NONE:
/*
- * This is the first time we've seen the object. We mark it as
- * part of the active potential cycle and recurse.
+ * We break cycles before looping, so an ACTIVE state (or any
+ * other cruft which made its way into the state variable)
+ * is a bug.
*/
- entry->dfs_state = DFS_ACTIVE;
- break_delta_chains(entry->delta);
+ if (cur->dfs_state != DFS_NONE)
+ die("BUG: confusing delta dfs state in first pass: %d",
+ cur->dfs_state);
/*
- * Once we've recursed, our base (if we still have one) knows
- * its depth, so we can compute ours (and check it against
- * the limit).
+ * Now we know this is the first time we've seen the object. If
+ * it's not a delta, we're done traversing, but we'll mark it
+ * done to save time on future traversals.
*/
- if (entry->delta) {
- entry->depth = entry->delta->depth + 1;
- if (entry->depth > depth)
- drop_reused_delta(entry);
+ if (!cur->delta) {
+ cur->dfs_state = DFS_DONE;
+ break;
}
- entry->dfs_state = DFS_DONE;
- break;
+ /*
+ * Mark ourselves as active and see if the next step causes
+ * us to cycle to another active object. It's important to do
+ * this _before_ we loop, because it impacts where we make the
+ * cut, and thus how our total_depth counter works.
+ * E.g., We may see a partial loop like:
+ *
+ * A -> B -> C -> D -> B
+ *
+ * Cutting B->C breaks the cycle. But now the depth of A is
+ * only 1, and our total_depth counter is at 3. The size of the
+ * error is always one less than the size of the cycle we
+ * broke. Commits C and D were "lost" from A's chain.
+ *
+ * If we instead cut D->B, then the depth of A is correct at 3.
+ * We keep all commits in the chain that we examined.
+ */
+ cur->dfs_state = DFS_ACTIVE;
+ if (cur->delta->dfs_state == DFS_ACTIVE) {
+ drop_reused_delta(cur);
+ cur->dfs_state = DFS_DONE;
+ break;
+ }
+ }
- case DFS_DONE:
- /* object already examined, and not part of a cycle */
- break;
+ /*
+ * And now that we've gone all the way to the bottom of the chain, we
+ * need to clear the active flags and set the depth fields as
+ * appropriate. Unlike the loop above, which can quit when it drops a
+ * delta, we need to keep going to look for more depth cuts. So we need
+ * an extra "next" pointer to keep going after we reset cur->delta.
+ */
+ for (cur = entry; cur; cur = next) {
+ next = cur->delta;
- case DFS_ACTIVE:
/*
- * We found a cycle that needs broken. It would be correct to
- * break any link in the chain, but it's convenient to
- * break this one.
+ * We should have a chain of zero or more ACTIVE states down to
+ * a final DONE. We can quit after the DONE, because either it
+ * has no bases, or we've already handled them in a previous
+ * call.
*/
- drop_reused_delta(entry);
- entry->dfs_state = DFS_DONE;
- break;
+ if (cur->dfs_state == DFS_DONE)
+ break;
+ else if (cur->dfs_state != DFS_ACTIVE)
+ die("BUG: confusing delta dfs state in second pass: %d",
+ cur->dfs_state);
+
+ /*
+ * If the total_depth is more than depth, then we need to snip
+ * the chain into two or more smaller chains that don't exceed
+ * the maximum depth. Most of the resulting chains will contain
+ * (depth + 1) entries (i.e., depth deltas plus one base), and
+ * the last chain (i.e., the one containing entry) will contain
+ * whatever entries are left over, namely
+ * (total_depth % (depth + 1)) of them.
+ *
+ * Since we are iterating towards decreasing depth, we need to
+ * decrement total_depth as we go, and we need to write to the
+ * entry what its final depth will be after all of the
+ * snipping. Since we're snipping into chains of length (depth
+ * + 1) entries, the final depth of an entry will be its
+ * original depth modulo (depth + 1). Any time we encounter an
+ * entry whose final depth is supposed to be zero, we snip it
+ * from its delta base, thereby making it so.
+ */
+ cur->depth = (total_depth--) % (depth + 1);
+ if (!cur->depth)
+ drop_reused_delta(cur);
+
+ cur->dfs_state = DFS_DONE;
}
}
--
2.11.0.914.gb3b960f50
^ permalink raw reply related
* [PATCH 1/2] pack-objects: enforce --depth limit in reused deltas
From: Jeff King @ 2017-01-27 22:02 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <20170127220143.boo5phhgogqlucsj@sigill.intra.peff.net>
Since 898b14c (pack-objects: rework check_delta_limit usage,
2007-04-16), we check the delta depth limit only when
figuring out whether we should make a new delta. We don't
consider it at all when reusing deltas, which means that
packing once with --depth=250, and then again with
--depth=50, the second pack my still contain chains larger
than 50.
This is generally considered a feature, as the results of
earlier high-depth repacks are carried forward, used for
serving fetches, etc. However, since we started using
cross-pack deltas in c9af708b1 (pack-objects: use mru list
when iterating over packs, 2016-08-11), we are no longer
bounded by the length of an existing delta chain in a single
pack.
Here's one particular pathological case: a sequence of N
packs, each with 2 objects, the base of which is stored as a
delta in a previous pack. If we chain all the deltas
together, we have a cycle of length N. We break the cycle,
but the tip delta is still at depth N-1.
This is less unlikely than it might sound. See the included
test for a reconstruction based on real-world actions. I
ran into such a case in the wild, where a client was rapidly
sending packs, and we had accumulated 10,000 before doing a
server-side repack. The pack that "git repack" tried to
generate had a very deep chain, which caused pack-objects to
run out of stack space in the recursive write_one().
This patch bounds the length of delta chains in the output
pack based on --depth, regardless of whether they are caused
by cross-pack deltas or existed in the input packs. This
fixes the problem, but does have two possible downsides:
1. High-depth aggressive repacks followed by "normal"
repacks will throw away the high-depth chains.
In the long run this is probably OK; investigation
showed that high-depth repacks aren't actually
beneficial, and we dropped the aggressive depth default
to match the normal case in 07e7dbf0d (gc: default
aggressive depth to 50, 2016-08-11).
2. If you really do want to store high-depth deltas on
disk, they may be discarded and new delta computed when
serving a fetch, unless you set pack.depth to match
your high-depth size.
The implementation uses the existing search for delta
cycles. That lets us compute the depth of any node based on
the depth of its base, because we know the base is DFS_DONE
by the time we look at it (modulo any cycles in the graph,
but we know there cannot be any because we break them as we
see them).
There is some subtlety worth mentioning, though. We record
the depth of each object as we compute it. It might seem
like we could save the per-object storage space by just
keeping track of the depth of our traversal (i.e., have
break_delta_chains() report how deep it went). But we may
visit an object through multiple delta paths, and on
subsequent paths we want to know its depth immediately,
without having to walk back down to its final base (doing so
would make our graph walk quadratic rather than linear).
Likewise, one could try to record the depth not from the
base, but from our starting point (i.e., start
recursion_depth at 0, and pass "recursion_depth + 1" to each
invocation of break_delta_chains()). And then when
recursion_depth gets too big, we know that we must cut the
delta chain. But that technique is wrong if we do not visit
the nodes in topological order. In a chain A->B->C, it
if we visit "C", then "B", then "A", we will never recurse
deeper than 1 link (because we see at each node that we have
already visited it).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/pack-objects.c | 18 +++++++++
pack-objects.h | 4 ++
t/t5316-pack-delta-depth.sh | 93 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 115 insertions(+)
create mode 100755 t/t5316-pack-delta-depth.sh
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8841f8b36..2b08c8121 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1539,6 +1539,8 @@ static int pack_offset_sort(const void *_a, const void *_b)
* 2. Updating our size/type to the non-delta representation. These were
* either not recorded initially (size) or overwritten with the delta type
* (type) when check_object() decided to reuse the delta.
+ *
+ * 3. Resetting our delta depth, as we are now a base object.
*/
static void drop_reused_delta(struct object_entry *entry)
{
@@ -1552,6 +1554,7 @@ static void drop_reused_delta(struct object_entry *entry)
p = &(*p)->delta_sibling;
}
entry->delta = NULL;
+ entry->depth = 0;
oi.sizep = &entry->size;
oi.typep = &entry->type;
@@ -1570,6 +1573,9 @@ static void drop_reused_delta(struct object_entry *entry)
* Follow the chain of deltas from this entry onward, throwing away any links
* that cause us to hit a cycle (as determined by the DFS state flags in
* the entries).
+ *
+ * We also detect too-long reused chains that would violate our --depth
+ * limit.
*/
static void break_delta_chains(struct object_entry *entry)
{
@@ -1587,6 +1593,18 @@ static void break_delta_chains(struct object_entry *entry)
*/
entry->dfs_state = DFS_ACTIVE;
break_delta_chains(entry->delta);
+
+ /*
+ * Once we've recursed, our base (if we still have one) knows
+ * its depth, so we can compute ours (and check it against
+ * the limit).
+ */
+ if (entry->delta) {
+ entry->depth = entry->delta->depth + 1;
+ if (entry->depth > depth)
+ drop_reused_delta(entry);
+ }
+
entry->dfs_state = DFS_DONE;
break;
diff --git a/pack-objects.h b/pack-objects.h
index cc9b9a9b9..03f119165 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -30,12 +30,16 @@ struct object_entry {
/*
* State flags for depth-first search used for analyzing delta cycles.
+ *
+ * The depth is measured in delta-links to the base (so if A is a delta
+ * against B, then A has a depth of 1, and B a depth of 0).
*/
enum {
DFS_NONE = 0,
DFS_ACTIVE,
DFS_DONE
} dfs_state;
+ int depth;
};
struct packing_data {
diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
new file mode 100755
index 000000000..236d60fe6
--- /dev/null
+++ b/t/t5316-pack-delta-depth.sh
@@ -0,0 +1,93 @@
+#!/bin/sh
+
+test_description='pack-objects breaks long cross-pack delta chains'
+. ./test-lib.sh
+
+# This mirrors a repeated push setup:
+#
+# 1. A client repeatedly modifies some files, makes a
+# commit, and pushes the result. It does this N times
+# before we get around to repacking.
+#
+# 2. Each push generates a thin pack with the new version of
+# various objects. Let's consider some file in the root tree
+# which is updated in each commit.
+#
+# When generating push number X, we feed commit X-1 (and
+# thus blob X-1) as a preferred base. The resulting pack has
+# blob X as a thin delta against blob X-1.
+#
+# On the receiving end, "index-pack --fix-thin" will
+# complete the pack with a base copy of tree X-1.
+#
+# 3. In older versions of git, if we used the delta from
+# pack X, then we'd always find blob X-1 as a base in the
+# same pack (and generate a fresh delta).
+#
+# But with the pack mru, we jump from delta to delta
+# following the traversal order:
+#
+# a. We grab blob X from pack X as a delta, putting it at
+# the tip of our mru list.
+#
+# b. Eventually we move onto commit X-1. We need other
+# objects which are only in pack X-1 (in the test code
+# below, it's the containing tree). That puts pack X-1
+# at the tip of our mru list.
+#
+# c. Eventually we look for blob X-1, and we find the
+# version in pack X-1 (because it's the mru tip).
+#
+# Now we have blob X as a delta against X-1, which is a delta
+# against X-2, and so forth.
+#
+# In the real world, these small pushes would get exploded by
+# unpack-objects rather than "index-pack --fix-thin", but the
+# same principle applies to larger pushes (they only need one
+# repeatedly-modified file to generate the delta chain).
+
+test_expect_success 'create series of packs' '
+ test-genrandom foo 4096 >content &&
+ prev= &&
+ for i in $(test_seq 1 10)
+ do
+ cat content >file &&
+ echo $i >>file &&
+ git add file &&
+ git commit -m $i &&
+ cur=$(git rev-parse HEAD^{tree}) &&
+ {
+ test -n "$prev" && echo "-$prev"
+ echo $cur
+ echo "$(git rev-parse :file) file"
+ } | git pack-objects --stdout >tmp &&
+ git index-pack --stdin --fix-thin <tmp || return 1
+ prev=$cur
+ done
+'
+
+max_chain() {
+ git index-pack --verify-stat-only "$1" >output &&
+ perl -lne '
+ /chain length = (\d+)/ and $len = $1;
+ END { print $len }
+ ' output
+}
+
+# Note that this whole setup is pretty reliant on the current
+# packing heuristics. We double-check that our test case
+# actually produces a long chain. If it doesn't, it should be
+# adjusted (or scrapped if the heuristics have become too unreliable)
+test_expect_success 'packing produces a long delta' '
+ # Use --window=0 to make sure we are seeing reused deltas,
+ # not computing a new long chain.
+ pack=$(git pack-objects --all --window=0 </dev/null pack) &&
+ test 9 = "$(max_chain pack-$pack.pack)"
+'
+
+test_expect_success '--depth limits depth' '
+ pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
+ test 5 = "$(max_chain pack-$pack.pack)"
+'
+
+test_done
--
2.11.0.914.gb3b960f50
^ permalink raw reply related
* [PATCH 0/2] limit reused delta chains based on --depth
From: Jeff King @ 2017-01-27 22:01 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
Back when we switched pack-objects to visiting packs in
most-recently-used order last August, we realized that this could reuse
cross-pack deltas, and that the result could have longer delta chains
than any single pack contains.
I produced a patch back then[1], but we decided not to follow through
with it. Two things happened to make me revive that patch:
1. I hit a case in the wild with a really long delta chain that caused
pack-objects' write_one() to run out of stack space.
2. We dropped the --aggressive depth to match the normal one. So
there's less concern about mismatches throwing out on-disk deltas
from a previous aggressive repack (but see the caveats in the first
patch's commit message).
So here it is, plus another patch that converts the recursion to
iteration (the stack space needed by the function is small enough that
just the first patch was enough to fix my problem case, but it seemed
like tempting fate to leave it recursive).
[1/2]: pack-objects: enforce --depth limit in reused deltas
[2/2]: pack-objects: convert recursion to iteration in break_delta_chain()
builtin/pack-objects.c | 133 ++++++++++++++++++++++++++++++++++++--------
pack-objects.h | 4 ++
t/t5316-pack-delta-depth.sh | 93 +++++++++++++++++++++++++++++++
3 files changed, 207 insertions(+), 23 deletions(-)
create mode 100755 t/t5316-pack-delta-depth.sh
-Peff
[1] http://public-inbox.org/git/20160811095710.p2bffympjlwmv3gc@sigill.intra.peff.net/
^ permalink raw reply
* Re: octopus merge --no-ff claims to fast-forward even though it doesn't
From: Junio C Hamano @ 2017-01-27 21:59 UTC (permalink / raw)
To: Samuel Lijin; +Cc: git@vger.kernel.org
In-Reply-To: <CAJZjrdWdRGZ5DC1XV_YiNt-1sKiNgAtiS-eS9L6H2GJ+_8n08w@mail.gmail.com>
Samuel Lijin <sxlijin@gmail.com> writes:
> I was doing an octopus merge earlier and noticed that it claims to
> fast-forward when you specify --no-ff, even though it does actually
> abide by --no-ff.
This was intentional and hasn't changed since it was first designed;
the octopus was to be used only for the simple and obvious merges.
If anything, I think it should error out when the --no-ff option is
given, issuing the same error message as the one given when any step
other than the last one needs manual resolution.
^ permalink raw reply
* Re: [PATCH] doc: clarify distinction between sign-off and pgp-signing
From: Junio C Hamano @ 2017-01-27 21:40 UTC (permalink / raw)
To: Cornelius Weig; +Cc: git@vger.kernel.org
In-Reply-To: <d65bc165-9bd3-c7a6-9a55-1904d1bc095e@tngtech.com>
Cornelius Weig <cornelius.weig@tngtech.com> writes:
> Sorry, I forgot to mark this patch as follow-up to message
> <xmqq60l01jr9.fsf@gitster.mtv.corp.google.com>
I appreciate that you are very considerate, but in practice, if you
do not have too many topics in flight and your response time is less
than 48 hours, we can tell which new message is about which older
discussion thread. Don't worry about it too much.
Thanks.
^ permalink raw reply
* Re: [PATCH] doc: clarify distinction between sign-off and pgp-signing
From: Philip Oakley @ 2017-01-27 21:38 UTC (permalink / raw)
To: Stefan Beller, Cornelius Weig; +Cc: Junio C Hamano, git
In-Reply-To: <CAGZ79kb29usw4WyMdy3c5-rGH1nEcQ1gUabzdAtGgOW9zfTCDA@mail.gmail.com>
From: "Stefan Beller" <sbeller@google.com>
> On Fri, Jan 27, 2017 at 12:01 PM, <cornelius.weig@tngtech.com> wrote:
>> From: Cornelius Weig <cornelius.weig@tngtech.com>
>>
>> The documentation for submission discourages pgp-signing, but demands
>> a proper sign-off by contributors. However, when skimming the headings,
>> the wording of the section for sign-off could mistakenly be understood
>> as concerning pgp-signing. Thus, new contributors could oversee the
>> necessary sign-off.
>>
>> This commit improves the wording such that the section about sign-off
>> cannot be misunderstood as pgp-signing. In addition, the paragraph about
>> pgp-signing is changed such that it avoids the impression that
>> pgp-signing could be relevant at later stages of the submission.
>>
>> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Philip Oakley <philipoakley@iee.org>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>> This patch summarizes the suggested changes.
>>
>> As I don't know what is appropriate, I took the liberty to add
>> everybody's
>> sign-off who was involved in the discussion in alphabetic order.
>
> Heh, my first though was to conclude you haven't read the
> sign off part, yet apart from the changed header.
> /me goes back and actually reads the DCO again.
> And actually these sign offs were there in other patches in this area,
> so you'd claim (a) that yours was just created partly by you and having
> other patches that were also signed off (b), whose sign offs you
> merely copy over to here.
>
> And then after reading I realized I slightly confused the signing
> myself as the sign offs are also used to track the flow of a patch.
> These sign offs suggest that you made the patch initially, then
> passed it to Junio, then to Philip and then to me.
> And Junio will sign it again when applying the patch.
>
> So maybe s/signed-off-by/helped-by/?
Helped-by: Philip Oakley <philipoakley@iee.org>
is sufficient for me (if that).
>
> The patch with the aggregation looks good to me.
>
> Thanks,
> Stefan
>
>>
>> Documentation/SubmittingPatches | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/SubmittingPatches
>> b/Documentation/SubmittingPatches
>> index 08352de..3faf7eb 100644
>> --- a/Documentation/SubmittingPatches
>> +++ b/Documentation/SubmittingPatches
>> @@ -216,12 +216,11 @@ that it will be postponed.
>> Exception: If your mailer is mangling patches then someone may ask
>> you to re-send them using MIME, that is OK.
>>
>> -Do not PGP sign your patch, at least for now. Most likely, your
>> -maintainer or other people on the list would not have your PGP
>> -key and would not bother obtaining it anyway. Your patch is not
>> -judged by who you are; a good patch from an unknown origin has a
>> -far better chance of being accepted than a patch from a known,
>> -respected origin that is done poorly or does incorrect things.
>> +Do not PGP sign your patch. Most likely, your maintainer or other people
>> on the
>> +list would not have your PGP key and would not bother obtaining it
>> anyway.
>> +Your patch is not judged by who you are; a good patch from an unknown
>> origin
>> +has a far better chance of being accepted than a patch from a known,
>> respected
>> +origin that is done poorly or does incorrect things.
>>
>> If you really really really really want to do a PGP signed
>> patch, format it as "multipart/signed", not a text/plain message
>> @@ -246,7 +245,7 @@ patch.
>> *2* The mailing list: git@vger.kernel.org
>>
>>
>> -(5) Sign your work
>> +(5) Certify your work by adding your "Signed-off-by: " line
>>
>> To improve tracking of who did what, we've borrowed the
>> "sign-off" procedure from the Linux kernel project on patches
>> --
>> 2.10.2
>>
>
^ permalink raw reply
* [PATCH v2 0/7] completion: recognize more long-options
From: cornelius.weig @ 2017-01-27 21:17 UTC (permalink / raw)
To: j6t; +Cc: szeder.dev, spearce, git, Cornelius Weig
In-Reply-To: <74ecd09c-55da-3858-5187-52c286a6bf62@kdbg.org>
From: Cornelius Weig <cornelius.weig@tngtech.com>
This revision addresses Johannes' concerns. Changes wrt v1:
- fixed the commit message: two of the "dangerous" options erroneously ended
up in the commit message. These options were already in the list of
auto-completable options.
- removed the possibly dangerous option '--unsafe-paths' from git-apply.
- added my sign-off
Patches 1-6 are not resent, because they have not changed (other than my added sign-off).
Also, I added further people to CC, because nobody actually has looked at the code yet.
Cornelius Weig (7):
completion: recognize more long-options
contrib/completion/git-completion.bash | 132 +++++++++++++++++++++++++++------
1 file changed, 110 insertions(+), 22 deletions(-)
--
2.10.2
^ permalink raw reply
* [PATCH v2 7/7] completion: recognize more long-options
From: cornelius.weig @ 2017-01-27 21:17 UTC (permalink / raw)
To: j6t; +Cc: szeder.dev, spearce, git, Cornelius Weig
In-Reply-To: <20170127211703.24910-1-cornelius.weig@tngtech.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
Recognize several new long-options for bash completion in the following
commands:
- apply: --recount --directory=
- archive: --output
- branch: --column --no-column --sort= --points-at
- clone: --no-single-branch --shallow-submodules
- commit: --patch --short --date --allow-empty
- describe: --first-parent
- fetch, pull: --unshallow --update-shallow
- fsck: --name-objects
- grep: --break --heading --show-function --function-context
--untracked --no-index
- mergetool: --prompt --no-prompt
- reset: --keep
- revert: --strategy= --strategy-option=
- rm: --force
- shortlog: --email
- tag: --merged --no-merged --create-reflog
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
---
contrib/completion/git-completion.bash | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0e09519..933bb6e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -936,6 +936,7 @@ _git_apply ()
--apply --no-add --exclude=
--ignore-whitespace --ignore-space-change
--whitespace= --inaccurate-eof --verbose
+ --recount --directory=
"
return
esac
@@ -974,7 +975,7 @@ _git_archive ()
--*)
__gitcomp "
--format= --list --verbose
- --prefix= --remote= --exec=
+ --prefix= --remote= --exec= --output
"
return
;;
@@ -1029,6 +1030,7 @@ _git_branch ()
--track --no-track --contains --merged --no-merged
--set-upstream-to= --edit-description --list
--unset-upstream --delete --move --remotes
+ --column --no-column --sort= --points-at
"
;;
*)
@@ -1142,6 +1144,8 @@ _git_clone ()
--single-branch
--branch
--recurse-submodules
+ --no-single-branch
+ --shallow-submodules
"
return
;;
@@ -1183,6 +1187,7 @@ _git_commit ()
--reset-author --file= --message= --template=
--cleanup= --untracked-files --untracked-files=
--verbose --quiet --fixup= --squash=
+ --patch --short --date --allow-empty
"
return
esac
@@ -1201,7 +1206,7 @@ _git_describe ()
--*)
__gitcomp "
--all --tags --contains --abbrev= --candidates=
- --exact-match --debug --long --match --always
+ --exact-match --debug --long --match --always --first-parent
"
return
esac
@@ -1284,6 +1289,7 @@ __git_fetch_recurse_submodules="yes on-demand no"
__git_fetch_options="
--quiet --verbose --append --upload-pack --force --keep --depth=
--tags --no-tags --all --prune --dry-run --recurse-submodules=
+ --unshallow --update-shallow
"
_git_fetch ()
@@ -1333,7 +1339,7 @@ _git_fsck ()
--*)
__gitcomp "
--tags --root --unreachable --cache --no-reflogs --full
- --strict --verbose --lost-found
+ --strict --verbose --lost-found --name-objects
"
return
;;
@@ -1377,6 +1383,8 @@ _git_grep ()
--max-depth
--count
--and --or --not --all-match
+ --break --heading --show-function --function-context
+ --untracked --no-index
"
return
;;
@@ -1576,7 +1584,7 @@ _git_mergetool ()
return
;;
--*)
- __gitcomp "--tool="
+ __gitcomp "--tool= --prompt --no-prompt"
return
;;
esac
@@ -2456,7 +2464,7 @@ _git_reset ()
case "$cur" in
--*)
- __gitcomp "--merge --mixed --hard --soft --patch"
+ __gitcomp "--merge --mixed --hard --soft --patch --keep"
return
;;
esac
@@ -2472,7 +2480,10 @@ _git_revert ()
fi
case "$cur" in
--*)
- __gitcomp "--edit --mainline --no-edit --no-commit --signoff"
+ __gitcomp "
+ --edit --mainline --no-edit --no-commit --signoff
+ --strategy= --strategy-option=
+ "
return
;;
esac
@@ -2483,7 +2494,7 @@ _git_rm ()
{
case "$cur" in
--*)
- __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
+ __gitcomp "--cached --dry-run --ignore-unmatch --quiet --force"
return
;;
esac
@@ -2500,7 +2511,7 @@ _git_shortlog ()
__gitcomp "
$__git_log_common_options
$__git_log_shortlog_options
- --numbered --summary
+ --numbered --summary --email
"
return
;;
@@ -2778,8 +2789,8 @@ _git_tag ()
--*)
__gitcomp "
--list --delete --verify --annotate --message --file
- --sign --cleanup --local-user --force --column --sort
- --contains --points-at
+ --sign --cleanup --local-user --force --column --sort=
+ --contains --points-at --merged --no-merged --create-reflog
"
;;
esac
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] doc: clarify distinction between sign-off and pgp-signing
From: Stefan Beller @ 2017-01-27 21:01 UTC (permalink / raw)
To: Cornelius Weig; +Cc: Junio C Hamano, Philip Oakley, git@vger.kernel.org
In-Reply-To: <01fc4c33-2d4e-e19f-d447-6a187e15d2ed@tngtech.com>
On Fri, Jan 27, 2017 at 12:48 PM, Cornelius Weig
<cornelius.weig@tngtech.com> wrote:
>>
>> So maybe s/signed-off-by/helped-by/?
>>
>
> This is getting real complex :-/
uh; sorry for that. I do not mind the patch as posted,
just in case you reroll for another reason, this is worth thinking about.
In fact, as said before I like that patch.
>
> As I said in the notes for the patch:
>
>>> As I don't know what is appropriate, I took the liberty to add everybody's
>>> sign-off who was involved in the discussion in alphabetic order.
>
> With your explanation, I guess the most accurate sign-off chain would be:
>
> Signed-off-by: Stefan Beller <sbeller@google.com> (as you sent a patch)
...and here we could continue arguing. ;)
Is the patch I sent note-worthy enough to be deriving work from?
My gut reaction would be "no".
> Helped-by: Philip Oakley <philipoakley@iee.org> (no patch, but helpful)
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com> (this patch)
> Signed-off-by: Junio C Hamano <gitster@pobox.com> (once he decides it's good)
^ permalink raw reply
* Re: [PATCH] doc: clarify distinction between sign-off and pgp-signing
From: Cornelius Weig @ 2017-01-27 20:48 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Philip Oakley, git@vger.kernel.org
In-Reply-To: <CAGZ79kb29usw4WyMdy3c5-rGH1nEcQ1gUabzdAtGgOW9zfTCDA@mail.gmail.com>
>
> So maybe s/signed-off-by/helped-by/?
>
This is getting real complex :-/
As I said in the notes for the patch:
>> As I don't know what is appropriate, I took the liberty to add everybody's
>> sign-off who was involved in the discussion in alphabetic order.
With your explanation, I guess the most accurate sign-off chain would be:
Signed-off-by: Stefan Beller <sbeller@google.com> (as you sent a patch)
Helped-by: Philip Oakley <philipoakley@iee.org> (no patch, but helpful)
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com> (this patch)
Signed-off-by: Junio C Hamano <gitster@pobox.com> (once he decides it's good)
^ permalink raw reply
* Re: [PATCH] test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/
From: Junio C Hamano @ 2017-01-27 20:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Eric Wong, git
In-Reply-To: <xmqq8tpwz6jp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> On Thu, 21 Jul 2016, Eric Wong wrote:
>>
>>> Thanks, t5003 now works out-of-the-box.
>>> Tested with Info-ZIP unzip installed and uninstalled.
>>>
>>> Tested-by: Eric Wong <e@80x24.org>
>>
>> Did you forget about this?
>
> This fell off the radar.
>
> You could have resent with Eric's Tested-by: added, to make it
> easier to apply. I'll see if I can find the original but it won't
> happen until later this afternoon.
The errand I needed to run earlier in the day turned out to be not
so time consuming---I found the exchange and the patch is now
queued, and will be part of today's pushout.
Thanks, both.
^ permalink raw reply
* octopus merge --no-ff claims to fast-forward even though it doesn't
From: Samuel Lijin @ 2017-01-27 20:46 UTC (permalink / raw)
To: git@vger.kernel.org
I was doing an octopus merge earlier and noticed that it claims to
fast-forward when you specify --no-ff, even though it does actually
abide by --no-ff.
I can consistently reproduce as follows:
$ git clone https://github.com/sxlijin/merge-octopus-experiment
$ cd merge-octopus-experiment
$ git merge --no-ff origin/A origin/B --no-edit
Fast-forwarding to: origin/A
Trying simple merge with origin/B
Merge made by the 'octopus' strategy.
anotherA | 0
anotherB | 0
otherA | 0
otherB | 0
4 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 anotherA
create mode 100644 anotherB
create mode 100644 otherA
create mode 100644 otherB
$ git log --graph --pretty=oneline --decorate
I've reproduced the issue with 2.11.0 on both a Windows box (MSYS) and
Linux (Arch).
The issue seems to live in git-merge-octopus.sh, specifically in that
--no-ff does not affect the initial value of NON_FF_MERGE. I'm happy
to submit a patch if someone can point me in the right direction.
Sam
^ permalink raw reply
* Re: [PATCH 11/12] receive-pack: treat namespace .have lines like alternates
From: Junio C Hamano @ 2017-01-27 20:42 UTC (permalink / raw)
To: Jeff King; +Cc: Lukas Fleischer, git
In-Reply-To: <20170127175807.4tjxpenu2gk77dhv@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 27, 2017 at 06:45:26PM +0100, Lukas Fleischer wrote:
>
>> I think this is already possible using receive.hideRefs (which causes
>> the ref_is_hidden() branch above to return if applicable).
>> ...
>
> Thanks for the pointers. I think a "turn off namespace .have lines"
> option would be easier for some cases, but what you've implemented is
> much more flexible. So if people using namespaces are happy with it, I
> don't see any need to add another way to do the same thing.
Yeah, I agree. Thanks, both.
^ 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