From: Jonathan Tan <jonathantanmy@google.com>
To: matvore@google.com
Cc: git@vger.kernel.org, sbeller@google.com, git@jeffhostetler.com,
jeffhost@microsoft.com, peff@peff.net, stefanbeller@gmail.com,
jonathantanmy@google.com, pclouds@gmail.com
Subject: Re: [PATCH v2 1/2] list-objects-filter: teach tree:# how to handle >0
Date: Mon, 7 Jan 2019 17:56:31 -0800 [thread overview]
Message-ID: <20190108015631.22727-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20181210234030.176178-2-matvore@google.com>
Thanks - as stated in your commit message, this adds quite a useful
piece of functionality.
> - case LOFS_BEGIN_TREE:
> - case LOFS_BLOB:
> - if (filter_data->omits) {
> - oidset_insert(filter_data->omits, &obj->oid);
> - /* _MARK_SEEN but not _DO_SHOW (hard omit) */
> - return LOFR_MARK_SEEN;
> - } else {
> - /*
> - * Not collecting omits so no need to to traverse tree.
> - */
> - return LOFR_SKIP_TREE | LOFR_MARK_SEEN;
> - }
> -
> case LOFS_END_TREE:
> assert(obj->type == OBJ_TREE);
> + filter_data->current_depth--;
> return LOFR_ZERO;
>
> + case LOFS_BLOB:
> + filter_trees_update_omits(obj, filter_data, include_it);
> + return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;
Any reason for moving "case LOFS_BLOB" (and "case LOFS_BEGIN_TREE"
below) after LOFS_END_TREE?
This is drastically different from the previous case, but this makes
sense - previously, all blobs accessed through traversal were not shown,
but now they are sometimes shown. Here, filter_trees_update_omits() is
only ever used to remove a blob from the omits set, since once this blob
is encountered with include_it == true, it is marked as LOFR_MARK_SEEN
and will not be traversed again.
> + case LOFS_BEGIN_TREE:
> + seen_info = oidmap_get(
> + &filter_data->seen_at_depth, &obj->oid);
> + if (!seen_info) {
> + seen_info = xcalloc(1, sizeof(struct seen_map_entry));
Use sizeof(*seen_info).
> + seen_info->base.oid = obj->oid;
We have been using oidcpy, but come to think of it, I'm not sure why...
> + seen_info->depth = filter_data->current_depth;
> + oidmap_put(&filter_data->seen_at_depth, seen_info);
> + already_seen = 0;
> + } else
> + already_seen =
> + filter_data->current_depth >= seen_info->depth;
There has been recently some clarification that if one branch of an
if/else construct requires braces, braces should be put on all of them:
1797dc5176 ("CodingGuidelines: clarify multi-line brace style",
2017-01-17). Likewise below.
> + if (already_seen)
> + filter_res = LOFR_SKIP_TREE;
> + else {
> + seen_info->depth = filter_data->current_depth;
> + filter_trees_update_omits(obj, filter_data, include_it);
> +
> + if (include_it)
> + filter_res = LOFR_DO_SHOW;
> + else if (filter_data->omits)
> + filter_res = LOFR_ZERO;
> + else
> + filter_res = LOFR_SKIP_TREE;
Looks straightforward. If we have already seen it at a shallower or
equal depth, we can skip it (since we have already done the appropriate
processing). Otherwise, we need to ensure that its "omit" is correctly
set, and:
- show it if include_it
- don't do anything special if not include_it and we need the omit set
- skip the tree if not include_it and we don't need the omit set
> +static void filter_trees_free(void *filter_data) {
> + struct filter_trees_depth_data* d = filter_data;
> + oidmap_free(&d->seen_at_depth, 1);
> + free(d);
> +}
Check for NULL-ness of filter_data too, to match the usual behavior of
free functions.
> diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
> index eb32505a6e..54e7096d40 100755
> --- a/t/t6112-rev-list-filters-objects.sh
> +++ b/t/t6112-rev-list-filters-objects.sh
[snip]
Thanks for the tests that cover quite a wide range of cases. Can you
also demonstrate the case where a blob would normally be omitted
(because it is too deep) but it is directly specified, so it is
included.
> +expect_has_with_different_name () {
> + repo=$1 &&
> + name=$2 &&
> +
> + hash=$(git -C $repo rev-parse HEAD:$name) &&
> + ! grep "^$hash $name$" actual &&
> + grep "^$hash " actual &&
> + ! grep "~$hash" actual
> +}
Should we also check that a "~" entry appears with $name?
next prev parent reply other threads:[~2019-01-08 1:56 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-10 23:40 [PATCH v2 0/2] support for filtering trees and blobs based on depth Matthew DeVore
2018-12-10 23:40 ` [PATCH v2 1/2] list-objects-filter: teach tree:# how to handle >0 Matthew DeVore
2019-01-08 1:56 ` Jonathan Tan [this message]
2019-01-08 19:22 ` Matthew DeVore
2019-01-08 23:19 ` Jonathan Tan
2019-01-08 23:36 ` Junio C Hamano
2019-01-08 23:41 ` Jonathan Tan
2019-01-08 23:39 ` Junio C Hamano
2019-01-09 2:43 ` MATTHEW DEVORE
2018-12-10 23:40 ` [PATCH v2 2/2] tree:<depth>: skip some trees even when collecting omits Matthew DeVore
2019-01-08 2:00 ` Jonathan Tan
2019-01-08 23:22 ` Jonathan Tan
2019-01-09 2:47 ` MATTHEW DEVORE
2019-01-09 0:29 ` Matthew DeVore
2018-12-11 8:45 ` [PATCH v2 0/2] support for filtering trees and blobs based on depth Junio C Hamano
2019-01-08 0:56 ` Matthew DeVore
2019-01-09 2:59 ` [PATCH v3 " Matthew DeVore
2019-01-09 2:59 ` [PATCH v3 1/2] list-objects-filter: teach tree:# how to handle >0 Matthew DeVore
2019-01-09 2:59 ` [PATCH v3 2/2] tree:<depth>: skip some trees even when collecting omits Matthew DeVore
2019-01-09 18:06 ` [PATCH v3 0/2] support for filtering trees and blobs based on depth Jonathan Tan
2019-01-15 23:35 ` Junio C Hamano
2019-01-15 23:41 ` Junio C Hamano
2019-01-17 0:14 ` Matthew DeVore
2019-01-17 18:44 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190108015631.22727-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=git@jeffhostetler.com \
--cc=git@vger.kernel.org \
--cc=jeffhost@microsoft.com \
--cc=matvore@google.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=sbeller@google.com \
--cc=stefanbeller@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).