Skip to content

Revamp the attribute system; another round

Brandon Williams git 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest27 patches 5 repliesIn review
  2. v227 patches 10 repliesSuperseded
  3. v127 patches 10 repliesSuperseded

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/27commit.c: use strchrnul() to scan for one linecommit.c+1 -20 replies
2/27attr.c: use strchrnul() to scan for one lineattr.c+2 -20 replies
3/27attr.c: update a stale comment on "struct match_attr"attr.c+2 -30 replies
4/27attr.c: explain the lack of attr-name syntax check in parse_attr()attr.c+6 -00 replies
5/27attr.c: complete a sentence in a commentattr.c+1 -10 replies
6/27attr.c: mark where #if DEBUG ends more clearlyattr.c+1 -10 replies
7/27attr.c: simplify macroexpand_one()attr.c+4 -70 replies
8/27attr.c: tighten constness around "git_attr" structureattr.c attr.h+3 -30 replies
9/27attr.c: plug small leak in parse_attr_line()attr.c+8 -40 replies
10/27attr: support quoting pathname patterns in C styleDocumentation/gitattributes.txt attr.c t/t0003-attributes.sh+44 -50 replies
11/27attr.c: add push_stack() helperattr.c+33 -380 replies
12/27Documentation: fix a typoDocumentation/gitattributes.txt+1 -10 replies
13/27attr.c: outline the future plans by heavily commentingattr.c+39 -10 replies
14/27attr: rename function and struct related to checking attributesarchive.c attr.c attr.h +6 more+42 -410 replies
15/27attr: (re)introduce git_check_attr() and struct attr_checkattr.c attr.h+91 -00 replies
15/27attr: (re)introduce git_check_attr() and struct attr_checkattr.c attr.h+91 -00 replies
16/27attr: convert git_all_attrs() to use "struct attr_check"attr.c attr.h builtin/check-attr.c+112 -1252 replies
16/27attr: convert git_all_attrs() to use "struct attr_check"attr.c attr.h builtin/check-attr.c+43 -560 replies
17/27attr: convert git_check_attrs() callers to use the new APIarchive.c builtin/pack-objects.c convert.c +3 more+45 -860 replies
18/27attr: retire git_check_attrs() APIDocumentation/technical/api-gitattributes.txt attr.c attr.h+58 -320 replies
19/27attr: pass struct attr_check to collect_some_attrsattr.c+13 -200 replies
20/27attr: change validity check for attribute names to use positive logicattr.c+20 -140 replies
21/27attr: use hashmap for attribute dictionaryattr.c attr.h common-main.c+133 -450 replies
22/27attr: eliminate global check_all_attr arrayattr.c attr.h+87 -390 replies
23/27attr: remove maybe-real, maybe-macro from git_attrattr.c+37 -380 replies
24/27attr: tighten const correctness with git_attr and match_attrattr.c attr.h builtin/check-attr.c+9 -80 replies
25/27attr: store attribute stack in attr_check structureattr.c attr.h+199 -890 replies
26/27attr: push the bare repo check into read_attr()attr.c+54 -600 replies
27/27attr: reformat git_attr_set_direction() functionattr.c attr.h+22 -300 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v3 yet.

Read the v3 thread

Cover letter [PATCH v3 00/27] Revamp the attribute system; another round

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(-)
Signature · 1 line
2.11.0.483.g087da7b7c-goog