git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Aaron Lipman <alipman88@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 2/2] ref-filter: allow merged and no-merged filters
Date: Fri, 11 Sep 2020 14:47:00 -0700
Message-ID: <xmqqlfhgm1ez.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200911185754.64173-3-alipman88@gmail.com> (Aaron Lipman's message of "Fri, 11 Sep 2020 14:57:54 -0400")

Aaron Lipman <alipman88@gmail.com> writes:

> If passed more than one merged (or more than one no-merged) filter, refs
> must be reachable from any one of the merged commits, and reachable from
> none of the no-merged commits.

This mirrors how the "contains" behaves, which is good.

>  Documentation/git-branch.txt       |  6 +--
>  Documentation/git-for-each-ref.txt |  6 +--
>  Documentation/git-tag.txt          |  4 +-

It is a bit sad that this only removes from documentation without
adding (the removal is because merged and no-merged are no longer
mutually exclusive).  There should be a new description on how it
behaves when both of them are given---the combination were impossible
so it was sufficient to say "they are incompatible", but now the user
needs to know how they interact with each other.

Perhaps --contains side can already be combined so they have
description we can borrow here?  I didn't look too carefully.

> diff --git a/ref-filter.c b/ref-filter.c
> index 110bcd741a..c04dca47d1 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2231,11 +2231,17 @@ void ref_array_clear(struct ref_array *array)
>  	}
>  }
>  
> -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
> +static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
>  {
> +	struct commit_list *check_reachable_list = reachable ?
> +		ref_cbdata->filter->reachable_from :
> +		ref_cbdata->filter->unreachable_from;
> +
> +	if (!check_reachable_list)
> +		return;
> +

We do not allow decl-after-statement.

>  	struct rev_info revs;
>  	int i, old_nr;
> -	struct ref_filter *filter = ref_cbdata->filter;
>  	struct ref_array *array = ref_cbdata->array;
>  	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
>  
> @@ -2247,12 +2253,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
>  		to_clear[i] = item->commit;
>  	}
>  
> -	filter->merge_commit->object.flags |= UNINTERESTING;
> -	add_pending_object(&revs, &filter->merge_commit->object, "");
> +	for (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {
> +		struct commit *merge_commit = rl->item;
> +		merge_commit->object.flags |= UNINTERESTING;
> +		add_pending_object(&revs, &merge_commit->object, "");
> -	revs.limited = 1;
> -	if (prepare_revision_walk(&revs))
> -		die(_("revision walk setup failed"));
> +		revs.limited = 1;
> +		if (prepare_revision_walk(&revs))
> +			die(_("revision walk setup failed"));
> +	}

This looks wrong.  I would have expected, instead of placing the
single object to the pending queue, the loop places all the filter
objects in a queue, and then makes a limited revision walk just
once.  In general, each time after the code makes a call to
prepare_revision_walk() to perform a revision walk, before doing so
again, the object flags used for the walking must be cleared.  You
can tell that the original code structure follows the pattern.
Calling it inside a loop breaks the pattern a big time.

In any case, we mark the named commits as UNINTERESTING and walk
from them and the tips of (surviving) refs to see which refs are
reachable from _any_ of the named commits.  The original code is a
degenerated case of having only one named commit.

The idea of the original is when checking for "--merged", any ref
whose tip is reachable from the named commit is merged and should be
shown.  That extends naturally to multiple commits given to
"--merged"; any ref whose tip is reachable from one of the named
commits is selected.

For "--no-merged", any ref whose tip is reachable from the named
commit is merged and should be excluded.  That also extends to
multiple commits.  Any ref whose tip is reachable from one of the
"--no-merged" commit is rejected.

> @@ -2263,14 +2272,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
>  
>  		int is_merged = !!(commit->object.flags & UNINTERESTING);
>  
> -		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
> +		if (is_merged == reachable)
>  			array->items[array->nr++] = array->items[i];
>  		else
>  			free_array_item(item);

So, in the original code, if the tip of the ref is reachable from
the given commit (i.e. painted UNINTERESTING), we keep it in the
array when looking for "--merged".  That corresponds to the case 
where reachable is true.  Similarly for the --no-merged case.

So the overall logic seems sound.  It's just the callsite of
prepare_revision_walk() which does the actual painting of the graph
looks wrong.

> @@ -2541,31 +2555,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
>  {
>  	struct ref_filter *rf = opt->value;
>  	struct object_id oid;
> -	int no_merged = starts_with(opt->long_name, "no");
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	if (rf->merge) {
> -		if (no_merged) {
> -			return error(_("option `%s' is incompatible with --merged"),
> -				     opt->long_name);
> -		} else {
> -			return error(_("option `%s' is incompatible with --no-merged"),
> -				     opt->long_name);
> -		}
> -	}
> -
> -	rf->merge = no_merged
> -		? REF_FILTER_MERGED_OMIT
> -		: REF_FILTER_MERGED_INCLUDE;
> -
>  	if (get_oid(arg, &oid))
>  		die(_("malformed object name %s"), arg);
>  
> -	rf->merge_commit = lookup_commit_reference_gently(the_repository,
> -							  &oid, 0);
> -	if (!rf->merge_commit)
> +	struct commit *merge_commit = lookup_commit_reference_gently(the_repository,
> +								     &oid, 0);

decl-after-statement here, too.

> +	if (!merge_commit)
>  		return error(_("option `%s' must point to a commit"), opt->long_name);
>  
> +	if (starts_with(opt->long_name, "no"))
> +		commit_list_insert(merge_commit, &rf->unreachable_from);
> +	else
> +		commit_list_insert(merge_commit, &rf->reachable_from);
> +
>  	return 0;
>  }

Thanks.

  reply	other threads:[~2020-09-11 21:47 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-08 15:37 [PATCH] " Aaron Lipman
2020-09-08 22:46 ` Junio C Hamano
2020-09-08 23:57   ` Aaron Lipman
2020-09-09  0:00     ` Junio C Hamano
2020-09-11 18:57 ` [PATCH v2 0/2] git branch: allow combining " Aaron Lipman
2020-09-11 18:57   ` [PATCH v2 1/2] t3201: test multiple branch filter combinations Aaron Lipman
2020-09-11 18:57   ` [PATCH v2 2/2] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-11 21:47     ` Junio C Hamano [this message]
2020-09-13 19:31     ` [PATCH v3 0/3] git branch: allow combining " Aaron Lipman
2020-09-13 19:31       ` [PATCH v3 1/3] t3201: test multiple branch filter combinations Aaron Lipman
2020-09-13 20:58         ` Eric Sunshine
2020-09-14 20:07           ` Junio C Hamano
2020-09-13 19:31       ` [PATCH v3 2/3] Doc: cover multiple contains/no-contains filters Aaron Lipman
2020-09-13 21:10         ` Eric Sunshine
2020-09-14 20:07           ` Junio C Hamano
2020-09-13 19:31       ` [PATCH v3 3/3] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-13 21:36         ` Eric Sunshine
2020-09-13 21:56           ` Junio C Hamano
2020-09-13 22:31             ` Eric Sunshine
2020-09-16  2:08       ` [PATCH v4 0/3] git branch: allow combining " Aaron Lipman
2020-09-16  2:08         ` [PATCH v4 1/3] t3201: test multiple branch filter combinations Aaron Lipman
2020-09-16  2:08         ` [PATCH v4 2/3] Doc: cover multiple contains/no-contains filters Aaron Lipman
2020-09-16 19:45           ` Junio C Hamano
2020-09-16  2:08         ` [PATCH v4 3/3] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-16  4:53         ` [PATCH v4 0/3] git branch: allow combining " Junio C Hamano
2020-09-16  5:08           ` Eric Sunshine
2020-09-18  0:49         ` [PATCH 0/2] ref-filter: merged & no-merged touch-ups Aaron Lipman
2020-09-18  0:49           ` [PATCH 1/2] ref-filter: refactor do_merge_filter() Aaron Lipman
2020-09-18  5:03             ` Eric Sunshine
2020-09-18  5:04               ` Eric Sunshine
2020-09-18 17:01               ` Junio C Hamano
2020-09-18  0:49           ` [PATCH 2/2] Doc: prefer more specific file name Aaron Lipman
2020-09-18  2:52           ` [PATCH 0/2] ref-filter: merged & no-merged touch-ups Eric Sunshine
2020-09-18 21:58           ` [PATCH v2 " Aaron Lipman
2020-09-18 21:58             ` [PATCH v2 1/2] ref-filter: make internal reachable-filter API more precise Aaron Lipman
2020-09-18 21:58             ` [PATCH v2 2/2] Doc: prefer more specific file name Aaron Lipman

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=xmqqlfhgm1ez.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=alipman88@gmail.com \
    --cc=git@vger.kernel.org \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git