* [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
* 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
* 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: 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
* 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: 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: 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: [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: 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
* [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
* [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 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 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 03/27] attr.c: update a stale comment on "struct match_attr"
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>
When 82dce998 (attr: more matching optimizations from .gitignore,
2012-10-15) changed a pointer to a string "*pattern" into an
embedded "struct pattern" in struct match_attr, it forgot to update
the comment that describes the structure.
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 | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 04d24334e..007f1a299 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
* If is_macro is true, then u.attr is a pointer to the git_attr being
* defined.
*
- * If is_macro is false, then u.pattern points at the filename pattern
- * to which the rule applies. (The memory pointed to is part of the
- * memory block allocated for the match_attr instance.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
*
* In either case, num_attr is the number of attributes affected by
* this rule, and state is an array listing them. The attributes are
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 07/27] attr.c: simplify macroexpand_one()
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>
The double-loop wants to do an early return immediately when one
matching macro is found. Eliminate the extra variable 'a' used for
that purpose and rewrite the "assign the found item to 'a' to make
it non-NULL and force the loop(s) to terminate" with a direct return
from there.
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 | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 17297fffe..e42f931b3 100644
--- a/attr.c
+++ b/attr.c
@@ -705,24 +705,21 @@ static int fill(const char *path, int pathlen, int basename_offset,
static int macroexpand_one(int nr, int rem)
{
struct attr_stack *stk;
- struct match_attr *a = NULL;
int i;
if (check_all_attr[nr].value != ATTR__TRUE ||
!check_all_attr[nr].attr->maybe_macro)
return rem;
- for (stk = attr_stack; !a && stk; stk = stk->prev)
- for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+ for (stk = attr_stack; stk; stk = stk->prev) {
+ for (i = stk->num_matches - 1; 0 <= i; i--) {
struct match_attr *ma = stk->attrs[i];
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
- a = ma;
+ return fill_one("expand", ma, rem);
}
-
- if (a)
- rem = fill_one("expand", a, rem);
+ }
return rem;
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 06/27] attr.c: mark where #if DEBUG ends more clearly
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 9bdf87a6f..17297fffe 100644
--- a/attr.c
+++ b/attr.c
@@ -469,7 +469,7 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr
#define debug_push(a) do { ; } while (0)
#define debug_pop(a) do { ; } while (0)
#define debug_set(a,b,c,d) do { ; } while (0)
-#endif
+#endif /* DEBUG_ATTR */
static void drop_attr_stack(void)
{
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 04/27] attr.c: explain the lack of attr-name syntax check in parse_attr()
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 | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/attr.c b/attr.c
index 007f1a299..6b55a57ef 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
return NULL;
}
} else {
+ /*
+ * As this function is always called twice, once with
+ * e == NULL in the first pass and then e != NULL in
+ * the second pass, no need for invalid_attr_name()
+ * check here.
+ */
if (*cp == '-' || *cp == '!') {
e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
cp++;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 08/27] attr.c: tighten constness around "git_attr" structure
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>
It holds an interned string, and git_attr_name() is a way to peek
into it. Make sure the involved pointer types are pointer-to-const.
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 +-
attr.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index e42f931b3..f7cf7ae30 100644
--- a/attr.c
+++ b/attr.c
@@ -43,7 +43,7 @@ static int cannot_trust_maybe_real;
static struct git_attr_check *check_all_attr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
-char *git_attr_name(struct git_attr *attr)
+const char *git_attr_name(const struct git_attr *attr)
{
return attr->name;
}
diff --git a/attr.h b/attr.h
index 8b08d33af..00d7a662c 100644
--- a/attr.h
+++ b/attr.h
@@ -25,7 +25,7 @@ extern const char git_attr__false[];
* Unset one is returned as NULL.
*/
struct git_attr_check {
- struct git_attr *attr;
+ const struct git_attr *attr;
const char *value;
};
@@ -34,7 +34,7 @@ struct git_attr_check {
* return value is a pointer to a null-delimited string that is part
* of the internal data structure; it should not be modified or freed.
*/
-char *git_attr_name(struct git_attr *);
+extern const char *git_attr_name(const struct git_attr *);
int git_check_attr(const char *path, int, struct git_attr_check *);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 09/27] attr.c: plug small leak in parse_attr_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>
If any error is noticed after the match_attr structure is allocated,
we shouldn't just return NULL from this function.
Add a fail_return label that frees the allocated structure and
returns NULL, and consistently jump there when we want to return
NULL after cleaning up.
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 | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index f7cf7ae30..d180c7833 100644
--- a/attr.c
+++ b/attr.c
@@ -223,7 +223,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (!macro_ok) {
fprintf(stderr, "%s not allowed: %s:%d\n",
name, src, lineno);
- return NULL;
+ goto fail_return;
}
is_macro = 1;
name += strlen(ATTRIBUTE_MACRO_PREFIX);
@@ -233,7 +233,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
fprintf(stderr,
"%.*s is not a valid attribute name: %s:%d\n",
namelen, name, src, lineno);
- return NULL;
+ goto fail_return;
}
}
else
@@ -246,7 +246,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
for (cp = states, num_attr = 0; *cp; num_attr++) {
cp = parse_attr(src, lineno, cp, NULL);
if (!cp)
- return NULL;
+ goto fail_return;
}
res = xcalloc(1,
@@ -267,7 +267,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
warning(_("Negative patterns are ignored in git attributes\n"
"Use '\\!' for literal leading exclamation."));
- return NULL;
+ goto fail_return;
}
}
res->is_macro = is_macro;
@@ -283,6 +283,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
}
return res;
+
+fail_return:
+ free(res);
+ return NULL;
}
/*
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 10/27] attr: support quoting pathname patterns in C style
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, sbeller, gitster,
Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
not part of the pattern and document comment syntax.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.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>
---
Documentation/gitattributes.txt | 8 +++++---
attr.c | 15 +++++++++++++--
t/t0003-attributes.sh | 26 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e0b66c122..3173dee7e 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -21,9 +21,11 @@ Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
That is, a pattern followed by an attributes list,
-separated by whitespaces. When the pattern matches the
-path in question, the attributes listed on the line are given to
-the path.
+separated by whitespaces. Leading and trailing whitespaces are
+ignored. Lines that begin with '#' are ignored. Patterns
+that begin with a double quote are quoted in C style.
+When the pattern matches the path in question, the attributes
+listed on the line are given to the path.
Each attribute can be in one of these states for a given path:
diff --git a/attr.c b/attr.c
index d180c7833..e1c630f79 100644
--- a/attr.c
+++ b/attr.c
@@ -13,6 +13,7 @@
#include "attr.h"
#include "dir.h"
#include "utf8.h"
+#include "quote.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -212,12 +213,21 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
const char *cp, *name, *states;
struct match_attr *res = NULL;
int is_macro;
+ struct strbuf pattern = STRBUF_INIT;
cp = line + strspn(line, blank);
if (!*cp || *cp == '#')
return NULL;
name = cp;
- namelen = strcspn(name, blank);
+
+ if (*cp == '"' && !unquote_c_style(&pattern, name, &states)) {
+ name = pattern.buf;
+ namelen = pattern.len;
+ } else {
+ namelen = strcspn(name, blank);
+ states = name + namelen;
+ }
+
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
if (!macro_ok) {
@@ -239,7 +249,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
else
is_macro = 0;
- states = name + namelen;
states += strspn(states, blank);
/* First pass to count the attr_states */
@@ -282,9 +291,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
cannot_trust_maybe_real = 1;
}
+ strbuf_release(&pattern);
return res;
fail_return:
+ strbuf_release(&pattern);
free(res);
return NULL;
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb4255..f19ae4f8c 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -13,10 +13,31 @@ attr_check () {
test_line_count = 0 err
}
+attr_check_quote () {
+
+ path="$1"
+ quoted_path="$2"
+ expect="$3"
+
+ git check-attr test -- "$path" >actual &&
+ echo "\"$quoted_path\": test: $expect" >expect &&
+ test_cmp expect actual
+
+}
+
+test_expect_success 'open-quoted pathname' '
+ echo "\"a test=a" >.gitattributes &&
+ test_must_fail attr_check a a
+'
+
+
test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
echo "[attr]notest !test"
+ echo "\" d \" test=d"
+ echo " e test=e"
+ echo " e\" test=e"
echo "f test=f"
echo "a/i test=a/i"
echo "onoff test -test"
@@ -69,6 +90,11 @@ test_expect_success 'command line checks' '
'
test_expect_success 'attribute test' '
+
+ attr_check " d " d &&
+ attr_check e e &&
+ attr_check_quote e\" e\\\" e &&
+
attr_check f f &&
attr_check a/f f &&
attr_check a/c/f f &&
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 11/27] attr.c: add push_stack() helper
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>
There are too many repetitious "I have this new attr_stack element;
push it at the top of the stack" sequence. The new helper function
push_stack() gives us a way to express what is going on at these
places, and as a side effect, halves the number of times we mention
the attr_stack global variable.
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 | 71 +++++++++++++++++++++++++++++++-----------------------------------
1 file changed, 33 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index e1c630f79..8026d68bd 100644
--- a/attr.c
+++ b/attr.c
@@ -510,6 +510,18 @@ static int git_attr_system(void)
static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
+static void push_stack(struct attr_stack **attr_stack_p,
+ struct attr_stack *elem, char *origin, size_t originlen)
+{
+ if (elem) {
+ elem->origin = origin;
+ if (origin)
+ elem->originlen = originlen;
+ elem->prev = *attr_stack_p;
+ *attr_stack_p = elem;
+ }
+}
+
static void bootstrap_attr_stack(void)
{
struct attr_stack *elem;
@@ -517,37 +529,23 @@ static void bootstrap_attr_stack(void)
if (attr_stack)
return;
- elem = read_attr_from_array(builtin_attr);
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
-
- if (git_attr_system()) {
- elem = read_attr_from_file(git_etc_gitattributes(), 1);
- if (elem) {
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
- }
- }
+ push_stack(&attr_stack, read_attr_from_array(builtin_attr), NULL, 0);
+
+ if (git_attr_system())
+ push_stack(&attr_stack,
+ read_attr_from_file(git_etc_gitattributes(), 1),
+ NULL, 0);
if (!git_attributes_file)
git_attributes_file = xdg_config_home("attributes");
- if (git_attributes_file) {
- elem = read_attr_from_file(git_attributes_file, 1);
- if (elem) {
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
- }
- }
+ if (git_attributes_file)
+ push_stack(&attr_stack,
+ read_attr_from_file(git_attributes_file, 1),
+ NULL, 0);
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
elem = read_attr(GITATTRIBUTES_FILE, 1);
- elem->origin = xstrdup("");
- elem->originlen = 0;
- elem->prev = attr_stack;
- attr_stack = elem;
+ push_stack(&attr_stack, elem, xstrdup(""), 0);
debug_push(elem);
}
@@ -558,15 +556,12 @@ static void bootstrap_attr_stack(void)
if (!elem)
elem = xcalloc(1, sizeof(*elem));
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
+ push_stack(&attr_stack, elem, NULL, 0);
}
static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int len;
const char *cp;
/*
@@ -626,20 +621,21 @@ static void prepare_attr_stack(const char *path, int dirlen)
assert(attr_stack->origin);
while (1) {
- len = strlen(attr_stack->origin);
+ size_t len = strlen(attr_stack->origin);
+ char *origin;
+
if (dirlen <= len)
break;
cp = memchr(path + len + 1, '/', dirlen - len - 1);
if (!cp)
cp = path + dirlen;
- strbuf_add(&pathbuf, path, cp - path);
- strbuf_addch(&pathbuf, '/');
- strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
+ strbuf_addf(&pathbuf,
+ "%.*s/%s", (int)(cp - path), path,
+ GITATTRIBUTES_FILE);
elem = read_attr(pathbuf.buf, 0);
strbuf_setlen(&pathbuf, cp - path);
- elem->origin = strbuf_detach(&pathbuf, &elem->originlen);
- elem->prev = attr_stack;
- attr_stack = elem;
+ origin = strbuf_detach(&pathbuf, &len);
+ push_stack(&attr_stack, elem, origin, len);
debug_push(elem);
}
@@ -649,8 +645,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
/*
* Finally push the "info" one at the top of the stack.
*/
- info->prev = attr_stack;
- attr_stack = info;
+ push_stack(&attr_stack, info, NULL, 0);
}
static int path_matches(const char *pathname, int pathlen,
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 12/27] Documentation: fix a typo
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Stefan Beller, gitster, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/gitattributes.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 3173dee7e..a53d093ca 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -88,7 +88,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead.
Attributes for all users on a system should be placed in the
`$(prefix)/etc/gitattributes` file.
-Sometimes you would need to override an setting of an attribute
+Sometimes you would need to override a setting of an attribute
for a path to `Unspecified` state. This can be done by listing
the name of the attribute prefixed with an exclamation point `!`.
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 15/27] attr: (re)introduce git_check_attr() and struct attr_check
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>
A common pattern to check N attributes for many paths is to
(1) prepare an array A of N attr_check_item items;
(2) call git_attr() to intern the N attribute names and fill A;
(3) repeatedly call git_check_attrs() for path with N and A;
A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.
An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it. While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.
What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought. The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.
Introduce "struct attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
attr_check_item items, and a function git_check_attr() that
takes a path P and this structure as its parameters. This structure
can later be extended to hold extra data necessary for optimization.
Also, to make it easier to write the first two steps in common
cases, introduce git_attr_check_initl() helper function, which takes
a NULL-terminated list of attribute names and initialize this
structure.
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 | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
attr.h | 17 +++++++++++++++
2 files changed, 91 insertions(+)
diff --git a/attr.c b/attr.c
index 2f180d609..de8bf35a3 100644
--- a/attr.c
+++ b/attr.c
@@ -132,6 +132,75 @@ struct git_attr *git_attr(const char *name)
return git_attr_internal(name, strlen(name));
}
+struct attr_check *attr_check_alloc(void)
+{
+ return xcalloc(1, sizeof(struct attr_check));
+}
+
+struct attr_check *attr_check_initl(const char *one, ...)
+{
+ struct attr_check *check;
+ int cnt;
+ va_list params;
+ const char *param;
+
+ va_start(params, one);
+ for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+ ;
+ va_end(params);
+
+ check = attr_check_alloc();
+ check->nr = cnt;
+ check->alloc = cnt;
+ check->items = xcalloc(cnt, sizeof(struct attr_check_item));
+
+ check->items[0].attr = git_attr(one);
+ va_start(params, one);
+ for (cnt = 1; cnt < check->nr; cnt++) {
+ const struct git_attr *attr;
+ param = va_arg(params, const char *);
+ if (!param)
+ die("BUG: counted %d != ended at %d",
+ check->nr, cnt);
+ attr = git_attr(param);
+ if (!attr)
+ die("BUG: %s: not a valid attribute name", param);
+ check->items[cnt].attr = attr;
+ }
+ va_end(params);
+ return check;
+}
+
+struct attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr)
+{
+ struct attr_check_item *item;
+
+ ALLOC_GROW(check->items, check->nr + 1, check->alloc);
+ item = &check->items[check->nr++];
+ item->attr = attr;
+ return item;
+}
+
+void attr_check_reset(struct attr_check *check)
+{
+ check->nr = 0;
+}
+
+void attr_check_clear(struct attr_check *check)
+{
+ free(check->items);
+ check->items = NULL;
+ check->alloc = 0;
+ check->nr = 0;
+}
+
+void attr_check_free(struct attr_check *check)
+{
+ attr_check_clear(check);
+ free(check);
+}
+
/* What does a matched pattern decide? */
struct attr_state {
struct git_attr *attr;
@@ -865,6 +934,11 @@ int git_all_attrs(const char *path, int *num, struct attr_check_item **check)
return 0;
}
+int git_check_attr(const char *path, struct attr_check *check)
+{
+ return git_check_attrs(path, check->nr, check->items);
+}
+
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
{
enum git_attr_direction old = direction;
diff --git a/attr.h b/attr.h
index efc7bb3b3..e611b139a 100644
--- a/attr.h
+++ b/attr.h
@@ -29,6 +29,22 @@ struct attr_check_item {
const char *value;
};
+struct attr_check {
+ int nr;
+ int alloc;
+ struct attr_check_item *items;
+};
+
+extern struct attr_check *attr_check_alloc(void);
+extern struct attr_check *attr_check_initl(const char *, ...);
+
+extern struct attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr);
+
+extern void attr_check_reset(struct attr_check *check);
+extern void attr_check_clear(struct attr_check *check);
+extern void attr_check_free(struct attr_check *check);
+
/*
* Return the name of the attribute represented by the argument. The
* return value is a pointer to a null-delimited string that is part
@@ -37,6 +53,7 @@ struct attr_check_item {
extern const char *git_attr_name(const struct git_attr *);
int git_check_attrs(const char *path, int, struct attr_check_item *);
+extern int git_check_attr(const char *path, struct attr_check *check);
/*
* Retrieve all attributes that apply to the specified path. *num
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 13/27] attr.c: outline the future plans by heavily commenting
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 | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 8026d68bd..50e5ee393 100644
--- a/attr.c
+++ b/attr.c
@@ -30,6 +30,11 @@ static const char git_attr__unknown[] = "(builtin)unknown";
#define DEBUG_ATTR 0
#endif
+/*
+ * NEEDSWORK: the global dictionary of the interned attributes
+ * must stay a singleton even after we become thread-ready.
+ * Access to these must be surrounded with mutex when it happens.
+ */
struct git_attr {
struct git_attr *next;
unsigned h;
@@ -39,10 +44,19 @@ struct git_attr {
char name[FLEX_ARRAY];
};
static int attr_nr;
+static struct git_attr *(git_attr_hash[HASHSIZE]);
+
+/*
+ * NEEDSWORK: maybe-real, maybe-macro are not property of
+ * an attribute, as it depends on what .gitattributes are
+ * read. Once we introduce per git_attr_check attr_stack
+ * and check_all_attr, the optimization based on them will
+ * become unnecessary and can go away. So is this variable.
+ */
static int cannot_trust_maybe_real;
+/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check *check_all_attr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
const char *git_attr_name(const struct git_attr *attr)
{
@@ -102,6 +116,11 @@ static struct git_attr *git_attr_internal(const char *name, int len)
a->maybe_real = 0;
git_attr_hash[pos] = a;
+ /*
+ * NEEDSWORK: per git_attr_check check_all_attr
+ * will be initialized a lot more lazily, not
+ * like this, and not here.
+ */
REALLOC_ARRAY(check_all_attr, attr_nr);
check_all_attr[a->attr_nr].attr = a;
check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
@@ -318,6 +337,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
+/* NEEDSWORK: This will become per git_attr_check */
static struct attr_stack {
struct attr_stack *prev;
char *origin;
@@ -382,6 +402,24 @@ static struct attr_stack *read_attr_from_array(const char **list)
return res;
}
+/*
+ * NEEDSWORK: these two are tricky. The callers assume there is a
+ * single, system-wide global state "where we read attributes from?"
+ * and when the state is flipped by calling git_attr_set_direction(),
+ * attr_stack is discarded so that subsequent attr_check will lazily
+ * read from the right place. And they do not know or care who called
+ * by them uses the attribute subsystem, hence have no knowledge of
+ * existing git_attr_check instances or future ones that will be
+ * created).
+ *
+ * Probably we need a thread_local that holds these two variables,
+ * and a list of git_attr_check instances (which need to be maintained
+ * by hooking into git_attr_check_alloc(), git_attr_check_initl(), and
+ * git_attr_check_clear(). Then git_attr_set_direction() updates the
+ * fields in that thread_local for these two variables, iterate over
+ * all the active git_attr_check instances and discard the attr_stack
+ * they hold. Yuck, but it sounds doable.
+ */
static enum git_attr_direction direction;
static struct index_state *use_index;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 16/27] attr: convert git_all_attrs() to use "struct attr_check"
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>
This updates the other two ways the attribute check is done via an
array of "struct attr_check_item" elements. These two niches
appear only in "git check-attr".
* The caller does not know offhand what attributes it wants to ask
about and cannot use attr_check_initl() to prepare the
attr_check structure.
* The caller may not know what attributes it wants to ask at all,
and instead wants to learn everything that the given path has.
Such a caller can call attr_check_alloc() to allocate an empty
attr_check, and then call attr_check_append() to add attribute names
one by one.
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 | 168 ++++++++++++++++++++++++---------------------------
attr.h | 9 +--
builtin/check-attr.c | 60 +++++++++---------
3 files changed, 112 insertions(+), 125 deletions(-)
diff --git a/attr.c b/attr.c
index de8bf35a3..40818246f 100644
--- a/attr.c
+++ b/attr.c
@@ -132,75 +132,6 @@ struct git_attr *git_attr(const char *name)
return git_attr_internal(name, strlen(name));
}
-struct attr_check *attr_check_alloc(void)
-{
- return xcalloc(1, sizeof(struct attr_check));
-}
-
-struct attr_check *attr_check_initl(const char *one, ...)
-{
- struct attr_check *check;
- int cnt;
- va_list params;
- const char *param;
-
- va_start(params, one);
- for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
- ;
- va_end(params);
-
- check = attr_check_alloc();
- check->nr = cnt;
- check->alloc = cnt;
- check->items = xcalloc(cnt, sizeof(struct attr_check_item));
-
- check->items[0].attr = git_attr(one);
- va_start(params, one);
- for (cnt = 1; cnt < check->nr; cnt++) {
- const struct git_attr *attr;
- param = va_arg(params, const char *);
- if (!param)
- die("BUG: counted %d != ended at %d",
- check->nr, cnt);
- attr = git_attr(param);
- if (!attr)
- die("BUG: %s: not a valid attribute name", param);
- check->items[cnt].attr = attr;
- }
- va_end(params);
- return check;
-}
-
-struct attr_check_item *attr_check_append(struct attr_check *check,
- const struct git_attr *attr)
-{
- struct attr_check_item *item;
-
- ALLOC_GROW(check->items, check->nr + 1, check->alloc);
- item = &check->items[check->nr++];
- item->attr = attr;
- return item;
-}
-
-void attr_check_reset(struct attr_check *check)
-{
- check->nr = 0;
-}
-
-void attr_check_clear(struct attr_check *check)
-{
- free(check->items);
- check->items = NULL;
- check->alloc = 0;
- check->nr = 0;
-}
-
-void attr_check_free(struct attr_check *check)
-{
- attr_check_clear(check);
- free(check);
-}
-
/* What does a matched pattern decide? */
struct attr_state {
struct git_attr *attr;
@@ -439,6 +370,75 @@ static void free_attr_elem(struct attr_stack *e)
free(e);
}
+struct attr_check *attr_check_alloc(void)
+{
+ return xcalloc(1, sizeof(struct attr_check));
+}
+
+struct attr_check *attr_check_initl(const char *one, ...)
+{
+ struct attr_check *check;
+ int cnt;
+ va_list params;
+ const char *param;
+
+ va_start(params, one);
+ for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+ ;
+ va_end(params);
+
+ check = attr_check_alloc();
+ check->nr = cnt;
+ check->alloc = cnt;
+ check->items = xcalloc(cnt, sizeof(struct attr_check_item));
+
+ check->items[0].attr = git_attr(one);
+ va_start(params, one);
+ for (cnt = 1; cnt < check->nr; cnt++) {
+ const struct git_attr *attr;
+ param = va_arg(params, const char *);
+ if (!param)
+ die("BUG: counted %d != ended at %d",
+ check->nr, cnt);
+ attr = git_attr(param);
+ if (!attr)
+ die("BUG: %s: not a valid attribute name", param);
+ check->items[cnt].attr = attr;
+ }
+ va_end(params);
+ return check;
+}
+
+struct attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr)
+{
+ struct attr_check_item *item;
+
+ ALLOC_GROW(check->items, check->nr + 1, check->alloc);
+ item = &check->items[check->nr++];
+ item->attr = attr;
+ return item;
+}
+
+void attr_check_reset(struct attr_check *check)
+{
+ check->nr = 0;
+}
+
+void attr_check_clear(struct attr_check *check)
+{
+ free(check->items);
+ check->items = NULL;
+ check->alloc = 0;
+ check->nr = 0;
+}
+
+void attr_check_free(struct attr_check *check)
+{
+ attr_check_clear(check);
+ free(check);
+}
+
static const char *builtin_attr[] = {
"[attr]binary -diff -merge -text",
NULL,
@@ -906,32 +906,22 @@ int git_check_attrs(const char *path, int num, struct attr_check_item *check)
return 0;
}
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check)
+void git_all_attrs(const char *path, struct attr_check *check)
{
- int i, count, j;
+ int i;
- collect_some_attrs(path, 0, NULL);
+ attr_check_reset(check);
+ collect_some_attrs(path, check->nr, check->items);
- /* Count the number of attributes that are set. */
- count = 0;
- for (i = 0; i < attr_nr; i++) {
- const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
- ++count;
- }
- *num = count;
- ALLOC_ARRAY(*check, count);
- j = 0;
for (i = 0; i < attr_nr; i++) {
+ const char *name = check_all_attr[i].attr->name;
const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
- (*check)[j].attr = check_all_attr[i].attr;
- (*check)[j].value = value;
- ++j;
- }
+ struct attr_check_item *item;
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ item = attr_check_append(check, git_attr(name));
+ item->value = value;
}
-
- return 0;
}
int git_check_attr(const char *path, struct attr_check *check)
diff --git a/attr.h b/attr.h
index e611b139a..9f2729842 100644
--- a/attr.h
+++ b/attr.h
@@ -56,13 +56,10 @@ int git_check_attrs(const char *path, int, struct attr_check_item *);
extern int git_check_attr(const char *path, struct attr_check *check);
/*
- * Retrieve all attributes that apply to the specified path. *num
- * will be set to the number of attributes on the path; **check will
- * be set to point at a newly-allocated array of git_attr_check
- * objects describing the attributes and their values. *check must be
- * free()ed by the caller.
+ * Retrieve all attributes that apply to the specified path.
+ * check holds the attributes and their values.
*/
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check);
+extern void git_all_attrs(const char *path, struct attr_check *check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 889264a5b..40cdff13e 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,12 +24,13 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(int cnt, struct attr_check_item *check,
- const char *file)
+static void output_attr(struct attr_check *check, const char *file)
{
int j;
+ int cnt = check->nr;
+
for (j = 0; j < cnt; j++) {
- const char *value = check[j].value;
+ const char *value = check->items[j].value;
if (ATTR_TRUE(value))
value = "set";
@@ -42,36 +43,38 @@ static void output_attr(int cnt, struct attr_check_item *check,
printf("%s%c" /* path */
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
- file, 0, git_attr_name(check[j].attr), 0, value, 0);
+ file, 0,
+ git_attr_name(check->items[j].attr), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
- printf(": %s: %s\n", git_attr_name(check[j].attr), value);
+ printf(": %s: %s\n",
+ git_attr_name(check->items[j].attr), value);
}
-
}
}
static void check_attr(const char *prefix,
- int cnt, struct attr_check_item *check,
+ struct attr_check *check,
+ int collect_all,
const char *file)
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
- if (check != NULL) {
- if (git_check_attrs(full_path, cnt, check))
- die("git_check_attrs died");
- output_attr(cnt, check, file);
+
+ if (collect_all) {
+ git_all_attrs(full_path, check);
} else {
- if (git_all_attrs(full_path, &cnt, &check))
- die("git_all_attrs died");
- output_attr(cnt, check, file);
- free(check);
+ if (git_check_attr(full_path, check))
+ die("git_check_attr died");
}
+ output_attr(check, file);
+
free(full_path);
}
static void check_attr_stdin_paths(const char *prefix,
- int cnt, struct attr_check_item *check)
+ struct attr_check *check,
+ int collect_all)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -85,7 +88,7 @@ static void check_attr_stdin_paths(const char *prefix,
die("line is badly quoted");
strbuf_swap(&buf, &unquoted);
}
- check_attr(prefix, cnt, check, buf.buf);
+ check_attr(prefix, check, collect_all, buf.buf);
maybe_flush_or_die(stdout, "attribute to stdout");
}
strbuf_release(&buf);
@@ -100,7 +103,7 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
- struct attr_check_item *check;
+ struct attr_check *check;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ -160,28 +163,25 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
error_with_usage("No file specified");
}
- if (all_attrs) {
- check = NULL;
- } else {
- check = xcalloc(cnt, sizeof(*check));
+ check = attr_check_alloc();
+ if (!all_attrs) {
for (i = 0; i < cnt; i++) {
- const char *name;
- struct git_attr *a;
- name = argv[i];
- a = git_attr(name);
+ struct git_attr *a = git_attr(argv[i]);
if (!a)
return error("%s: not a valid attribute name",
- name);
- check[i].attr = a;
+ argv[i]);
+ attr_check_append(check, a);
}
}
if (stdin_paths)
- check_attr_stdin_paths(prefix, cnt, check);
+ check_attr_stdin_paths(prefix, check, all_attrs);
else {
for (i = filei; i < argc; i++)
- check_attr(prefix, cnt, check, argv[i]);
+ check_attr(prefix, check, all_attrs, argv[i]);
maybe_flush_or_die(stdout, "attribute to stdout");
}
+
+ attr_check_free(check);
return 0;
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
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