From: Christian Couder <christian.couder@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Patrick Steinhardt <ps@pks.im>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
John Cai <johncai86@gmail.com>,
Derrick Stolee <stolee@gmail.com>,
Phillip Wood <phillip.wood123@gmail.com>,
Calvin Wan <calvinwan@google.com>, Toon Claes <toon@iotcl.com>,
Dragan Simic <dsimic@manjaro.org>,
Linus Arver <linusa@google.com>
Subject: Re: [PATCH v6 00/14] Introduce new `git replay` command
Date: Tue, 7 Nov 2023 10:43:22 +0100 [thread overview]
Message-ID: <CAP8UFD24fzhiecJtANqEsxvh1mxT4pKR=QjfUFZh8C6HQE-k1A@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BE6G2qaF50bhz-CwxSsvxGDHzwvsWtfQO4zVcX6ERppLw@mail.gmail.com>
Hi,
On Tue, Nov 7, 2023 at 3:44 AM Elijah Newren <newren@gmail.com> wrote:
> Looking good, just one comment on one small hunk...
>
> On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> [...]
>
> > @@ 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
> > ++ * 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]);
> > ++ }
> > ++
>
> Two things:
>
> 1) Not sure it makes sense to throw a warning with --topo-order or
> --full-history, since they would result in a value matching what we
> would be setting anyway.
Yeah, I am not sure about this either. About "--reverse" I think it
makes sense because even if the command is setting the reverse bit, it
would be possible to reverse the reverse like Dscho wanted. But I
agree "--topo-order" and "--full-history" will very unlikely be reused
for anything else in the future.
> 2) This seems like an inefficient way to provide this warning; could
> we avoid parsing the arguments for an extra time? Perhaps instead
> a) set the desired values here, before setup_revisions()
> b) after setup_revisions, check whether these values differ from the
> desired values, if so throw a warning.
> c) set the desired values, again
Yeah, that would work. The downside is that it would be more difficult
in the warning to tell the user which command line option was
overridden as there are some values changed by different options.
Maybe we can come up with a warning message that is still useful and
enough for now, as the command is supposed to be used by experienced
users. Perhaps something like:
warning(_("some rev walking options will be overridden as '%s' bit in
'struct rev_info' will be forced"), "sort_order");
Thanks!
next prev parent reply other threads:[~2023-11-07 9:43 UTC|newest]
Thread overview: 208+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-07 7:24 [PATCH 00/14] Introduce new `git replay` command Christian Couder
2023-04-07 7:24 ` [PATCH 01/14] replay: introduce new builtin Christian Couder
2023-04-07 7:24 ` [PATCH 02/14] replay: start using parse_options API Christian Couder
2023-04-07 7:24 ` [PATCH 03/14] replay: die() instead of failing assert() Christian Couder
2023-04-07 7:24 ` [PATCH 04/14] replay: introduce pick_regular_commit() Christian Couder
2023-04-07 7:24 ` [PATCH 05/14] replay: don't simplify history Christian Couder
2023-04-07 7:24 ` [PATCH 06/14] replay: add an important FIXME comment about gpg signing Christian Couder
2023-04-07 7:24 ` [PATCH 07/14] replay: remove progress and info output Christian Couder
2023-04-07 7:24 ` [PATCH 08/14] replay: remove HEAD related sanity check Christian Couder
2023-04-07 7:24 ` [PATCH 09/14] replay: very coarse worktree updating Christian Couder
2023-04-07 7:24 ` [PATCH 10/14] replay: make it a minimal server side command Christian Couder
2023-04-07 7:24 ` [PATCH 11/14] replay: use standard revision ranges Christian Couder
2023-04-14 14:09 ` Derrick Stolee
2023-04-14 14:23 ` Derrick Stolee
2023-04-15 19:07 ` Elijah Newren
2023-04-16 5:28 ` Elijah Newren
2023-04-17 14:05 ` Derrick Stolee
2023-04-18 5:54 ` Elijah Newren
2023-04-18 13:10 ` Derrick Stolee
2023-04-20 4:53 ` Elijah Newren
2023-04-20 13:44 ` Derrick Stolee
2023-04-23 1:18 ` Elijah Newren
2023-04-24 15:23 ` Derrick Stolee
2023-04-30 6:45 ` Elijah Newren
2023-09-03 15:47 ` Johannes Schindelin
2023-09-07 8:39 ` Christian Couder
2023-09-07 10:22 ` Johannes Schindelin
2023-04-17 15:45 ` Junio C Hamano
2023-04-18 5:58 ` Elijah Newren
2023-04-18 4:58 ` Elijah Newren
2023-04-15 18:30 ` Elijah Newren
2023-04-07 7:24 ` [PATCH 12/14] replay: introduce guess_new_base() Christian Couder
2023-04-07 7:24 ` [PATCH 13/14] replay: add different modes Christian Couder
2023-04-07 7:24 ` [PATCH 14/14] replay: stop assuming replayed branches do not diverge Christian Couder
2023-04-14 10:12 ` [PATCH 00/14] Introduce new `git replay` command Phillip Wood
2023-04-15 17:18 ` Elijah Newren
2023-04-14 17:39 ` Felipe Contreras
2023-04-15 6:44 ` Elijah Newren
2023-05-09 17:53 ` [PATCH v2 00/15] " Christian Couder
2023-05-09 17:53 ` [PATCH v2 01/15] t6429: remove switching aspects of fast-rebase Christian Couder
2023-05-09 17:53 ` [PATCH v2 02/15] replay: introduce new builtin Christian Couder
2023-05-09 17:53 ` [PATCH v2 03/15] replay: start using parse_options API Christian Couder
2023-05-09 17:53 ` [PATCH v2 04/15] replay: die() instead of failing assert() Christian Couder
2023-05-09 17:53 ` [PATCH v2 05/15] replay: introduce pick_regular_commit() Christian Couder
2023-05-09 17:53 ` [PATCH v2 06/15] replay: don't simplify history Christian Couder
2023-05-09 17:53 ` [PATCH v2 07/15] replay: add an important FIXME comment about gpg signing Christian Couder
2023-05-09 17:53 ` [PATCH v2 08/15] replay: remove progress and info output Christian Couder
2023-05-09 17:53 ` [PATCH v2 09/15] replay: remove HEAD related sanity check Christian Couder
2023-05-09 17:53 ` [PATCH v2 10/15] replay: make it a minimal server side command Christian Couder
2023-05-09 17:53 ` [PATCH v2 11/15] replay: use standard revision ranges Christian Couder
2023-05-09 17:53 ` [PATCH v2 12/15] replay: disallow revision specific options and pathspecs Christian Couder
2023-05-16 4:25 ` Elijah Newren
2023-05-09 17:53 ` [PATCH v2 13/15] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-05-09 17:53 ` [PATCH v2 14/15] replay: add --contained to rebase contained branches Christian Couder
2023-05-16 4:26 ` Elijah Newren
2023-05-09 17:53 ` [PATCH v2 15/15] replay: stop assuming replayed branches do not diverge Christian Couder
2023-05-16 4:26 ` Elijah Newren
2023-05-09 22:28 ` [PATCH v2 00/15] Introduce new `git replay` command Junio C Hamano
2023-05-10 7:33 ` Christian Couder
2023-05-16 4:42 ` Elijah Newren
2023-06-02 10:25 ` [PATCH v3 " Christian Couder
2023-06-02 10:25 ` [PATCH v3 01/15] t6429: remove switching aspects of fast-rebase Christian Couder
2023-06-02 10:25 ` [PATCH v3 02/15] replay: introduce new builtin Christian Couder
2023-06-02 10:25 ` [PATCH v3 03/15] replay: start using parse_options API Christian Couder
2023-06-02 10:25 ` [PATCH v3 04/15] replay: die() instead of failing assert() Christian Couder
2023-06-02 10:25 ` [PATCH v3 05/15] replay: introduce pick_regular_commit() Christian Couder
2023-06-02 10:25 ` [PATCH v3 06/15] replay: don't simplify history Christian Couder
2023-06-02 10:25 ` [PATCH v3 07/15] replay: add an important FIXME comment about gpg signing Christian Couder
2023-06-02 10:25 ` [PATCH v3 08/15] replay: remove progress and info output Christian Couder
2023-06-02 10:25 ` [PATCH v3 09/15] replay: remove HEAD related sanity check Christian Couder
2023-06-02 10:25 ` [PATCH v3 10/15] replay: make it a minimal server side command Christian Couder
2023-06-22 10:01 ` Toon Claes
2023-09-07 8:32 ` Christian Couder
2023-06-02 10:25 ` [PATCH v3 11/15] replay: use standard revision ranges Christian Couder
2023-06-22 10:03 ` Toon Claes
2023-09-07 8:32 ` Christian Couder
2023-09-07 21:02 ` Dragan Simic
2023-10-10 12:44 ` Christian Couder
2023-10-10 14:02 ` Dragan Simic
2023-06-02 10:25 ` [PATCH v3 12/15] replay: disallow revision specific options and pathspecs Christian Couder
2023-07-25 21:16 ` Junio C Hamano
2023-09-07 8:33 ` Christian Couder
2023-06-02 10:25 ` [PATCH v3 13/15] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-06-22 10:05 ` Toon Claes
2023-09-07 8:35 ` Christian Couder
2023-07-25 21:41 ` Junio C Hamano
2023-09-07 8:35 ` Christian Couder
2023-06-02 10:25 ` [PATCH v3 14/15] replay: add --contained to rebase contained branches Christian Couder
2023-06-22 10:10 ` Toon Claes
2023-09-07 8:37 ` Christian Couder
2023-06-02 10:25 ` [PATCH v3 15/15] replay: stop assuming replayed branches do not diverge Christian Couder
2023-06-03 1:42 ` [PATCH v3 00/15] Introduce new `git replay` command Junio C Hamano
2023-06-05 7:11 ` Christian Couder
2023-09-07 9:25 ` [PATCH v4 " Christian Couder
2023-09-07 9:25 ` [PATCH v4 01/15] t6429: remove switching aspects of fast-rebase Christian Couder
2023-09-07 9:25 ` [PATCH v4 02/15] replay: introduce new builtin Christian Couder
2023-09-07 10:23 ` Johannes Schindelin
2023-10-10 12:42 ` Christian Couder
2023-09-07 9:25 ` [PATCH v4 03/15] replay: start using parse_options API Christian Couder
2023-09-07 9:25 ` [PATCH v4 04/15] replay: die() instead of failing assert() Christian Couder
2023-09-07 9:25 ` [PATCH v4 05/15] replay: introduce pick_regular_commit() Christian Couder
2023-09-07 9:25 ` [PATCH v4 06/15] replay: don't simplify history Christian Couder
2023-09-07 10:23 ` Johannes Schindelin
2023-10-10 12:43 ` Christian Couder
2023-09-07 9:25 ` [PATCH v4 07/15] replay: add an important FIXME comment about gpg signing Christian Couder
2023-09-07 9:25 ` [PATCH v4 08/15] replay: remove progress and info output Christian Couder
2023-09-07 9:25 ` [PATCH v4 09/15] replay: remove HEAD related sanity check Christian Couder
2023-09-07 9:25 ` [PATCH v4 10/15] replay: make it a minimal server side command Christian Couder
2023-09-07 9:25 ` [PATCH v4 11/15] replay: use standard revision ranges Christian Couder
2023-09-07 10:24 ` Johannes Schindelin
2023-10-10 12:49 ` Christian Couder
2023-09-08 22:55 ` Linus Arver
2023-09-10 3:20 ` Linus Arver
2023-10-10 12:48 ` Christian Couder
2023-10-10 12:48 ` Christian Couder
2023-10-19 19:26 ` Linus Arver
2023-09-07 9:25 ` [PATCH v4 12/15] replay: disallow revision specific options and pathspecs Christian Couder
2023-09-07 10:24 ` Johannes Schindelin
2023-10-10 12:49 ` Christian Couder
2023-09-07 9:25 ` [PATCH v4 13/15] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-09-07 9:25 ` [PATCH v4 14/15] replay: add --contained to rebase contained branches Christian Couder
2023-09-07 9:25 ` [PATCH v4 15/15] replay: stop assuming replayed branches do not diverge Christian Couder
2023-09-07 10:25 ` [PATCH v4 00/15] Introduce new `git replay` command Johannes Schindelin
2023-10-10 12:50 ` Christian Couder
2023-10-10 12:38 ` [PATCH v5 00/14] " Christian Couder
2023-10-10 12:38 ` [PATCH v5 01/14] t6429: remove switching aspects of fast-rebase Christian Couder
2023-10-10 12:38 ` [PATCH v5 02/14] replay: introduce new builtin Christian Couder
2023-10-10 12:38 ` [PATCH v5 03/14] replay: start using parse_options API Christian Couder
2023-10-10 12:38 ` [PATCH v5 04/14] replay: die() instead of failing assert() Christian Couder
2023-10-10 12:38 ` [PATCH v5 05/14] replay: introduce pick_regular_commit() Christian Couder
2023-10-10 12:38 ` [PATCH v5 06/14] replay: change rev walking options Christian Couder
2023-10-10 12:38 ` [PATCH v5 07/14] replay: add an important FIXME comment about gpg signing Christian Couder
2023-10-10 12:38 ` [PATCH v5 08/14] replay: remove progress and info output Christian Couder
2023-10-10 12:38 ` [PATCH v5 09/14] replay: remove HEAD related sanity check Christian Couder
2023-10-10 12:38 ` [PATCH v5 10/14] replay: make it a minimal server side command Christian Couder
2023-10-10 12:38 ` [PATCH v5 11/14] replay: use standard revision ranges Christian Couder
2023-10-19 19:49 ` Linus Arver
2023-10-10 12:38 ` [PATCH v5 12/14] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-10-10 12:38 ` [PATCH v5 13/14] replay: add --contained to rebase contained branches Christian Couder
2023-10-10 12:38 ` [PATCH v5 14/14] replay: stop assuming replayed branches do not diverge Christian Couder
2023-10-26 13:44 ` [PATCH v5 00/14] Introduce new `git replay` command Johannes Schindelin
2023-10-29 6:01 ` Elijah Newren
2023-11-02 14:59 ` Christian Couder
2023-11-08 12:25 ` Johannes Schindelin
2023-11-02 15:06 ` Christian Couder
2023-11-08 12:25 ` Johannes Schindelin
2023-10-29 6:00 ` Elijah Newren
2023-10-29 14:14 ` Johannes Schindelin
2023-10-30 17:18 ` Elijah Newren
2023-11-02 14:44 ` Christian Couder
2023-11-02 14:48 ` Christian Couder
2023-11-02 13:51 ` [PATCH v6 " Christian Couder
2023-11-02 13:51 ` [PATCH v6 01/14] t6429: remove switching aspects of fast-rebase Christian Couder
2023-11-02 13:51 ` [PATCH v6 02/14] replay: introduce new builtin Christian Couder
2023-11-02 13:51 ` [PATCH v6 03/14] replay: start using parse_options API Christian Couder
2023-11-02 13:51 ` [PATCH v6 04/14] replay: die() instead of failing assert() Christian Couder
2023-11-02 13:51 ` [PATCH v6 05/14] replay: introduce pick_regular_commit() Christian Couder
2023-11-02 13:51 ` [PATCH v6 06/14] replay: change rev walking options Christian Couder
2023-11-02 13:51 ` [PATCH v6 07/14] replay: add an important FIXME comment about gpg signing Christian Couder
2023-11-02 13:51 ` [PATCH v6 08/14] replay: remove progress and info output Christian Couder
2023-11-02 13:51 ` [PATCH v6 09/14] replay: remove HEAD related sanity check Christian Couder
2023-11-02 13:51 ` [PATCH v6 10/14] replay: make it a minimal server side command Christian Couder
2023-11-02 13:51 ` [PATCH v6 11/14] replay: use standard revision ranges Christian Couder
2023-11-02 13:51 ` [PATCH v6 12/14] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-11-02 13:51 ` [PATCH v6 13/14] replay: add --contained to rebase contained branches Christian Couder
2023-11-02 13:51 ` [PATCH v6 14/14] replay: stop assuming replayed branches do not diverge Christian Couder
2023-11-07 2:43 ` [PATCH v6 00/14] Introduce new `git replay` command Elijah Newren
2023-11-07 9:43 ` Christian Couder [this message]
2023-11-15 14:51 ` Christian Couder
2023-11-08 12:19 ` Johannes Schindelin
2023-11-08 12:47 ` Johannes Schindelin
2023-11-15 14:46 ` Christian Couder
2023-11-16 8:45 ` Johannes Schindelin
2023-11-16 8:52 ` Christian Couder
2023-11-15 14:33 ` [PATCH v7 " Christian Couder
2023-11-15 14:33 ` [PATCH v7 01/14] t6429: remove switching aspects of fast-rebase Christian Couder
2023-11-15 14:33 ` [PATCH v7 02/14] replay: introduce new builtin Christian Couder
2023-11-15 14:33 ` [PATCH v7 03/14] replay: start using parse_options API Christian Couder
2023-11-15 14:33 ` [PATCH v7 04/14] replay: die() instead of failing assert() Christian Couder
2023-11-15 14:33 ` [PATCH v7 05/14] replay: introduce pick_regular_commit() Christian Couder
2023-11-15 14:33 ` [PATCH v7 06/14] replay: change rev walking options Christian Couder
2023-11-15 14:33 ` [PATCH v7 07/14] replay: add an important FIXME comment about gpg signing Christian Couder
2023-11-15 14:33 ` [PATCH v7 08/14] replay: remove progress and info output Christian Couder
2023-11-15 14:33 ` [PATCH v7 09/14] replay: remove HEAD related sanity check Christian Couder
2023-11-15 14:33 ` [PATCH v7 10/14] replay: make it a minimal server side command Christian Couder
2023-11-15 14:33 ` [PATCH v7 11/14] replay: use standard revision ranges Christian Couder
2023-11-15 14:33 ` [PATCH v7 12/14] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-11-15 14:33 ` [PATCH v7 13/14] replay: add --contained to rebase contained branches Christian Couder
2023-11-15 14:33 ` [PATCH v7 14/14] replay: stop assuming replayed branches do not diverge Christian Couder
2023-11-16 8:53 ` [PATCH v7 00/14] Introduce new `git replay` command Johannes Schindelin
2023-11-23 19:32 ` Elijah Newren
2023-11-24 0:28 ` Junio C Hamano
2023-11-24 11:10 ` [PATCH v8 " Christian Couder
2023-11-24 11:10 ` [PATCH v8 01/14] t6429: remove switching aspects of fast-rebase Christian Couder
2023-11-24 11:10 ` [PATCH v8 02/14] replay: introduce new builtin Christian Couder
2023-11-24 11:10 ` [PATCH v8 03/14] replay: start using parse_options API Christian Couder
2023-11-24 11:10 ` [PATCH v8 04/14] replay: die() instead of failing assert() Christian Couder
2023-11-24 11:10 ` [PATCH v8 05/14] replay: introduce pick_regular_commit() Christian Couder
2023-11-24 11:10 ` [PATCH v8 06/14] replay: change rev walking options Christian Couder
2023-11-24 11:10 ` [PATCH v8 07/14] replay: add an important FIXME comment about gpg signing Christian Couder
2023-11-24 11:10 ` [PATCH v8 08/14] replay: remove progress and info output Christian Couder
2023-11-24 11:10 ` [PATCH v8 09/14] replay: remove HEAD related sanity check Christian Couder
2023-11-24 11:10 ` [PATCH v8 10/14] replay: make it a minimal server side command Christian Couder
2023-11-24 11:10 ` [PATCH v8 11/14] replay: use standard revision ranges Christian Couder
2023-11-24 11:10 ` [PATCH v8 12/14] replay: add --advance or 'cherry-pick' mode Christian Couder
2023-11-24 11:10 ` [PATCH v8 13/14] replay: add --contained to rebase contained branches Christian Couder
2023-11-24 11:10 ` [PATCH v8 14/14] replay: stop assuming replayed branches do not diverge Christian Couder
2023-11-25 0:02 ` [PATCH v8 00/14] Introduce new `git replay` command Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP8UFD24fzhiecJtANqEsxvh1mxT4pKR=QjfUFZh8C6HQE-k1A@mail.gmail.com' \
--to=christian.couder@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=calvinwan@google.com \
--cc=dsimic@manjaro.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johncai86@gmail.com \
--cc=linusa@google.com \
--cc=newren@gmail.com \
--cc=phillip.wood123@gmail.com \
--cc=ps@pks.im \
--cc=stolee@gmail.com \
--cc=toon@iotcl.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).