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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 3, 2018 at 11:03 AM Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>
> On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> >
> > > -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.

Thanks for the clear explanation. I agree that this is a good approach
with these reasons given. So in case a resend is needed, maybe add
these to the commit message, as it explains why we deviate from
the pattern here.

> 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.

I would think repo = NULL would do? But we can defer this
discussion to later.

Thanks,
Stefan



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux