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

2018-09-10 Thread Stefan Beller
On Sun, Sep 9, 2018 at 1:02 AM Duy Nguyen  wrote:
>
> On Tue, Sep 4, 2018 at 9:54 PM Stefan Beller  wrote:
> >
> > On Mon, Sep 3, 2018 at 11:03 AM Duy Nguyen  wrote:
> > >
> > > On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller  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 = 
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


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

2018-09-09 Thread Duy Nguyen
On Tue, Sep 4, 2018 at 9:54 PM Stefan Beller  wrote:
>
> On Mon, Sep 3, 2018 at 11:03 AM Duy Nguyen  wrote:
> >
> > On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller  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.

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


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

2018-09-04 Thread Stefan Beller
On Mon, Sep 3, 2018 at 11:03 AM Duy Nguyen  wrote:
>
> On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller  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


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

2018-09-03 Thread Duy Nguyen
On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller  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.

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


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

2018-08-27 Thread Stefan Beller
> -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)


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

2018-08-26 Thread Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/log.c | 2 +-
 patch-ids.c   | 4 ++--
 patch-ids.h   | 3 ++-
 revision.c| 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 33084102e0..2910122d90 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -914,7 +914,7 @@ static void get_patch_ids(struct rev_info *rev, struct 
patch_ids *ids)
if ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))
die(_("Not a range."));
 
-   init_patch_ids(ids);
+   init_patch_ids(ids, the_repository);
 
/* given a range a..b get all patch ids for b..a */
init_revisions(&check_rev, rev->prefix);
diff --git a/patch-ids.c b/patch-ids.c
index 9084604c77..72ef9e4e8a 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -56,10 +56,10 @@ static int patch_id_cmp(const void *cmpfn_data,
return oidcmp(&a->patch_id, &b->patch_id);
 }
 
-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);
ids->diffopts.detect_rename = 0;
ids->diffopts.flags.recursive = 1;
diff_setup_done(&ids->diffopts);
diff --git a/patch-ids.h b/patch-ids.h
index 79ac9a8498..1d4cf618f9 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -6,6 +6,7 @@
 
 struct commit;
 struct object_id;
+struct repository;
 
 struct patch_id {
struct hashmap_entry ent;
@@ -20,7 +21,7 @@ struct patch_ids {
 
 int commit_patch_id(struct commit *commit, struct diff_options *options,
struct object_id *oid, int);
-int init_patch_ids(struct patch_ids *);
+int init_patch_ids(struct patch_ids *, struct repository *);
 int free_patch_ids(struct patch_ids *);
 struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);
 struct patch_id *has_commit_patch_id(struct commit *, struct patch_ids *);
diff --git a/revision.c b/revision.c
index bfcb0f2a00..d4a539dc69 100644
--- a/revision.c
+++ b/revision.c
@@ -877,7 +877,7 @@ static void cherry_pick_list(struct commit_list *list, 
struct rev_info *revs)
return;
 
left_first = left_count < right_count;
-   init_patch_ids(&ids);
+   init_patch_ids(&ids, the_repository);
ids.diffopts.pathspec = revs->diffopt.pathspec;
 
/* Compute patch-ids for one side */
-- 
2.19.0.rc0.337.ge906d732e7