* [PATCH 0/1] Add blob find pickaxe option
@ 2017-12-28 0:00 Stefan Beller
2017-12-28 0:00 ` [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob Stefan Beller
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Beller @ 2017-12-28 0:00 UTC (permalink / raw)
To: git; +Cc: gitster, jrnieder, Stefan Beller
This is a resend for sb/diff-blobfind, but now the option --find-object
is a native of the pickaxe family.
Stefan Beller (1):
diffcore: add a pickaxe option to find a specific blob
Documentation/diff-options.txt | 10 +++++++
diff.c | 21 ++++++++++++-
diff.h | 3 ++
diffcore-pickaxe.c | 14 +++++++--
revision.c | 3 ++
t/t4064-diff-oidfind.sh | 68 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 115 insertions(+), 4 deletions(-)
create mode 100755 t/t4064-diff-oidfind.sh
--
2.15.1.620.gb9897f4670-goog
diff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt
index 21d1776996..f9cf85db05 100644
--- c/Documentation/diff-options.txt
+++ w/Documentation/diff-options.txt
@@ -492,6 +492,15 @@ occurrences of that string did not change).
See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
information.
+--find-object=<object-id>::
+ Look for differences that change the number of occurrences of
+ the specified object. Similar to `-S`, just the argument is different
+ in that it doesn't search for a specific string but for a specific
+ object id.
++
+The object can be a blob or a submodule commit. It implies the `-t` option in
+`git-log` to also find trees.
+
--pickaxe-all::
When `-S` or `-G` finds a change, show all the changes in that
changeset, not just the files that contain the change
@@ -501,11 +510,6 @@ information.
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).
-
endif::git-format-patch[]
-O<orderfile>::
diff --git c/Makefile w/Makefile
index c26596c30a..ee9d5eb11e 100644
--- c/Makefile
+++ w/Makefile
@@ -775,7 +775,6 @@ LIB_OBJS += date.o
LIB_OBJS += decorate.o
LIB_OBJS += diffcore-break.o
LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-objfind.o
LIB_OBJS += diffcore-order.o
LIB_OBJS += diffcore-pickaxe.o
LIB_OBJS += diffcore-rename.o
diff --git c/builtin/log.c w/builtin/log.c
index 08ea82d69f..6c1fa896ad 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
init_display_notes(&rev->notes_opt);
if (rev->diffopt.pickaxe || rev->diffopt.filter ||
- rev->diffopt.flags.follow_renames || rev->diffopt.objfind)
+ rev->diffopt.flags.follow_renames)
rev->always_show_header = 0;
if (source)
diff --git c/diff.c w/diff.c
index e13b8229d3..7acddaaee7 100644
--- c/diff.c
+++ w/diff.c
@@ -4497,6 +4497,10 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg)
if (!opt->objfind)
opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+
+ opt->pickaxe = ""; /* trigger pickaxe */
+ opt->flags.recursive = 1;
+ opt->flags.tree_in_recursive = 1;
oidset_insert(opt->objfind, &oid);
return 1;
}
@@ -5785,9 +5789,6 @@ void diffcore_std(struct diff_options *options)
diffcore_skip_stat_unmatch(options);
if (!options->found_follow) {
/* See try_to_follow_renames() in tree-diff.c */
-
- if (options->objfind)
- diffcore_objfind(options);
if (options->break_opt != -1)
diffcore_break(options->break_opt);
if (options->detect_rename)
diff --git c/diffcore-objfind.c w/diffcore-objfind.c
deleted file mode 100644
index 676bbfff00..0000000000
--- c/diffcore-objfind.c
+++ /dev/null
@@ -1,42 +0,0 @@
-/*
- * Copyright (c) 2017 Google Inc.
- */
-#include "cache.h"
-#include "diff.h"
-#include "diffcore.h"
-
-static void diffcore_filter_blobs(struct diff_queue_struct *q,
- struct diff_options *options)
-{
- int src, dst;
-
- if (!options->objfind)
- BUG("objfind oidset not initialized???");
-
- for (src = dst = 0; src < q->nr; src++) {
- struct diff_filepair *p = q->queue[src];
-
- if (!DIFF_PAIR_UNMERGED(p) &&
- ((DIFF_FILE_VALID(p->one) &&
- oidset_contains(options->objfind, &p->one->oid)) ||
- (DIFF_FILE_VALID(p->two) &&
- oidset_contains(options->objfind, &p->two->oid)))) {
- q->queue[dst] = p;
- dst++;
- } else {
- diff_free_filepair(p);
- }
- }
-
- if (!dst) {
- free(q->queue);
- DIFF_QUEUE_CLEAR(q);
- } else {
- q->nr = dst;
- }
-}
-
-void diffcore_objfind(struct diff_options *options)
-{
- diffcore_filter_blobs(&diff_queued_diff, options);
-}
diff --git c/diffcore-pickaxe.c w/diffcore-pickaxe.c
index 9476bd2108..0d0c697ae7 100644
--- c/diffcore-pickaxe.c
+++ w/diffcore-pickaxe.c
@@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
mmfile_t mf1, mf2;
int ret;
- if (!o->pickaxe[0])
- return 0;
-
/* ignore unmerged */
if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))
return 0;
+ if (o->objfind) {
+ if ((DIFF_FILE_VALID(p->one) &&
+ oidset_contains(o->objfind, &p->one->oid)) ||
+ (DIFF_FILE_VALID(p->two) &&
+ oidset_contains(o->objfind, &p->two->oid)))
+ return 1;
+ }
+
+ if (!o->pickaxe[0])
+ return 0;
+
if (o->flags.allow_textconv) {
textconv_one = get_textconv(p->one);
textconv_two = get_textconv(p->two);
diff --git c/diffcore.h w/diffcore.h
index cbde777bdd..a30da161da 100644
--- c/diffcore.h
+++ w/diffcore.h
@@ -107,7 +107,6 @@ extern struct diff_filepair *diff_queue(struct diff_queue_struct *,
struct diff_filespec *);
extern void diff_q(struct diff_queue_struct *, struct diff_filepair *);
-extern void diffcore_objfind(struct diff_options *);
extern void diffcore_break(int);
extern void diffcore_rename(struct diff_options *);
extern void diffcore_merge_broken(void);
diff --git c/revision.c w/revision.c
index 0a797bdfc7..43bb6265cd 100644
--- c/revision.c
+++ w/revision.c
@@ -2409,10 +2409,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
/* Pickaxe, diff-filter and rename following need diffs */
if (revs->diffopt.pickaxe ||
revs->diffopt.filter ||
- revs->diffopt.flags.follow_renames ||
- revs->diffopt.objfind)
+ revs->diffopt.flags.follow_renames)
revs->diff = 1;
+ if (revs->diffopt.objfind)
+ revs->simplify_history = 0;
+
if (revs->topo_order)
revs->limited = 1;
@@ -2884,8 +2886,6 @@ int prepare_revision_walk(struct rev_info *revs)
simplify_merges(revs);
if (revs->children.name)
set_children(revs);
- if (revs->diffopt.objfind)
- revs->simplify_history = 0;
return 0;
}
diff --git c/t/helper/test-lazy-init-name-hash.c w/t/helper/test-lazy-init-name-hash.c
index 6368a89345..297fb01d61 100644
--- c/t/helper/test-lazy-init-name-hash.c
+++ w/t/helper/test-lazy-init-name-hash.c
@@ -112,7 +112,7 @@ static void analyze_run(void)
{
uint64_t t1s, t1m, t2s, t2m;
int cache_nr_limit;
- int nr_threads_used;
+ int nr_threads_used = 0;
int i;
int nr;
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob
2017-12-28 0:00 [PATCH 0/1] Add blob find pickaxe option Stefan Beller
@ 2017-12-28 0:00 ` Stefan Beller
2017-12-28 21:33 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Beller @ 2017-12-28 0:00 UTC (permalink / raw)
To: git; +Cc: gitster, jrnieder, Stefan Beller
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
One might be tempted to extend git-describe to also work with blobs,
such that `git describe <blob-id>` gives a description as
'<commit-ish>:<path>'. This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.
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. The
reason why these commits both occur prior to v2.0.0 are evil
merges that are not found using this new mechanism.
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbeller@google.com/
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/diff-options.txt | 10 +++++++
diff.c | 21 ++++++++++++-
diff.h | 3 ++
diffcore-pickaxe.c | 14 +++++++--
revision.c | 3 ++
t/t4064-diff-oidfind.sh | 68 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 115 insertions(+), 4 deletions(-)
create mode 100755 t/t4064-diff-oidfind.sh
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..f9cf85db05 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -492,6 +492,15 @@ occurrences of that string did not change).
See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
information.
+--find-object=<object-id>::
+ Look for differences that change the number of occurrences of
+ the specified object. Similar to `-S`, just the argument is different
+ in that it doesn't search for a specific string but for a specific
+ object id.
++
+The object can be a blob or a submodule commit. It implies the `-t` option in
+`git-log` to also find trees.
+
--pickaxe-all::
When `-S` or `-G` finds a change, show all the changes in that
changeset, not just the files that contain the change
@@ -500,6 +509,7 @@ information.
--pickaxe-regex::
Treat the <string> given to `-S` as an extended POSIX regular
expression to match.
+
endif::git-format-patch[]
-O<orderfile>::
diff --git a/diff.c b/diff.c
index 0763e89263..7acddaaee7 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+ options->objfind = NULL;
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,23 @@ static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *ar
return 1;
}
+static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+{
+ struct object_id oid;
+
+ if (get_oid(arg, &oid))
+ return error("unable to resolve '%s'", arg);
+
+ if (!opt->objfind)
+ opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+
+ opt->pickaxe = ""; /* trigger pickaxe */
+ opt->flags.recursive = 1;
+ opt->flags.tree_in_recursive = 1;
+ oidset_insert(opt->objfind, &oid);
+ return 1;
+}
+
int diff_opt_parse(struct diff_options *options,
const char **av, int ac, const char *prefix)
{
@@ -4736,7 +4754,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, &optarg))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
- }
+ } else if (skip_prefix(arg, "--find-object=", &arg))
+ return parse_objfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
diff --git a/diff.h b/diff.h
index 0fb18dd735..8fa2ad8e2d 100644
--- a/diff.h
+++ b/diff.h
@@ -7,6 +7,7 @@
#include "tree-walk.h"
#include "pathspec.h"
#include "object.h"
+#include "oidset.h"
struct rev_info;
struct diff_options;
@@ -174,6 +175,8 @@ struct diff_options {
enum diff_words_type word_diff;
enum diff_submodule_format submodule_format;
+ struct oidset *objfind;
+
/* this is set by diffcore for DIFF_FORMAT_PATCH */
int found_changes;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 9476bd2108..0d0c697ae7 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
mmfile_t mf1, mf2;
int ret;
- if (!o->pickaxe[0])
- return 0;
-
/* ignore unmerged */
if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))
return 0;
+ if (o->objfind) {
+ if ((DIFF_FILE_VALID(p->one) &&
+ oidset_contains(o->objfind, &p->one->oid)) ||
+ (DIFF_FILE_VALID(p->two) &&
+ oidset_contains(o->objfind, &p->two->oid)))
+ return 1;
+ }
+
+ if (!o->pickaxe[0])
+ return 0;
+
if (o->flags.allow_textconv) {
textconv_one = get_textconv(p->one);
textconv_two = get_textconv(p->two);
diff --git a/revision.c b/revision.c
index e2e691dd5a..43bb6265cd 100644
--- a/revision.c
+++ b/revision.c
@@ -2412,6 +2412,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
revs->diffopt.flags.follow_renames)
revs->diff = 1;
+ if (revs->diffopt.objfind)
+ revs->simplify_history = 0;
+
if (revs->topo_order)
revs->limited = 1;
diff --git a/t/t4064-diff-oidfind.sh b/t/t4064-diff-oidfind.sh
new file mode 100755
index 0000000000..3bdf317af8
--- /dev/null
+++ b/t/t4064-diff-oidfind.sh
@@ -0,0 +1,68 @@
+#!/bin/sh
+
+test_description='test finding specific blobs in the revision walking'
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+ git commit --allow-empty -m "empty initial commit" &&
+
+ echo "Hello, world!" >greeting &&
+ git add greeting &&
+ git commit -m "add the greeting blob" && # borrowed from Git from the Bottom Up
+ git tag -m "the blob" greeting $(git rev-parse HEAD:greeting) &&
+
+ echo asdf >unrelated &&
+ git add unrelated &&
+ git commit -m "unrelated history" &&
+
+ git revert HEAD^ &&
+
+ git commit --allow-empty -m "another unrelated commit"
+'
+
+test_expect_success 'find the greeting blob' '
+ cat >expect <<-EOF &&
+ Revert "add the greeting blob"
+ add the greeting blob
+ EOF
+
+ git log --format=%s --find-object=greeting^{blob} >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'setup a tree' '
+ mkdir a &&
+ echo asdf >a/file &&
+ git add a/file &&
+ git commit -m "add a file in a subdirectory"
+'
+
+test_expect_success 'find a tree' '
+ cat >expect <<-EOF &&
+ add a file in a subdirectory
+ EOF
+
+ git log --format=%s -t --find-object=HEAD:a >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'setup a submodule' '
+ test_create_repo sub &&
+ test_commit -C sub sub &&
+ git submodule add ./sub sub &&
+ git commit -a -m "add sub"
+'
+
+test_expect_success 'find a submodule' '
+ cat >expect <<-EOF &&
+ add sub
+ EOF
+
+ git log --format=%s --find-object=HEAD:sub >actual &&
+
+ test_cmp expect actual
+'
+
+test_done
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob
2017-12-28 0:00 ` [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob Stefan Beller
@ 2017-12-28 21:33 ` Junio C Hamano
2017-12-28 21:43 ` Stefan Beller
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2017-12-28 21:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, jrnieder
Stefan Beller <sbeller@google.com> writes:
> +static int parse_objfind_opt(struct diff_options *opt, const char *arg)
> +{
> + struct object_id oid;
> +
> + if (get_oid(arg, &oid))
> + return error("unable to resolve '%s'", arg);
> +
> + if (!opt->objfind)
> + opt->objfind = xcalloc(1, sizeof(*opt->objfind));
> +
> + opt->pickaxe = ""; /* trigger pickaxe */
Hmmm. This feels like an ugly hack to me. Do we declare that "git
log -S''" never matches anything right now (if that is the case the
I do not think there is any compatibility issues)?
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 9476bd2108..0d0c697ae7 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
> mmfile_t mf1, mf2;
> int ret;
>
> - if (!o->pickaxe[0])
> - return 0;
> -
> /* ignore unmerged */
> if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))
> return 0;
>
> + if (o->objfind) {
> + if ((DIFF_FILE_VALID(p->one) &&
> + oidset_contains(o->objfind, &p->one->oid)) ||
> + (DIFF_FILE_VALID(p->two) &&
> + oidset_contains(o->objfind, &p->two->oid)))
> + return 1;
> + }
> +
> + if (!o->pickaxe[0])
> + return 0;
> +
Very nice. With just one place, the code covers both cases with and
without pickaxe-all option in effect.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob
2017-12-28 21:33 ` Junio C Hamano
@ 2017-12-28 21:43 ` Stefan Beller
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Beller @ 2017-12-28 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
On Thu, Dec 28, 2017 at 1:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +static int parse_objfind_opt(struct diff_options *opt, const char *arg)
>> +{
>> + struct object_id oid;
>> +
>> + if (get_oid(arg, &oid))
>> + return error("unable to resolve '%s'", arg);
>> +
>> + if (!opt->objfind)
>> + opt->objfind = xcalloc(1, sizeof(*opt->objfind));
>> +
>> + opt->pickaxe = ""; /* trigger pickaxe */
>
> Hmmm. This feels like an ugly hack to me.
Because it is.
$ git grep pickaxe diff.h
diff.h:94: unsigned pickaxe_ignore_case:1;
diff.h:130: const char *pickaxe;
diff.h:149: int pickaxe_opts;
diff.h:361:" --pickaxe-all\n" \
We could promote pickaxe_opts to be the important part
for pickaxing, currently we're using `pickaxe` in diff.c to
make a decision:
if (options->pickaxe)
diffcore_pickaxe(options);
So I think I'll cleanup the pickaxe_opts to be a real
unsigned flags field (such that pickaxe_ignore_case
could be part of it, too!) and put a new flag in
there in addition to DIFF_PICKAXE_KIND_{G, S}
> Do we declare that "git
> log -S''" never matches anything right now (if that is the case the
> I do not think there is any compatibility issues)?
Let's not go this route.
>
>> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
>> index 9476bd2108..0d0c697ae7 100644
>> --- a/diffcore-pickaxe.c
>> +++ b/diffcore-pickaxe.c
>> @@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
>> mmfile_t mf1, mf2;
>> int ret;
>>
>> - if (!o->pickaxe[0])
>> - return 0;
>> -
>> /* ignore unmerged */
>> if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))
>> return 0;
>>
>> + if (o->objfind) {
>> + if ((DIFF_FILE_VALID(p->one) &&
>> + oidset_contains(o->objfind, &p->one->oid)) ||
>> + (DIFF_FILE_VALID(p->two) &&
>> + oidset_contains(o->objfind, &p->two->oid)))
>> + return 1;
>> + }
>> +
>> + if (!o->pickaxe[0])
>> + return 0;
>> +
>
> Very nice. With just one place, the code covers both cases with and
> without pickaxe-all option in effect.
and this is already fixed regarding the ugly hack, too.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-28 21:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-28 0:00 [PATCH 0/1] Add blob find pickaxe option Stefan Beller
2017-12-28 0:00 ` [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob Stefan Beller
2017-12-28 21:33 ` Junio C Hamano
2017-12-28 21:43 ` Stefan Beller
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).