On Sun, Sep 9, 2018 at 1:02 AM Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>
> On Tue, Sep 4, 2018 at 9:54 PM Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> >
> > 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.
>
> Actually I looked at it again and diff_setup() clears diffopts as the
> first step, so any assignments prior to the diff_setup() call has no
> effect.
Eh, I botched that code in email, I meant to put it between
diff_setup(&diffopts);
diffopts.repo = <a repo>
diff_setup_done(&diffopts);
but I still agree with you that we want to have the repository as a
non-optional field.
>
> > > 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.
>
> Yes. But the the_hash_algo currently points to
> the_repository->hash_algo. We need to at least make the_hash_algo
> independent from 'struct repository *', then figure out how to set
> the_hash_algo without $GIT_DIR/config file...
That sounds as if it is unwise to rely on the_hash_algo and
rather prefer the repositories configured hash algorithm instead;
only falling back to the_hash_algo when we have no repository
handle available.
Stefan
