git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 3/5] diff: introduce DIFF_PICKAXE_KINDS_MASK
Date: Wed, 03 Jan 2018 14:01:52 -0800
Message-ID: <xmqqbmiaha9b.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20180103004624.222528-4-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Currently the check whether to perform pickaxing is done via checking
> `diffopt->pickaxe`, which contains the command line argument that we
> want to pickaxe for. Soon we'll introduce a new type of pickaxing, that
> will not store anything in the `.pickaxe` field, so let's migrate the
> check to be dependent on pickaxe_opts.
>
> It is not enough to just replace the check for pickaxe by pickaxe_opts,
> because flags might be set, but pickaxing was not requested ('-i').
> To cope with that, introduce a mask to check only for the bits indicating
> the modes of operation.

The resulting code after this series would leave a few "huh?" if it
were new code, but the series is not making anything worse, so take
this as just something noticed, not as something needs further work.

Because we do not allow "log -S<something> -G<something>", there is
no legitimate reason why they have to be a bit in the pickaxe_opts
flag word.  A single enum that says "We are doing pickaxe search and
_this_ is the kind of pickaxe search we are doing" would suffice,
i.e. the NULL-ness check of rev->diffopt.pickaxe string can be
replaced with a check of that enum field against PICKAXE_NONE or
something that signals us that no pickaxe is in effect.

On the other hand, if somebody comes up with a sensible way to
combine more than one pickaxe queries in a single traversal
(e.g. "log -S<something> -S<somethingelse>" might mean "find a
change that loses or gains <something> and <somethingelse> in the
same commit", or it may mean the same with "...<something> or
<somethingelse>"), then a more sensible data structure to represent
the pickaxe request may have been a list of struct, each of which
records the kind and the parameter (e.g. "-S" and "<something>"
would be in a single struct, and "-S" and "<somethingelse>" would be
in another, and these two are in the list that is diffopt->pickaxe).
The NULL-ness check of rev->diffopt.pickaxe string would be replaced
with a check of the length of that list.

In any case, this step looks sensible.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/log.c  | 4 ++--
>  combine-diff.c | 2 +-
>  diff.c         | 4 ++--
>  diff.h         | 2 ++
>  revision.c     | 2 +-
>  5 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 6c1fa896ad..bd6f2d1efb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -180,8 +180,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>  	if (rev->show_notes)
>  		init_display_notes(&rev->notes_opt);
>  
> -	if (rev->diffopt.pickaxe || rev->diffopt.filter ||
> -	    rev->diffopt.flags.follow_renames)
> +	if ((rev->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) ||
> +	    rev->diffopt.filter || rev->diffopt.flags.follow_renames)
>  		rev->always_show_header = 0;
>  
>  	if (source)
> diff --git a/combine-diff.c b/combine-diff.c
> index 2505de119a..bc08c4c5b1 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1438,7 +1438,7 @@ void diff_tree_combined(const struct object_id *oid,
>  			opt->flags.follow_renames	||
>  			opt->break_opt != -1	||
>  			opt->detect_rename	||
> -			opt->pickaxe		||
> +			(opt->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)	||
>  			opt->filter;
>  
>  
> diff --git a/diff.c b/diff.c
> index 0763e89263..5508745dc8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4173,7 +4173,7 @@ void diff_setup_done(struct diff_options *options)
>  	/*
>  	 * Also pickaxe would not work very well if you do not say recursive
>  	 */
> -	if (options->pickaxe)
> +	if (options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)
>  		options->flags.recursive = 1;
>  	/*
>  	 * When patches are generated, submodules diffed against the work tree
> @@ -5777,7 +5777,7 @@ void diffcore_std(struct diff_options *options)
>  		if (options->break_opt != -1)
>  			diffcore_merge_broken();
>  	}
> -	if (options->pickaxe)
> +	if (options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)
>  		diffcore_pickaxe(options);
>  	if (options->orderfile)
>  		diffcore_order(options->orderfile);
> diff --git a/diff.h b/diff.h
> index 8af1213684..9ec4f824fe 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -326,6 +326,8 @@ extern void diff_setup_done(struct diff_options *);
>  #define DIFF_PICKAXE_KIND_S	4 /* traditional plumbing counter */
>  #define DIFF_PICKAXE_KIND_G	8 /* grep in the patch */
>  
> +#define DIFF_PICKAXE_KINDS_MASK (DIFF_PICKAXE_KIND_S | DIFF_PICKAXE_KIND_G)
> +
>  #define DIFF_PICKAXE_IGNORE_CASE	32
>  
>  extern void diffcore_std(struct diff_options *);
> diff --git a/revision.c b/revision.c
> index ccf1d212ce..5d11ecaf27 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2407,7 +2407,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		revs->diff = 1;
>  
>  	/* Pickaxe, diff-filter and rename following need diffs */
> -	if (revs->diffopt.pickaxe ||
> +	if ((revs->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) ||
>  	    revs->diffopt.filter ||
>  	    revs->diffopt.flags.follow_renames)
>  		revs->diff = 1;

  reply index

Thread overview: 19+ messages in thread (expand / mbox.gz / Atom feed / [top])
2018-01-03  0:46 [PATCH 0/5] pickaxe refactorings and a new mode to find blobs (WAS: diffcore: add a pickaxe option to find a specific blob) Stefan Beller
2018-01-03  0:46 ` [PATCH 1/5] diff.h: Make pickaxe_opts an unsigned bit field Stefan Beller
2018-01-03 21:49   ` Junio C Hamano
2018-01-03  0:46 ` [PATCH 2/5] diff: migrate diff_flags.pickaxe_ignore_case to a pickaxe_opts bit Stefan Beller
2018-01-03  0:46 ` [PATCH 3/5] diff: introduce DIFF_PICKAXE_KINDS_MASK Stefan Beller
2018-01-03 22:01   ` Junio C Hamano [this message]
2018-01-03  0:46 ` [PATCH 4/5] diffcore: add a pickaxe option to find a specific blob Stefan Beller
2018-01-03  0:56   ` [PATCHv2 " Stefan Beller
2018-01-03  0:46 ` [PATCH 5/5] diff: properly error out when combining multiple pickaxe options Stefan Beller
2018-01-03  0:58   ` Jacob Keller
2018-01-03 22:08   ` Junio C Hamano
2018-01-03 22:18     ` Stefan Beller
2018-01-04 22:50 ` [PATCHv2 0/6] pickaxe refactorings and a new mode to find blobs (WAS: diffcore: add a pickaxe option to find a specific blob) Stefan Beller
2018-01-04 22:50   ` [PATCHv2 1/6] diff.h: Make pickaxe_opts an unsigned bit field Stefan Beller
2018-01-04 22:50   ` [PATCHv2 2/6] diff: migrate diff_flags.pickaxe_ignore_case to a pickaxe_opts bit Stefan Beller
2018-01-04 22:50   ` [PATCHv2 3/6] diff: introduce DIFF_PICKAXE_KINDS_MASK Stefan Beller
2018-01-04 22:50   ` [PATCHv2 4/6] diffcore: add a pickaxe option to find a specific blob Stefan Beller
2018-01-04 22:50   ` [PATCHv2 5/6] diff: properly error out when combining multiple pickaxe options Stefan Beller
2018-01-04 22:50   ` [PATCHv2 6/6] diff: use HAS_MULTI_BITS instead of counting bits manually Stefan Beller

Reply instructions:

You may reply publically 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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqbmiaha9b.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.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

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

Archives are clonable:
	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

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.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

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