Skip to contentSkip to conversation

rev-list-options: fix off-by-one in '--filter=blob:limit=<n>' explainer

PATCH In review Nikolay Edigaryev via GitGitGadget 2 participants 2 messages Jan 14, 2024 – Jan 16, 2024 Documentation

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rev-list-options: fix off-by-one in '--filter=blob:limit=<n>' explainer

From: Nikolay Edigaryev <[email protected]>

'--filter=blob:limit=<n>' was introduced in 25ec7bcac0 (list-objects:
filter objects in traverse_commit_list, 2017-11-21) and later expanded to bitmaps in 84243da129 (pack-bitmap: implement BLOB_LIMIT filtering, 2020-02-14)

The logic that was introduced in these commits (and that still persists to this day) omits blobs larger than _or equal_ to n bytes or units.

However, the documentation (Documentation/rev-list-options.txt) states:

Quoted 1 line

The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n

bytes or units. n may be zero.

Moreover, the t6113-rev-list-bitmap-filters.sh tests for exactly this logic, so it seems it is the documentation that needs fixing, not the code.

This changes the explanation to be similar to
Documentation/git-clone.txt, which is correct.

---
rev-list-options: fix off-by-one in '--filter=blob:limit=' explainer

'--filter=blob:limit=' was introduced in 25ec7bcac0 (list-objects:
filter objects in traverse_commit_list, 2017-11-21) and later expanded to bitmaps in 84243da129 (pack-bitmap: implement BLOB_LIMIT filtering, 2020-02-14)

The logic that was introduced in these commits (and that still persists to this day) omits blobs larger than or equal to n bytes or units.

However, the documentation (Documentation/rev-list-options.txt) states:

> The form '--filter=blob:limit=[kmg]' omits blobs larger than n bytes > or units. n may be zero.

Moreover, the t6113-rev-list-bitmap-filters.sh tests for exactly this logic, so it seems it is the documentation that needs fixing, not the code.

This changes the explanation to be similar to Documentation/git-clone.txt, which is correct.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1645%2Fedigaryev%2Ffix-blob-limit-off-by-one-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1645/edigaryev/fix-blob-limit-off-by-one-v1
Pull-Request: https://github.com/git/git/pull/1645

 Documentation/rev-list-options.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Documentation/rev-list-options.txt +4 -4
index 2bf239ff030..a583b52c612 100644
@@ -947,10 +947,10 @@ ifdef::git-rev-list[]
 +
 The form '--filter=blob:none' omits all blobs.
 +
-The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n bytes
-or units.  n may be zero.  The suffixes k, m, and g can be used to name
-units in KiB, MiB, or GiB.  For example, 'blob:limit=1k' is the same
-as 'blob:limit=1024'.
+The form '--filter=blob:limit=<n>[kmg]' omits blobs of size at least n
+bytes or units.  n may be zero.  The suffixes k, m, and g can be used
+to name units in KiB, MiB, or GiB.  For example, 'blob:limit=1k'
+is the same as 'blob:limit=1024'.
 +
 The form '--filter=object:type=(tag|commit|tree|blob)' omits all objects
 which are not of the requested type.

base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d

Signature · 1 line
gitgitgadget
#1 lore raw

"Nikolay Edigaryev via GitGitGadget" <[email protected]>

Quoted 9 lines · writes:

From: Nikolay Edigaryev <[email protected]>

'--filter=blob:limit=<n>' was introduced in 25ec7bcac0 (list-objects:
filter objects in traverse_commit_list, 2017-11-21) and later expanded to bitmaps in 84243da129 (pack-bitmap: implement BLOB_LIMIT filtering, 2020-02-14)

The logic that was introduced in these commits (and that still persists to this day) omits blobs larger than _or equal_ to n bytes or units.

Good eyes. The former does this

		if (object_length < filter_data->max_bytes)
			goto include_it;

and the latter does this

                if (!bitmap_get(tips, pos) &&
                    get_size_by_pos(bitmap_git, pos) >= limit)
                        bitmap_unset(to_filter, pos);
Quoted 8 lines

However, the documentation (Documentation/rev-list-options.txt) states:

Quoted 1 line

The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n

bytes or units. n may be zero.

Moreover, the t6113-rev-list-bitmap-filters.sh tests for exactly this logic, so it seems it is the documentation that needs fixing, not the code.

Yup. The mechanism is used for things like "we do not want a large blob, like 100MB", and a byte on the boundary does not matter all that much in such a countext, but it does not hurt to be more correct ;-)

Documentation/rev-list-options.txt ifdef::git-rev-list[] · line 953

Quoted 10 lines

The form '--filter=blob:none' omits all blobs.
+

Documentation/rev-list-options.txt +4 -4
-The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n bytes
-or units.  n may be zero.  The suffixes k, m, and g can be used to name
-units in KiB, MiB, or GiB.  For example, 'blob:limit=1k' is the same
-as 'blob:limit=1024'.
+The form '--filter=blob:limit=<n>[kmg]' omits blobs of size at least n
+bytes or units.  n may be zero.  The suffixes k, m, and g can be used
+to name units in KiB, MiB, or GiB.  For example, 'blob:limit=1k'
+is the same as 'blob:limit=1024'.

With unnecessary paragraph wrapping, it is a bit hard to compare the preimage and the postimage, but I manually checked that this only does

	"larger than" -> "of size at least"

and nothing else, which is expected and in line with what the proposed commit message claimed to do. Good job.

Will queue. Thanks.

Quoted 5 lines

+
The form '--filter=object:type=(tag|commit|tree|blob)' omits all objects which are not of the requested type.

base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d