git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com, peff@peff.net
Subject: Re: [PATCH] diffcore: add a filter to find a specific blob
Date: Thu, 14 Dec 2017 13:22:34 -0800	[thread overview]
Message-ID: <20171214212234.GC32842@aiede.mtv.corp.google.com> (raw)
In-Reply-To: <20171212012422.123332-1-sbeller@google.com>

Hi,

Stefan Beller wrote:

> Junio hinted at a different approach of solving this problem, which this
> patch implements. Teach the diff machinery another flag for restricting
> the information to what is shown. For example:
>
>   $ ./git log --oneline --find-object=v2.0.0:Makefile
>   b2feb64309 Revert the whole "ask curl-config" topic for now
>   47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"
>
> we observe that the Makefile as shipped with 2.0 was appeared in
> v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.

Neat!

Nit: s/was appeared/appeared/ (not a big deal though, since this is
already in 'next').

From the above it's not clear to me whether this is about commits
where the object was added or where the object was removed.
Fortunately, the docs are a bit clearer:

                                 ... one side of the diff
       matches the given object id. The object can be a blob,
       gitlink entry or tree (when `-t` is given).

So this appears to be about both additions and removals.  Followup
questions:

- are copies and renames shown (if I am passing -M -C)?
- what about mode changes?  If the file became executable but the
  blob content didn't change, does that commit match?

Nit, not related to this change: it would be nice to have a long
option to go along with the short name '-t' --- e.g. --include-trees.

Another nit: s/gitlink entry/submodule commit/, perhaps.  The commit
object is not a gitlink entry: it is pointed to by a gitlink entry.

Another documentation idea: it may be nice to point out that this
is only about the preimage and postimage submodule commit and that
it doesn't look at the history in between.

>                                                          The
> reason why these commits both occur prior to v2.0.0 are evil
> merges that are not found using this new mechanism.

Would it be worth the doc mentioning that this doesn't look at merges
unless you use one of the -m/-c/--cc options, or does that go without
saying?

[...]
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -500,6 +500,12 @@ information.
>  --pickaxe-regex::
>  	Treat the <string> given to `-S` as an extended POSIX regular
>  	expression to match.
> +
> +--find-object=<object-id>::
> +	Restrict the output such that one side of the diff
> +	matches the given object id. The object can be a blob,
> +	gitlink entry or tree (when `-t` is given).

I like this name --find-object more than --blobfind!  I am not sure it
quite matches what the user is looking for, though.  We are not
looking for all occurences of the object; we only care about when the
object appears (was added or removed) in the diff.

Putting it in context, we have:

	pickaxe options:
	-S: detect changes in occurence count of a string
	-G: grep lines in diff for a string

	--pickaxe-all:
		do not filter the diff when the patch matches pickaxe
		conditions.

		kind of like log --full-diff, but restricted to pickaxe
		options.
	--pickaxe-regex: treat -S argument as a regex, not a string

Is this another kind of pickaxe option?  It feels similar to -S, but
at an object level instead of a substring level, so in a way it would
be appealing to call it --pickaxe-object.  Does --pickaxe-all affect
it like it affects -S and -G?

Another context to put it in is:

	--diff-filter:
		limit paths (but not commits?) to those with a change
		matching optarg

If I understand correctly, then --diff-filter does not interact with
--pickaxe-all, or in other words it is a different filtering
condition.  Is this another kind of diff filter?  In that context, it
may be appealing to call it something like --object-filter.

--diff-filter is an example where it seems appealing to have a
--full-diff option to diff-tree that could apply to all filters and
not just pickaxe.

[... implementation snipped ...]

The implementation looks lovely and I'm especially happy about the
tests.  Thanks for writing it.

Thoughts?
Jonathan

  parent reply	other threads:[~2017-12-14 21:22 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-08  0:24 [PATCH 0/1] diffcore-blobfind Stefan Beller
2017-12-08  0:24 ` [PATCH 1/1] diffcore: add a filter to find a specific blob Stefan Beller
2017-12-08  9:34   ` Jeff King
2017-12-08 16:28     ` Ramsay Jones
2017-12-08 20:19       ` Jeff King
2017-12-08 20:39         ` Stefan Beller
2017-12-08 21:38           ` Jeff King
2017-12-08 15:04   ` Junio C Hamano
2017-12-08 17:21     ` Junio C Hamano
2017-12-08 21:11     ` Stefan Beller
2017-12-08 21:15       ` Junio C Hamano
2017-12-11 19:58 ` [PATCH 0/1] diff-core blobfind Stefan Beller
2017-12-11 19:58   ` [PATCH 1/1] diffcore: add a filter to find a specific blob Stefan Beller
2017-12-11 23:17     ` Junio C Hamano
2017-12-12  0:21       ` Stefan Beller
2017-12-12  1:24         ` [PATCH] " Stefan Beller
2017-12-12 18:36           ` Junio C Hamano
2017-12-14 21:22           ` Jonathan Nieder [this message]
2017-12-14 22:30             ` Stefan Beller
2017-12-14 22:52               ` Jonathan Nieder
2017-12-15  2:18                 ` Junio C Hamano
2017-12-27 18:49                   ` Stefan Beller
2017-12-27 18:59                     ` Jonathan Nieder
2017-12-27 19:57                       ` Junio C Hamano
2017-12-14 22: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=20171214212234.GC32842@aiede.mtv.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --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
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).