Re: [GSOC][PATCH v2] diff-index: enable sparse index

From: RAGHUL NANTH <nanth.raghul@gmail.com>
Date: Apr 19, 2023, 15:15 UTC
Message-ID: <CAPnUp-=3aoG9WwCcLnMZ4UL90j+snL8qUePPmm02WQK9tUkCzw@mail.gmail.com>
In-Reply-To: <62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com>

On Fri, Apr 14, 2023 at 2:44 AM Victoria Dye <vdye@github.com> wrote:

Show 7 quoted lines

>
> Please include the range-diff comparing the previous version to the new one
> in your future iterations & patch series in general. GitGitGadget adds it by
> default, but if you're using 'send-email' you should be able to use the
> '--range-diff' option to generate it (see MyFirstContribution [1] for more
> information).
>

Yeah, I will keep this in mind. Sorry about that

> Re: my last review [2] - did you look into the behavior of 'diff' with
> pathspecs and whether this 'pathspec_needs_expanded_index()' could be
> centralized (in e.g. 'run_diff_index()')? What did you find?

I hadn't understood the review properly. I just thought you wanted to make sure the function was added to diiff-index itself. I have read through some of it, but I am still not 100% sure of the behaviour. Will run through it more to get more definitive answers

Show 11 quoted lines

> Using '! ensure_not_expanded' will fail if the command expands the index
> _or_ if the command fails altogether, which could inadvertently make these
> tests pass even when there's a breakage in 'diff-index'. An
> 'ensure_expanded' function was created in [3] to test these types of cases;
> you can use that here if you base your branch on 'sl/diff-files-sparse' (see
> SubmittingPatches for more information [4]).
>
> [3] https://lore.kernel.org/git/20230322161820.3609-3-cheskaqiqi@gmail.com/
> [4] https://git-scm.com/docs/SubmittingPatches#base-branch
>
> > +     ! ensure_not_expanded diff-index "**a"

Yeah, I saw this function, but since this wasn't integrated into master, I wasn't sure how I would go about using it. I will base my work off of the mentioned branch for now then. As for making sure the function doesn't give false positives, it should be fine in this current case, since I did try to manually run through the commands just as a guarantee, and that seemed to run fine, but yes, I will make sure to make those updates

Show 7 quoted lines

> Git pathspec syntax [5] does not follow glob rules (without the ':(glob)'
> prefix, at least), so the '**' doesn't do anything special here that a
> single '*' wouldn't do. So, to make it clear that you aren't using glob
> patterns, it might be better to use '*a' instead.
>
> Also, why are the wildcard pathspecs here in double-quotes, but the ones in
> the previous test ('sparse index is not expanded: diff-index') aren't?

The double quotes were just to use the glob provided by pathspec. As for why the previous ones don't have them, they are just using regular pathspecs.

I will make the necessary changes as mentioned here.

Thank you, Raghul