Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

edigaryev
Copy link
Contributor

'--filter=blob:limit=' was introduced in 25ec7bc (list-objects: filter objects in traverse_commit_list, 2017-11-21) and later expanded to bitmaps in 84243da (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.

'--filter=blob:limit=<n>' was introduced in 25ec7bc (list-objects:
filter objects in traverse_commit_list, 2017-11-21) and later expanded
to bitmaps in 84243da (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=<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.

Signed-off-by: Nikolay Edigaryev <edigaryev@gmail.com>
@edigaryev
Copy link
Contributor Author

/submit

Copy link

Submitted as pull.1645.git.git.1705261850650.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1645/edigaryev/fix-blob-limit-off-by-one-v1

To fetch this version to local tag pr-git-1645/edigaryev/fix-blob-limit-off-by-one-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1645/edigaryev/fix-blob-limit-off-by-one-v1

Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Nikolay Edigaryev via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Nikolay Edigaryev <edigaryev@gmail.com>
>
> '--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);

> However, the documentation (Documentation/rev-list-options.txt) states:
>
>>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 ;-)

>  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'.

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.

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

Copy link

This branch is now known as ne/doc-filter-blob-limit-fix.

Copy link

This patch series was integrated into seen via a45d645.

Copy link

There was a status update in the "New Topics" section about the branch ne/doc-filter-blob-limit-fix on the Git mailing list:

Docfix.

Will merge to 'next'.
source: <pull.1645.git.git.1705261850650.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via bae6296.

Copy link

This patch series was integrated into seen via 3d6510d.

Copy link

This patch series was integrated into seen via 101ef5c.

Copy link

This patch series was integrated into next via 4f78975.

Copy link

There was a status update in the "Cooking" section about the branch ne/doc-filter-blob-limit-fix on the Git mailing list:

Docfix.

Will merge to 'master'.
source: <pull.1645.git.git.1705261850650.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via 1d262b7.

Copy link

This patch series was integrated into seen via 531e73d.

Copy link

This patch series was integrated into seen via 9d9a662.

Copy link

There was a status update in the "Cooking" section about the branch ne/doc-filter-blob-limit-fix on the Git mailing list:

Docfix.

Will merge to 'master'.
source: <pull.1645.git.git.1705261850650.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via cf63945.

Copy link

This patch series was integrated into seen via 9c6a09d.

Copy link

This patch series was integrated into seen via f3657b3.

Copy link

This patch series was integrated into master via f3657b3.

Copy link

This patch series was integrated into next via f3657b3.

Copy link

Closed via f3657b3.

@gitgitgadget-git gitgitgadget-git bot closed this Jan 26, 2024
@edigaryev edigaryev deleted the fix-blob-limit-off-by-one branch January 26, 2024 23:02
derrickstolee pushed a commit to derrickstolee/git that referenced this pull request Apr 30, 2024
This was pull request git#1645 from ZCube/master

Support windows container.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
derrickstolee pushed a commit to derrickstolee/git that referenced this pull request Apr 30, 2024
This was pull request git#1645 from ZCube/master

Support windows container.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
derrickstolee pushed a commit to derrickstolee/git that referenced this pull request Apr 30, 2024
This was pull request git#1645 from ZCube/master

Support windows container.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant