[v4,06/15] replay: don't simplify history

Commit Message

Christian Couder Sept. 7, 2023, 9:25 a.m. UTC

From: Elijah Newren <[e-mail]>

Let's set the rev walking options we need after calling
setup_revisions() instead of before. This makes it clearer which options
we need.

Also we don't want history simplification, as we want to deal with all
the commits in the affected range.

Co-authored-by: Christian Couder <[e-mail]>
Signed-off-by: Elijah Newren <[e-mail]>
Signed-off-by: Christian Couder <[e-mail]>
---
 builtin/replay.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Johannes Schindelin Sept. 7, 2023, 10:23 a.m. UTC | #1

Hi Christian & Elijah,

On Thu, 7 Sep 2023, Christian Couder wrote:

> From: Elijah Newren <[e-mail]>
>
> Let's set the rev walking options we need after calling
> setup_revisions() instead of before. This makes it clearer which options
> we need.

In light of the currently open issue about command-line validation, this
change does more than this paragraph lets on: It hardcodes certain
settings, overriding (silently) any rev-list options the user might have
passed.

Is there any chance that we can avoid this change?

> Also we don't want history simplification, as we want to deal with all
> the commits in the affected range.

This, however, is a good change. It deserves to live in its own commit,
with its own commit message, in particular because it is not obvious from
the attribute names which ones we're talking about (I guess it's `limited`
and `simplify_history`, not just the latter.

Ciao,
Johannes

Christian Couder Oct. 10, 2023, 12:43 p.m. UTC | #2

Hi Dscho,

On Thu, Sep 7, 2023 at 1:02 PM Johannes Schindelin
<[e-mail]> wrote:
>
> Hi Christian & Elijah,
>
> On Thu, 7 Sep 2023, Christian Couder wrote:
>
> > From: Elijah Newren <[e-mail]>
> >
> > Let's set the rev walking options we need after calling
> > setup_revisions() instead of before. This makes it clearer which options
> > we need.
>
> In light of the currently open issue about command-line validation, this
> change does more than this paragraph lets on: It hardcodes certain
> settings, overriding (silently) any rev-list options the user might have
> passed.
>
> Is there any chance that we can avoid this change?

In the version 5 I just sent, I have changed the commit message and the code.

The commit messages says:

"
   replay: change rev walking options

   Let's set the rev walking options we need after calling
   setup_revisions() instead of before. This enforces options we always
   want.

   We want the command to work from older commits to newer ones by default,
   but we are Ok with letting users reverse that, using --reverse, if that's
   what they really want.

   Also we don't want history simplification, as we want to deal with all
   the commits in the affected range.
"

So about "revs.reverse" the code is now:

  revs.reverse = !revs.reverse;

which seems to be what you suggested elsewhere.

Apart from that I think it's fair to enforce some values for a few
options. This way we can make the command work the way we want by
default, get consistent behavior and avoid users shooting themselves
in the foot for now. If more flexibility is needed and useful in the
future, then we might allow it in future patches with proper
justification, tests and docs. There is still a lot of flexibility
left especially as the patch that disallowed some rev walking options
and pathspecs has been removed as you suggested.

> > Also we don't want history simplification, as we want to deal with all
> > the commits in the affected range.
>
> This, however, is a good change. It deserves to live in its own commit,
> with its own commit message, in particular because it is not obvious from
> the attribute names which ones we're talking about (I guess it's `limited`
> and `simplify_history`, not just the latter.

We only enforce "revs.sort_order", "revs.topo_order" and yeah
"revs.simplify_history".

Also I don't think it makes much sense to separate these changes in
different commits/patches. I am Ok to improve the commit message if
you think it's worth it, but the patch series is designed to make it
easy to review the changes from the "fast-rebase" test-tool to a good
"replay" plumbing command, and I don't think separating those related
changes would improve things much.

Patch

diff --git a/builtin/replay.c b/builtin/replay.c
index c66888679b..4b1e501595 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -173,15 +173,6 @@  int cmd_replay(int argc, const char **argv, const char *prefix)
 
 	repo_init_revisions(the_repository, &revs, prefix);
 
-	revs.verbose_header = 1;
-	revs.max_parents = 1;
-	revs.cherry_mark = 1;
-	revs.limited = 1;
-	revs.reverse = 1;
-	revs.right_only = 1;
-	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
-	revs.topo_order = 1;
-
 	strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
 
 	if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) {
@@ -189,6 +180,12 @@  int cmd_replay(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 
+	/* requirements/overrides for revs */
+	revs.reverse = 1;
+	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
+	revs.topo_order = 1;
+	revs.simplify_history = 0;
+
 	strvec_clear(&rev_walk_args);
 
 	if (prepare_revision_walk(&revs) < 0) {