Re: [PATCH v3 2/2] blame: enable and test the sparse index

From: Elijah Newren <newren@gmail.com>
Date: Nov 21, 2021, 01:46 UTC
Message-ID: <CABPp-BH2O+fezCui0utYbpShp6NaMwWapyr8i+mDDjjhzec4LQ@mail.gmail.com>
In-Reply-To: <xmqqh7ct89us.fsf@gitster.g>

On Wed, Nov 3, 2021 at 9:47 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > We do not include paths outside the sparse checkout cone because blame
> > currently does not support blaming files outside of the sparse definition.
> > Attempting to do so fails with the following error:
> >
> >   fatal: no such path '<path outside sparse definition>' in HEAD
>
> Does this indicate that we need to update how the command line
> safety in verify_working_tree_path() works in a sparsely checked out
> working tree?

I wondered the same thing, but no I don't think we need any extra command line safety here. The behavior Lessley is reporting here is equivalent to the following in a regular (non-sparse) checkout:

$ rm t/test-lib.sh $ git blame t/test-lib.sh fatal: Cannot lstat 't/test-lib.sh': No such file or directory $ git blame -- t/test-lib.sh fatal: Cannot lstat 't/test-lib.sh': No such file or directory

blame without a revision has always failed for files not in the working tree, regardless of whether those files are found in the index or HEAD.

>  If foo/bar is outside the sparse definition,
>
>     git blame HEAD foo/bar

Actually, that works; there's no error and the code with Lessley's patch will show the blame info for foo/bar (assuming foo/bar was a path in HEAD, of course).

Show 5 quoted lines

> may get such a message, but shouldn't
>
>     git blame HEAD -- foo/bar
>
> make it work?

This also works. But both of these things are kind of testing something different; when given a revision, the checkout is irrelevant to git blame: git blame with a revision will work regardless of whether the checkout is full, sparse, completely empty, or non-existent (i.e. a bare clone).

Show 8 quoted lines

> > -# TODO: blame currently does not support blaming files outside of the
> > -# sparse definition. It complains that the file doesn't exist locally.
> > -test_expect_failure 'blame with pathspec outside sparse definition' '
> > +# Blame does not support blaming files outside of the sparse
> > +# definition, so we verify this scenario.
>
> IOW, why is it a good idea to drop the "TODO" and "currently" and pretend
> as if the current behaviour is the desirable one?

I think dropping the TODO is correct, but the wording is confusing -- it has nothing to do with sparse checkouts. I'd rather say, "Without a specified revision, blame will only handle files present in the current working directory and error on any other paths"

Show 44 quoted lines

> > +test_expect_success 'blame with pathspec outside sparse definition' '
> >       init_repos &&
> > +     test_sparse_match git sparse-checkout set &&
> >
> > -     test_all_match git blame folder1/a &&
> > -     test_all_match git blame folder2/a &&
> > -     test_all_match git blame deep/deeper2/a &&
> > -     test_all_match git blame deep/deeper2/deepest/a
> > +     for file in a \
> > +                     deep/a \
> > +                     deep/deeper1/a \
> > +                     deep/deeper1/deepest/a
> > +     do
> > +             test_sparse_match test_must_fail git blame $file &&
> > +             cat >expect <<-EOF &&
> > +             fatal: Cannot lstat '"'"'$file'"'"': No such file or directory
> > +             EOF
> > +             # We compare sparse-checkout-err and sparse-index-err in
> > +             # `test_sparse_match`. Given we know they are the same, we
> > +             # only check the content of sparse-index-err here.
> > +             test_cmp expect sparse-index-err
> > +     done
> >  '
> >
> >  test_expect_success 'checkout and reset (mixed)' '
> > @@ -878,6 +892,18 @@ test_expect_success 'sparse index is not expanded: diff' '
> >       ensure_not_expanded diff --staged
> >  '
> >
> > +test_expect_success 'sparse index is not expanded: blame' '
> > +     init_repos &&
> > +
> > +     for file in a \
> > +                     deep/a \
> > +                     deep/deeper1/a \
> > +                     deep/deeper1/deepest/a
> > +     do
> > +             ensure_not_expanded blame $file
> > +     done
> > +'
> > +
> >  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> >  # in this scenario, but it shouldn't.
> >  test_expect_success 'reset mixed and checkout orphan' '