Re: [PATCH] diffcore: add a filter to find a specific blob

Hi,

Stefan Beller wrote:

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.

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?

[...]

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