Hi Christian,
[focusing exclusively on the `range-diff` because I lack the capacity for
anything beyond that]
On Wed, 15 Nov 2023, Christian Couder wrote:
> # Range-diff between v6 and v7
>
> (Sorry it looks like patch 6/14 in v7 is considered to be completely
> different from what it was in v6, so the range-diff is not showing
> differences between them.)
>
> 1: fac0a9dff4 = 1: cddcd967b2 t6429: remove switching aspects of fast-rebase
> 2: bec2eb8928 ! 2: c8476fb093 replay: introduce new builtin
> @@ Documentation/git-replay.txt (new)
> +DESCRIPTION
> +-----------
> +
> -+Takes a range of commits and replays them onto a new location.
> ++Takes a range of commits, specified by <oldbase> and <branch>, and
> ++replays them onto a new location (see `--onto` option below).
> +
> +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> +
Thank you.
> 3: b0cdfdc0c3 = 3: 43322abd1e replay: start using parse_options API
> 4: c3403f0b9d = 4: 6524c7f045 replay: die() instead of failing assert()
> 5: 4188eeac30 = 5: 05d0efa3cb replay: introduce pick_regular_commit()
> 6: b7b4d9001e < -: ---------- replay: change rev walking options
> -: ---------- > 6: c7a5aad3d6 replay: change rev walking options
The actual range-diff for the sixth patch looks like this:
-- snip --
6: b7b4d9001e9 ! 6: c7a5aad3d62 replay: change rev walking options
@@ Metadata
## Commit message ##
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 for now.
+ Let's force the rev walking options we need after calling
+ setup_revisions() instead of before.
+
+ This might override some user supplied rev walking command line options
+ though. So let's detect that and warn users by:
+
+ a) setting the desired values, before setup_revisions(),
+ b) checking after setup_revisions() whether these values differ from
+ the desired values,
+ c) if so throwing a warning and setting the desired values again.
We want the command to work from older commits to newer ones by default.
Also we don't want history simplification, as we want to deal with all
the commits in the affected range.
- When we see an option that we are going to override, we emit a warning
- to avoid confusion as much as possible though.
-
Helped-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
Co-authored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
@@ Commit message
## builtin/replay.c ##
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
- struct merge_result result;
- struct strbuf reflog_msg = STRBUF_INIT;
- struct strbuf branch_name = STRBUF_INIT;
-- int ret = 0;
-+ int i, ret = 0;
-
- const char * const replay_usage[] = {
- N_("git replay --onto <newbase> <oldbase> <branch> # EXPERIMENTAL"),
-@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
repo_init_revisions(the_repository, &revs, prefix);
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
- revs.max_parents = 1;
- revs.cherry_mark = 1;
- revs.limited = 1;
-- revs.reverse = 1;
++ strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
++
++ /*
++ * Set desired values for rev walking options here. If they
++ * are changed by some user specified option in setup_revisions()
++ * below, we will detect that below and then warn.
++ *
++ * TODO: In the future we might want to either die(), or allow
++ * some options changing these values if we think they could
++ * be useful.
++ */
+ revs.reverse = 1;
- revs.right_only = 1;
-- revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
-- revs.topo_order = 1;
+ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
+ revs.topo_order = 1;
-
- strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
+- strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
++ revs.simplify_history = 0;
-+ /*
-+ * TODO: For now, let's warn when we see an option that we are
-+ * going to override after setup_revisions() below. In the
-+ * future we might want to either die() or allow them if we
-+ * think they could be useful though.
-+ */
-+ for (i = 0; i < argc; i++) {
-+ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
-+ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
-+ !strcmp(argv[i], "--full-history"))
-+ warning(_("option '%s' will be overridden"), argv[i]);
-+ }
-+
if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) {
ret = error(_("unhandled options"));
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;
++ /*
++ * Detect and warn if we override some user specified rev
++ * walking options.
++ */
++ if (revs.reverse != 1) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "reverse");
++ revs.reverse = 1;
++ }
++ if (revs.sort_order != REV_SORT_IN_GRAPH_ORDER) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "sort_order");
++ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
++ }
++ if (revs.topo_order != 1) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "topo_order");
++ revs.topo_order = 1;
++ }
++ if (revs.simplify_history != 0) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "simplify_history");
++ revs.simplify_history = 0;
++ }
+
strvec_clear(&rev_walk_args);
-- snap --
This looks really good. Thank you for going the extra step to make this
patch so much better.
> 7: c57577a9b8 = 7: 01f35f924b replay: add an important FIXME comment about gpg signing
> 8: e78be50f3d = 8: 1498b24bad replay: remove progress and info output
> 9: e4c79b676f = 9: 6786fc147b replay: remove HEAD related sanity check
> 10: 8d89f1b733 ! 10: 9a24dbb530 replay: make it a minimal server side command
> @@ Commit message
> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
>
> ## Documentation/git-replay.txt ##
> -@@ Documentation/git-replay.txt: SYNOPSIS
> - DESCRIPTION
> +@@ Documentation/git-replay.txt: DESCRIPTION
> -----------
>
> --Takes a range of commits and replays them onto a new location.
> -+Takes a range of commits and replays them onto a new location. Leaves
> -+the working tree and the index untouched, and updates no
> -+references. The output of this command is meant to be used as input to
> + Takes a range of commits, specified by <oldbase> and <branch>, and
> +-replays them onto a new location (see `--onto` option below).
> ++replays them onto a new location (see `--onto` option below). Leaves
> ++the working tree and the index untouched, and updates no references.
> ++The output of this command is meant to be used as input to
> +`git update-ref --stdin`, which would update the relevant branches.
>
> THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct merge_result result;
> - struct strbuf reflog_msg = STRBUF_INIT;
> struct strbuf branch_name = STRBUF_INIT;
> - int i, ret = 0;
> + int ret = 0;
>
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> onto = peel_committish(onto_name);
Looks good to me.
> 11: 3d433a1322 ! 11: ad6ca2fbef replay: use standard revision ranges
> @@ Documentation/git-replay.txt: git-replay - EXPERIMENTAL: Replay commits on a new
>
> DESCRIPTION
> -----------
> -@@ Documentation/git-replay.txt: DESCRIPTION
> - Takes a range of commits and replays them onto a new location. Leaves
> - the working tree and the index untouched, and updates no
> - references. The output of this command is meant to be used as input to
> +
> +-Takes a range of commits, specified by <oldbase> and <branch>, and
> +-replays them onto a new location (see `--onto` option below). Leaves
> ++Takes ranges of commits and replays them onto a new location. Leaves
> + the working tree and the index untouched, and updates no references.
> + The output of this command is meant to be used as input to
> -`git update-ref --stdin`, which would update the relevant branches.
> +`git update-ref --stdin`, which would update the relevant branches
> +(see the OUTPUT section below).
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct merge_options merge_opt;
> struct merge_result result;
> - struct strbuf branch_name = STRBUF_INIT;
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> - N_("git replay --onto <newbase> <oldbase> <branch> # EXPERIMENTAL"),
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> - strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
> -
> /*
> - * TODO: For now, let's warn when we see an option that we are
> - * going to override after setup_revisions() below. In the
> + * Set desired values for rev walking options here. If they
> + * are changed by some user specified option in setup_revisions()
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - warning(_("option '%s' will be overridden"), argv[i]);
> - }
> + revs.topo_order = 1;
> + revs.simplify_history = 0;
>
> - if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) {
> - ret = error(_("unhandled options"));
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> }
>
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - revs.topo_order = 1;
> - revs.simplify_history = 0;
> + revs.simplify_history = 0;
> + }
>
> - strvec_clear(&rev_walk_args);
> -
This is the correct spot for that documentation change. Good.
> 12: cca8105382 ! 12: 081864ed5f replay: add --advance or 'cherry-pick' mode
> @@ builtin/replay.c: static struct commit *pick_regular_commit(struct commit *pickm
> struct merge_options merge_opt;
> struct merge_result result;
> + struct strset *update_refs = NULL;
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> - N_("git replay --onto <newbase> <revision-range>... # EXPERIMENTAL"),
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
>
> /*
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - revs.topo_order = 1;
> - revs.simplify_history = 0;
> + revs.simplify_history = 0;
> + }
>
> + determine_replay_mode(&revs.cmdline, onto_name, &advance_name,
> + &onto, &update_refs);
> 13: 92287a2cc8 ! 13: 19c4016c7c replay: add --contained to rebase contained branches
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct rev_info revs;
> struct commit *last_commit = NULL;
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> - N_("git replay (--onto <newbase> | --advance <branch>) <revision-range>... # EXPERIMENTAL"),
> 14: 529a7fda40 ! 14: 29556bcc86 replay: stop assuming replayed branches do not diverge
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct merge_result result;
> struct strset *update_refs = NULL;
> + kh_oid_map_t *replayed_commits;
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
The last three only have context-only changes. Obviously good.
Apart from the one little outstanding nit where I would love to see
`(EXPERIMENTAL!)` as the first word of the synopsis both in the manual
page and in the output of `git replay -h`, you have addressed all of my
concerns.
Thank you!
Johannes
