Re: [PATCH 12/21] patch-ids.c: remove implicit dependency on the_index

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 3, 2018, 18:03 UTC
Message-ID: <CACsJy8D9ZMGA8FDmgcNMbWH7VP5O8bfn7tLnXda2TcQKmLM5NA@mail.gmail.com>
In-Reply-To: <CAGZ79kb=gcmEDGmTS-j-veFj8BgKup0g96i0Gm1P+j8eyX8_mQ@mail.gmail.com>

On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller <sbeller@google.com> wrote:

Show 29 quoted lines

>
> > -int init_patch_ids(struct patch_ids *ids)
> > +int init_patch_ids(struct patch_ids *ids, struct repository *repo)
> >  {
> >         memset(ids, 0, sizeof(*ids));
> > -       diff_setup(&ids->diffopts, the_repository);
> > +       diff_setup(&ids->diffopts, repo);
>
> Just realized when looking at this diff, though it applies to
> other patches as well. (and reading Documentation/technical/api-diff.txt
> confirms my thinking IMHO)
>
> What makes the repository argument any special compared
> to the rest of the diff options?
>
> So I would expect the setup to look like
>
>     memset(ids, 0, sizeof(*ids));
>     ids->diffopts->repo = the_repository;
>     diff_setup(&ids->diffopts);
>
> here and in diff_setup, we'd have
>
>   if (!options->repo)
>     options->repo = the_repository;
>
> or even put the_repository into default_diff_options,
> but then I wonder how this deals with no-repo invocations
> (git diff --no-index examples for bug reports)

That makes "repo" field optional and I'm very much against falling back to the_repository. revisions.c in the end does not have any the_repository reference, and it's actually undefined for most files. This makes accidentally adding the_repository back much more difficult.

Yes the --no-index stuff will have to be taken care of at some point, but I think for now we could just put "struct repository *" in place first to see what it looks like, then go from there.

-- 
Duy