Stefan Beller <sbel...@google.com> writes:

> diff --git a/diffcore-blobfind.c b/diffcore-blobfind.c
> new file mode 100644
> index 0000000000..e65c7cad6e
> --- /dev/null
> +++ b/diffcore-blobfind.c
> @@ -0,0 +1,41 @@
> +/*
> + * 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->blobfind)
> +             BUG("blobfind oidset not initialized???");
> +
> +     for (src = dst = 0; src < q->nr; src++) {
> +             struct diff_filepair *p = q->queue[src];
> +
> +             if ((DIFF_FILE_VALID(p->one) &&
> +                  oidset_contains(options->blobfind, &p->one->oid)) ||
> +                 (DIFF_FILE_VALID(p->two) &&
> +                  oidset_contains(options->blobfind, &p->two->oid))) {

Shouldn't this make sure that !DIFF_PAIR_UNMERGED(p) before looking
at the sides of the pair?  I think an unmerged pair is queued with
filespecs whose modes are cleared, but we should not be relying on
that---I think we even had bugs we fixed by introducing an explicit
is_unmerged field to the filepair struct.

> @@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)
>               simplify_merges(revs);
>       if (revs->children.name)
>               set_children(revs);
> +     if (revs->diffopt.blobfind)
> +             revs->simplify_history = 0;
>       return 0;
>  }

It makes sense to clear this bit by default, but is this an
unconditonal clearing?  In other words, is there a way for the user
to countermand this default and ask for merge simplification from
the command line, e.g. "diff --simplify-history --blobfind=<blob>"?

> +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 --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&
> +     cut -c 14- actual.raw >actual &&
> +     test_cmp expect actual

The hardcoded numbers look strange and smell like a workaround of
not asking for full object names.

Also, now it has the simplify-history bit in the change, don't we
want to check a mergy history, and also running "diff-files" while
the index has unmerged entries?

Other than that, the changes look quite sane and pleasnt read.

Thanks.


> +'
> +
> +test_done

Reply via email to