| Message ID | 20190403113457.20399-11-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nd/sha1-name-c-wo-the-repository updates | expand |
On Wed, Apr 03, 2019 at 06:34:35PM +0700, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Oh, look, an empty commit message ;) > --- > commit.c | 5 +++-- > commit.h | 3 ++- > contrib/coccinelle/commit.cocci | 4 ++-- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/commit.c b/commit.c > index a5333c7ac6..f0a5506f04 100644 > --- a/commit.c > +++ b/commit.c > @@ -340,7 +340,8 @@ void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit) > } > } > > -struct tree *get_commit_tree(const struct commit *commit) > +struct tree *repo_get_commit_tree(struct repository *r, > + const struct commit *commit) A rename to accomodate the additional 'struct repository*' parameter. Ok. > { > if (commit->maybe_tree || !commit->object.parsed) > return commit->maybe_tree; > @@ -348,7 +349,7 @@ struct tree *get_commit_tree(const struct commit *commit) > if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH) > BUG("commit has NULL tree, but was not loaded from commit-graph"); > > - return get_commit_tree_in_graph(the_repository, commit); > + return get_commit_tree_in_graph(r, commit); > } > > struct object_id *get_commit_tree_oid(const struct commit *commit) > diff --git a/commit.h b/commit.h > index 42728c2906..b576201be8 100644 > --- a/commit.h > +++ b/commit.h > @@ -143,7 +143,8 @@ void repo_unuse_commit_buffer(struct repository *r, > */ > void free_commit_buffer(struct parsed_object_pool *pool, struct commit *); > > -struct tree *get_commit_tree(const struct commit *); > +struct tree *repo_get_commit_tree(struct repository *, const struct commit *); > +#define get_commit_tree(c) repo_get_commit_tree(the_repository, c) But we still keep 'get_commit_tree' around as a macro wrapper defaulting to 'the_repository', leaving the 30+ callsites intact. Good. Furthermore, the comment describing the 'maybe_tree' field in 'struct commit's declaration still suggest get_commit_tree(): * If the commit is loaded from the commit-graph file, then this * member may be NULL. Only access it through get_commit_tree() * or get_commit_tree_oid(). > struct object_id *get_commit_tree_oid(const struct commit *); > > /* > diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci > index c49aa558f0..f5bc639981 100644 > --- a/contrib/coccinelle/commit.cocci > +++ b/contrib/coccinelle/commit.cocci > @@ -12,12 +12,12 @@ expression c; > > // These excluded functions must access c->maybe_tree direcly. > @@ > -identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; > +identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; > expression c; > @@ > f(...) {<... > - c->maybe_tree > -+ get_commit_tree(c) > ++ repo_get_commit_tree(the_repository, c) So, why this change? It would also require furher changes to 'commit.cocci', in particular to the last semantic patch, which is supposed to ensure that get_commit_tree() doesn't end up on the LHS of an assignment, but with this change Coccinelle does suggest transfomations with repo_get_commit_tree() on the LHS. > ...>} > > @@ > -- > 2.21.0.479.g47ac719cd3 >
On Fri, Apr 5, 2019 at 12:04 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > struct object_id *get_commit_tree_oid(const struct commit *); > > > > /* > > diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci > > index c49aa558f0..f5bc639981 100644 > > --- a/contrib/coccinelle/commit.cocci > > +++ b/contrib/coccinelle/commit.cocci > > @@ -12,12 +12,12 @@ expression c; > > > > // These excluded functions must access c->maybe_tree direcly. > > @@ > > -identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; > > +identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; > > expression c; > > @@ > > f(...) {<... > > - c->maybe_tree > > -+ get_commit_tree(c) > > ++ repo_get_commit_tree(the_repository, c) > > So, why this change? Because get_commit_tree() now becomes a compat wrapper (yes I'll fill in the commit message ;) and should be avoided. > It would also require furher changes to 'commit.cocci', in particular > to the last semantic patch, which is supposed to ensure that > get_commit_tree() doesn't end up on the LHS of an assignment, but with > this change Coccinelle does suggest transfomations with > repo_get_commit_tree() on the LHS. Oooh.. I see now. I actually updated that then dropped, thinking that a function call cannot be on LHS and that conversion is for already-long-gone code anyway. But yeah when you stack that conversion on top of this, it makes sense that we need double conversion to avoid build error. Since I will have to update this patch anyway, I'll update the commit message on the first cocci patch too. > > > ...>} > > > > @@ > > -- > > 2.21.0.479.g47ac719cd3 > >
Hi Duy, On Fri, 5 Apr 2019, Duy Nguyen wrote: > On Fri, Apr 5, 2019 at 12:04 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > struct object_id *get_commit_tree_oid(const struct commit *); > > > > > > /* > > > diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci > > > index c49aa558f0..f5bc639981 100644 > > > --- a/contrib/coccinelle/commit.cocci > > > +++ b/contrib/coccinelle/commit.cocci > > > @@ -12,12 +12,12 @@ expression c; > > > > > > // These excluded functions must access c->maybe_tree direcly. > > > @@ > > > -identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; > > > +identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; > > > expression c; > > > @@ > > > f(...) {<... > > > - c->maybe_tree > > > -+ get_commit_tree(c) > > > ++ repo_get_commit_tree(the_repository, c) > > > > So, why this change? > > Because get_commit_tree() now becomes a compat wrapper (yes I'll fill > in the commit message ;) and should be avoided. > > > It would also require furher changes to 'commit.cocci', in particular > > to the last semantic patch, which is supposed to ensure that > > get_commit_tree() doesn't end up on the LHS of an assignment, but with > > this change Coccinelle does suggest transfomations with > > repo_get_commit_tree() on the LHS. > > Oooh.. I see now. I actually updated that then dropped, thinking that > a function call cannot be on LHS and that conversion is for > already-long-gone code anyway. But yeah when you stack that conversion > on top of this, it makes sense that we need double conversion to avoid > build error. > > Since I will have to update this patch anyway, I'll update the commit > message on the first cocci patch too. This actually broke the build. So here is what I have on top of `pu` to make it work again: -- snip -- Subject: [PATCH] fixup??? commit.c: add repo_get_commit_tree() The search/replace of this patch was apparently not done fully. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- contrib/coccinelle/commit.cocci | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci index f5bc639981..7f53f361c7 100644 --- a/contrib/coccinelle/commit.cocci +++ b/contrib/coccinelle/commit.cocci @@ -24,5 +24,5 @@ expression c; expression c; expression s; @@ -- get_commit_tree(c) = s +- repo_get_commit_tree(the_repository, c) = s + c->maybe_tree = s -- snap -- Ciao, Johannes > > > > > > ...>} > > > > > > @@ > > > -- > > > 2.21.0.479.g47ac719cd3 > > > > > > > -- > Duy >
diff --git a/commit.c b/commit.c index a5333c7ac6..f0a5506f04 100644 --- a/commit.c +++ b/commit.c @@ -340,7 +340,8 @@ void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit) } } -struct tree *get_commit_tree(const struct commit *commit) +struct tree *repo_get_commit_tree(struct repository *r, + const struct commit *commit) { if (commit->maybe_tree || !commit->object.parsed) return commit->maybe_tree; @@ -348,7 +349,7 @@ struct tree *get_commit_tree(const struct commit *commit) if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH) BUG("commit has NULL tree, but was not loaded from commit-graph"); - return get_commit_tree_in_graph(the_repository, commit); + return get_commit_tree_in_graph(r, commit); } struct object_id *get_commit_tree_oid(const struct commit *commit) diff --git a/commit.h b/commit.h index 42728c2906..b576201be8 100644 --- a/commit.h +++ b/commit.h @@ -143,7 +143,8 @@ void repo_unuse_commit_buffer(struct repository *r, */ void free_commit_buffer(struct parsed_object_pool *pool, struct commit *); -struct tree *get_commit_tree(const struct commit *); +struct tree *repo_get_commit_tree(struct repository *, const struct commit *); +#define get_commit_tree(c) repo_get_commit_tree(the_repository, c) struct object_id *get_commit_tree_oid(const struct commit *); /* diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci index c49aa558f0..f5bc639981 100644 --- a/contrib/coccinelle/commit.cocci +++ b/contrib/coccinelle/commit.cocci @@ -12,12 +12,12 @@ expression c; // These excluded functions must access c->maybe_tree direcly. @@ -identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; +identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; expression c; @@ f(...) {<... - c->maybe_tree -+ get_commit_tree(c) ++ repo_get_commit_tree(the_repository, c) ...>} @@
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- commit.c | 5 +++-- commit.h | 3 ++- contrib/coccinelle/commit.cocci | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-)