git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
BWBrandon Williams <bmwill@google.com>
Date
Jan 28, 2017, 02:01 UTC
Message-ID
<20170128020207.179015-1-bmwill@google.com>
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
Previous: Brandon WilliamsNext: Brandon Williams
Message 77 of 111 in “Revamp the attribute system; another round”
  1. 00/27 Revamp the attribute system; another roundBrandon Williams, Jan 12, 2017
  2. 20/27 attr: change validity check for attribute names to use positive logicBrandon Williams, Jan 12, 2017
  3. 19/27 attr: pass struct attr_check to collect_some_attrsBrandon Williams, Jan 12, 2017
  4. 18/27 attr: retire git_check_attrs() APIBrandon Williams, Jan 12, 2017
  5. 17/27 attr: convert git_check_attrs() callers to use the new APIBrandon Williams, Jan 12, 2017
  6. 15/27 attr: (re)introduce git_check_attr() and struct attr_checkBrandon Williams, Jan 12, 2017
  7. 16/27 attr: convert git_all_attrs() to use "struct attr_check"Brandon Williams, Jan 12, 2017
  8. 23/27 attr: remove maybe-real, maybe-macro from git_attrBrandon Williams, Jan 12, 2017
  9. 24/27 attr: tighten const correctness with git_attr and match_attrBrandon Williams, Jan 12, 2017
  10. 21/27 attr: use hashmap for attribute dictionaryBrandon Williams, Jan 12, 2017
  11. Stefan BellerJan 18, 2017
  12. Brandon WilliamsJan 18, 2017
  13. 25/27 attr: store attribute stacks in hashmapBrandon Williams, Jan 12, 2017
  14. Junio C HamanoJan 13, 2017
  15. Brandon WilliamsJan 18, 2017
  16. Brandon WilliamsJan 23, 2017
  17. Stefan BellerJan 18, 2017
  18. Brandon WilliamsJan 18, 2017
  19. Stefan BellerJan 18, 2017
  20. 22/27 attr: eliminate global check_all_attr arrayBrandon Williams, Jan 12, 2017
  21. 27/27 attr: reformat git_attr_set_direction() functionBrandon Williams, Jan 12, 2017
  22. 26/27 attr: push the bare repo check into read_attr()Brandon Williams, Jan 12, 2017
  23. 13/27 attr.c: outline the future plans by heavily commentingBrandon Williams, Jan 12, 2017
  24. 12/27 Documentation: fix a typoBrandon Williams, Jan 12, 2017
  25. 01/27 commit.c: use strchrnul() to scan for one lineBrandon Williams, Jan 12, 2017
  26. 02/27 attr.c: use strchrnul() to scan for one lineBrandon Williams, Jan 12, 2017
  27. 11/27 attr.c: add push_stack() helperBrandon Williams, Jan 12, 2017
  28. 08/27 attr.c: tighten constness around "git_attr" structureBrandon Williams, Jan 12, 2017
  29. 14/27 attr: rename function and struct related to checking attributesBrandon Williams, Jan 12, 2017
  30. 06/27 attr.c: mark where #if DEBUG ends more clearlyBrandon Williams, Jan 12, 2017
  31. 07/27 attr.c: simplify macroexpand_one()Brandon Williams, Jan 12, 2017
  32. 10/27 attr: support quoting pathname patterns in C styleBrandon Williams, Jan 12, 2017
  33. 09/27 attr.c: plug small leak in parse_attr_line()Brandon Williams, Jan 12, 2017
  34. 04/27 attr.c: explain the lack of attr-name syntax check in parse_attr()Brandon Williams, Jan 12, 2017
  35. 05/27 attr.c: complete a sentence in a commentBrandon Williams, Jan 12, 2017
  36. 03/27 attr.c: update a stale comment on "struct match_attr"Brandon Williams, Jan 12, 2017
  37. Junio C HamanoJan 15, 2017
  38. Jeff KingJan 16, 2017
  39. 00/27 Revamp the attribute system; another roundBrandon Williams, Jan 23, 2017
  40. 01/27 commit.c: use strchrnul() to scan for one lineBrandon Williams, Jan 23, 2017
  41. 02/27 attr.c: use strchrnul() to scan for one lineBrandon Williams, Jan 23, 2017
  42. 03/27 attr.c: update a stale comment on "struct match_attr"Brandon Williams, Jan 23, 2017
  43. 04/27 attr.c: explain the lack of attr-name syntax check in parse_attr()Brandon Williams, Jan 23, 2017
  44. 05/27 attr.c: complete a sentence in a commentBrandon Williams, Jan 23, 2017
  45. 07/27 attr.c: simplify macroexpand_one()Brandon Williams, Jan 23, 2017
  46. 10/27 attr: support quoting pathname patterns in C styleBrandon Williams, Jan 23, 2017
  47. 12/27 Documentation: fix a typoBrandon Williams, Jan 23, 2017
  48. 13/27 attr.c: outline the future plans by heavily commentingBrandon Williams, Jan 23, 2017
  49. 09/27 attr.c: plug small leak in parse_attr_line()Brandon Williams, Jan 23, 2017
  50. 11/27 attr.c: add push_stack() helperBrandon Williams, Jan 23, 2017
  51. 14/27 attr: rename function and struct related to checking attributesBrandon Williams, Jan 23, 2017
  52. 20/27 attr: change validity check for attribute names to use positive logicBrandon Williams, Jan 23, 2017
  53. 21/27 attr: use hashmap for attribute dictionaryBrandon Williams, Jan 23, 2017
  54. 22/27 attr: eliminate global check_all_attr arrayBrandon Williams, Jan 23, 2017
  55. Junio C HamanoJan 23, 2017
  56. 16/27 attr: convert git_all_attrs() to use "struct attr_check"Brandon Williams, Jan 23, 2017
  57. 23/27 attr: remove maybe-real, maybe-macro from git_attrBrandon Williams, Jan 23, 2017
  58. 24/27 attr: tighten const correctness with git_attr and match_attrBrandon Williams, Jan 23, 2017
  59. 17/27 attr: convert git_check_attrs() callers to use the new APIBrandon Williams, Jan 23, 2017
  60. 18/27 attr: retire git_check_attrs() APIBrandon Williams, Jan 23, 2017
  61. 15/27 attr: (re)introduce git_check_attr() and struct attr_checkBrandon Williams, Jan 23, 2017
  62. 06/27 attr.c: mark where #if DEBUG ends more clearlyBrandon Williams, Jan 23, 2017
  63. 08/27 attr.c: tighten constness around "git_attr" structureBrandon Williams, Jan 23, 2017
  64. 19/27 attr: pass struct attr_check to collect_some_attrsBrandon Williams, Jan 23, 2017
  65. 26/27 attr: push the bare repo check into read_attr()Brandon Williams, Jan 23, 2017
  66. 27/27 attr: reformat git_attr_set_direction() functionBrandon Williams, Jan 23, 2017
  67. 25/27 attr: store attribute stack in attr_check structureBrandon Williams, Jan 23, 2017
  68. Junio C HamanoJan 23, 2017
  69. Brandon WilliamsJan 23, 2017
  70. Brandon WilliamsJan 24, 2017
  71. Junio C HamanoJan 24, 2017
  72. Brandon WilliamsJan 25, 2017
  73. Stefan BellerJan 25, 2017
  74. Junio C HamanoJan 25, 2017
  75. Brandon WilliamsJan 25, 2017
  76. Brandon WilliamsJan 25, 2017
  77. 00/27 Revamp the attribute system; another roundBrandon Williams, Jan 28, 2017
  78. 01/27 commit.c: use strchrnul() to scan for one lineBrandon Williams, Jan 28, 2017
  79. 02/27 attr.c: use strchrnul() to scan for one lineBrandon Williams, Jan 28, 2017
  80. 05/27 attr.c: complete a sentence in a commentBrandon Williams, Jan 28, 2017
  81. 03/27 attr.c: update a stale comment on "struct match_attr"Brandon Williams, Jan 28, 2017
  82. 07/27 attr.c: simplify macroexpand_one()Brandon Williams, Jan 28, 2017
  83. 06/27 attr.c: mark where #if DEBUG ends more clearlyBrandon Williams, Jan 28, 2017
  84. 08/27 attr.c: tighten constness around "git_attr" structureBrandon Williams, Jan 28, 2017
  85. 04/27 attr.c: explain the lack of attr-name syntax check in parse_attr()Brandon Williams, Jan 28, 2017
  86. 09/27 attr.c: plug small leak in parse_attr_line()Brandon Williams, Jan 28, 2017
  87. 10/27 attr: support quoting pathname patterns in C styleBrandon Williams, Jan 28, 2017
  88. 11/27 attr.c: add push_stack() helperBrandon Williams, Jan 28, 2017
  89. 12/27 Documentation: fix a typoBrandon Williams, Jan 28, 2017
  90. 15/27 attr: (re)introduce git_check_attr() and struct attr_checkBrandon Williams, Jan 28, 2017
  91. 15/27 attr: (re)introduce git_check_attr() and struct attr_checkBrandon Williams, Jan 30, 2017
  92. 13/27 attr.c: outline the future plans by heavily commentingBrandon Williams, Jan 28, 2017
  93. 16/27 attr: convert git_all_attrs() to use "struct attr_check"Brandon Williams, Jan 28, 2017
  94. Stefan BellerJan 28, 2017
  95. Brandon WilliamsJan 29, 2017
  96. 16/27 attr: convert git_all_attrs() to use "struct attr_check"Brandon Williams, Jan 30, 2017
  97. 14/27 attr: rename function and struct related to checking attributesBrandon Williams, Jan 28, 2017
  98. 18/27 attr: retire git_check_attrs() APIBrandon Williams, Jan 28, 2017
  99. 19/27 attr: pass struct attr_check to collect_some_attrsBrandon Williams, Jan 28, 2017
  100. 17/27 attr: convert git_check_attrs() callers to use the new APIBrandon Williams, Jan 28, 2017
  101. 22/27 attr: eliminate global check_all_attr arrayBrandon Williams, Jan 28, 2017
  102. 21/27 attr: use hashmap for attribute dictionaryBrandon Williams, Jan 28, 2017
  103. 26/27 attr: push the bare repo check into read_attr()Brandon Williams, Jan 28, 2017
  104. 25/27 attr: store attribute stack in attr_check structureBrandon Williams, Jan 28, 2017
  105. 20/27 attr: change validity check for attribute names to use positive logicBrandon Williams, Jan 28, 2017
  106. 24/27 attr: tighten const correctness with git_attr and match_attrBrandon Williams, Jan 28, 2017
  107. 27/27 attr: reformat git_attr_set_direction() functionBrandon Williams, Jan 28, 2017
  108. 23/27 attr: remove maybe-real, maybe-macro from git_attrBrandon Williams, Jan 28, 2017
  109. Junio C HamanoFeb 2, 2017
  110. Brandon WilliamsFeb 9, 2017
  111. Junio C HamanoFeb 9, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.