Re: [PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
Duy Nguyen writes: > In the end, there's no variant, only one function that always takes > 'struct repository *' and I wanted to keep the shorter name 'rerere'. > But let's go with adding repo_rerere() and deprecating rerere(). If it > turns out later that repo_rerere is too long (or it's repo_xyz > everywhere) then we can do another rename. Yup, I am not opposed to another rename after the dust settles, but do nto want to see it as part of the series. Thanks.
Re: [PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
On Wed, Sep 5, 2018 at 11:04 PM Junio C Hamano wrote: > > now and here, but at some later date, we would want to > > 'git revert 24/24' after fixing all in-flights of today. > > No. We do not want to revert the whole thing. > > If the function that takes a_repository is called repo_rerere(), as > opposed to just rerere(), it should keep that name after we > deprecate the function rerere(). > > We will want to get rid of #define that gives a thin wrapper and > make everybody use the API that requires a_repository parameter. > > And from that point of view, it is backwards not to introduce > repo_rerere() when rerere.c gains a variant that can work in an > arbitrary repository, not limited to the_repository, and fix it up > saying "oops, we were wrong and this will break topics in flight" at > the very end. In the end, there's no variant, only one function that always takes 'struct repository *' and I wanted to keep the shorter name 'rerere'. But let's go with adding repo_rerere() and deprecating rerere(). If it turns out later that repo_rerere is too long (or it's repo_xyz everywhere) then we can do another rename. -- Duy
Re: [PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
Stefan Beller writes: > Presumably you merge the tip of this series (which contains 24/24) > with the other in-flight topics, that make new uses of > init_revisions(revs, prefix), which 24/24 allows. Going on either > parent side of such a merge will have working commits, that compile. > > So from that POV it doesn't matter when the #define is introduced. But that is not a very interesting point of view anyway. > now and here, but at some later date, we would want to > 'git revert 24/24' after fixing all in-flights of today. No. We do not want to revert the whole thing. If the function that takes a_repository is called repo_rerere(), as opposed to just rerere(), it should keep that name after we deprecate the function rerere(). We will want to get rid of #define that gives a thin wrapper and make everybody use the API that requires a_repository parameter. And from that point of view, it is backwards not to introduce repo_rerere() when rerere.c gains a variant that can work in an arbitrary repository, not limited to the_repository, and fix it up saying "oops, we were wrong and this will break topics in flight" at the very end.
Re: [PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
On Tue, Sep 4, 2018 at 1:57 PM Junio C Hamano wrote: > > Nguyễn Thái Ngọc Duy writes: > > > The three functions init_revisions(), diff_setup() and rerere() are > > prefixed temporarily with repo_ to avoid breaking other topics which > > add new call sites for these functions. This is a temporary > > measure. Once everything is merged, it will be reverted and the new > > call sites fixed. > > That's a sensible thing to do, but isn't it too late at 24/24 stage? > IOW, doesn't in-flight topics break if up to 23/24 of this series is > merged? Presumably you merge the tip of this series (which contains 24/24) with the other in-flight topics, that make new uses of init_revisions(revs, prefix), which 24/24 allows. Going on either parent side of such a merge will have working commits, that compile. So from that POV it doesn't matter when the #define is introduced. > IOW, the one that teaches "work in this repository" to rerere(int) > for example should have introduced > > repo_rerere(struct repository *, int); > #define rerere(x) repo_rerere(the_repository, x) > > in its own step, not this late in the series, no? That might make for a better presentation to reviewers now and here, but at some later date, we would want to 'git revert 24/24' after fixing all in-flights of today. And with that in mind it makes sense to separate out all these changes into this patch, as that allows us to have the revert rename back to easy names. You may be asking this question as it was done differently in the series sb/object-store-lookup. But there the #define's served a different purpose. That series used the #define to aid author to know where dependencies to the_repository are and which function is done already. This series solely uses the #define after the main series is done to aid the maintainer to merge it which as you noted was one of the problems with that other approach. The downside of the series as it is presented here is that it is very easy to miss some hidden dependency, that leads to a bug only discovered later (e.g. some function still uses the_repository, when working on a submodule for example, but as that function is used in some corner case our test suite would not find it). So why don't we try this approach as well and see how well it goes over time? Thanks, Stefan
Re: [PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
Nguyễn Thái Ngọc Duy writes: > The three functions init_revisions(), diff_setup() and rerere() are > prefixed temporarily with repo_ to avoid breaking other topics which > add new call sites for these functions. This is a temporary > measure. Once everything is merged, it will be reverted and the new > call sites fixed. That's a sensible thing to do, but isn't it too late at 24/24 stage? IOW, doesn't in-flight topics break if up to 23/24 of this series is merged? IOW, the one that teaches "work in this repository" to rerere(int) for example should have introduced repo_rerere(struct repository *, int); #define rerere(x) repo_rerere(the_repository, x) in its own step, not this late in the series, no? > diff --git a/apply.c b/apply.c > index fc52993548..fdae1d423b 100644 > --- a/apply.c > +++ b/apply.c > @@ -4632,7 +4632,7 @@ static int write_out_results(struct apply_state *state, > struct patch *list) > } > string_list_clear(, 0); > > - rerere(state->repo, 0); > + repo_rerere(state->repo, 0); > } > > return errs;
Re: [PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
On Mon, Sep 3, 2018 at 11:10 AM Nguyễn Thái Ngọc Duy wrote: > > The three functions init_revisions(), diff_setup() and rerere() are > prefixed temporarily with repo_ to avoid breaking other topics which > add new call sites for these functions. This is a temporary > measure. Once everything is merged, it will be reverted and the new > call sites fixed. Thanks for writing the whole series; it was a pleasant read. Stefan
[PATCH v2 24/24] Rename functions to avoid breaking in-flight topics
The three functions init_revisions(), diff_setup() and rerere() are prefixed temporarily with repo_ to avoid breaking other topics which add new call sites for these functions. This is a temporary measure. Once everything is merged, it will be reverted and the new call sites fixed. Signed-off-by: Nguyễn Thái Ngọc Duy --- apply.c | 2 +- bisect.c | 4 ++-- blame.c | 6 +++--- builtin/add.c| 4 ++-- builtin/am.c | 10 +- builtin/blame.c | 2 +- builtin/checkout.c | 4 ++-- builtin/commit.c | 4 ++-- builtin/describe.c | 4 ++-- builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c | 2 +- builtin/diff.c | 2 +- builtin/fast-export.c| 2 +- builtin/fmt-merge-msg.c | 2 +- builtin/log.c| 18 +- builtin/merge.c | 8 builtin/pack-objects.c | 2 +- builtin/prune.c | 2 +- builtin/range-diff.c | 2 +- builtin/reflog.c | 2 +- builtin/rerere.c | 2 +- builtin/rev-list.c | 2 +- builtin/revert.c | 2 +- builtin/shortlog.c | 2 +- builtin/submodule--helper.c | 2 +- bundle.c | 4 ++-- diff-lib.c | 4 ++-- diff-no-index.c | 2 +- diff.c | 2 +- diff.h | 3 ++- http-push.c | 2 +- merge-recursive.c| 4 ++-- notes-merge.c| 4 ++-- pack-bitmap-write.c | 2 +- patch-ids.c | 2 +- read-cache.c | 2 +- ref-filter.c | 2 +- remote.c | 2 +- rerere.c | 2 +- rerere.h | 3 ++- revision.c | 4 ++-- revision.h | 3 ++- sequencer.c | 12 ++-- shallow.c| 2 +- submodule.c | 6 +++--- t/helper/test-revision-walking.c | 2 +- tree-diff.c | 2 +- wt-status.c | 10 +- 49 files changed, 90 insertions(+), 87 deletions(-) diff --git a/apply.c b/apply.c index fc52993548..fdae1d423b 100644 --- a/apply.c +++ b/apply.c @@ -4632,7 +4632,7 @@ static int write_out_results(struct apply_state *state, struct patch *list) } string_list_clear(, 0); - rerere(state->repo, 0); + repo_rerere(state->repo, 0); } return errs; diff --git a/bisect.c b/bisect.c index e19c60829c..560493acd2 100644 --- a/bisect.c +++ b/bisect.c @@ -632,7 +632,7 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix, struct argv_array rev_argv = ARGV_ARRAY_INIT; int i; - init_revisions(revs, the_repository, prefix); + repo_init_revisions(revs, the_repository, prefix); revs->abbrev = 0; revs->commit_format = CMIT_FMT_UNSPECIFIED; @@ -889,7 +889,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) struct rev_info opt; /* diff-tree init */ - init_revisions(, the_repository, prefix); + repo_init_revisions(, the_repository, prefix); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ opt.abbrev = 0; opt.diff = 1; diff --git a/blame.c b/blame.c index 8dbd436e95..39fbd796c0 100644 --- a/blame.c +++ b/blame.c @@ -563,7 +563,7 @@ static struct blame_origin *find_origin(struct repository *r, * and origin first. Most of the time they are the * same and diff-tree is fairly efficient about this. */ - diff_setup(_opts, r); + repo_diff_setup(_opts, r); diff_opts.flags.recursive = 1; diff_opts.detect_rename = 0; diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; @@ -638,7 +638,7 @@ static struct blame_origin *find_rename(struct repository *r, struct diff_options diff_opts; int i; - diff_setup(_opts, r); + repo_diff_setup(_opts, r); diff_opts.flags.recursive = 1; diff_opts.detect_rename = DIFF_DETECT_RENAME; diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; @@ -1262,7 +1262,7 @@ static void find_copy_in_parent(struct blame_scoreboard *sb, if (!unblamed) return; /* nothing remains for this target */ - diff_setup(_opts, sb->repo); + repo_diff_setup(_opts, sb->repo); diff_opts.flags.recursive = 1; diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; diff --git a/builtin/add.c b/builtin/add.c index
