Message ID | 20231102135151.843758-1-christian.couder@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Introduce new `git replay` command | expand |
Hi, 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. 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 ?
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!
Hi, On Mon, 6 Nov 2023, Elijah Newren wrote: > 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]); > > ++ } > > ++ > > [...] This seems like an inefficient way to provide this warning; Not only inefficient: think about the false positive when looking at a command-line containing `--grep --reverse`. In this instance, `--reverse` is an argument of the `--grep` option, but would be mistaken for an option in its own right. Ciao, Johannes
Hi Christian, On Thu, 2 Nov 2023, Christian Couder wrote: > # Range-diff between v5 and v6 > > (Sorry it's very long mostly due to doc and test changes over a number > of patches.) I am grateful for the detailed range-diff that let's me focus exclusively on what changed between iterations. > 1: 72c34a0eba = 1: fac0a9dff4 t6429: remove switching aspects of fast-rebase > 2: f85e6c823c ! 2: 8a605ddef8 replay: introduce new builtin > @@ Commit message > For now, this is just a rename from `t/helper/test-fast-rebase.c` into > `builtin/replay.c` with minimal changes to make it build appropriately. > > + There is a stub documentation and a stub test script though. > + > Subsequent commits will flesh out its capabilities and make it a more > standard regular builtin. > > @@ .gitignore > /git-rerere > /git-reset > > + ## Documentation/git-replay.txt (new) ## > +@@ > ++git-replay(1) > ++============= > ++ > ++NAME > ++---- > ++git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos too > ++ > ++ > ++SYNOPSIS > ++-------- > ++[verse] > ++'git replay' --onto <newbase> <revision-range>... # EXPERIMENTAL Technically, at this stage `git replay` requires precisely 5 arguments, so the `<revision>...` is incorrect and should be `<upstream> <branch>` instead. But it's not worth a new iteration to fix this. > ++ > ++DESCRIPTION > ++----------- > ++ > ++Takes a range of commits and replays them onto a new location. > ++ > ++THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE. > ++ > ++OPTIONS > ++------- > ++ > ++--onto <newbase>:: > ++ Starting point at which to create the new commits. May be any > ++ valid commit, and not just an existing branch name. > ++ Traditionally, this would be a place to describe the `<revision>` argument (or, in this patch, to reflect the current state of `builtin/replay.c`, the `<upstream> <branch>` arguments instead). > ++EXIT STATUS > ++----------- > ++ > ++For a successful, non-conflicted replay, the exit status is 0. When > ++the replay has conflicts, the exit status is 1. If the replay is not > ++able to complete (or start) due to some kind of error, the exit status > ++is something other than 0 or 1. > ++ > ++GIT > ++--- > ++Part of the linkgit:git[1] suite > + > ## Makefile ## > [... snipping non-controversial changes ...] > 6: 37d545d5d6 ! 6: edafe4846f replay: change rev walking options > [... snipping changes Elijah already talked about ...] > 7: 2943f08926 = 7: b81574744a replay: add an important FIXME comment about gpg signing > 8: f81962ba41 = 8: b08ad2b2f1 replay: remove progress and info output > 9: 236747497e ! 9: 5099c94d2e replay: remove HEAD related sanity check > {... snipping non-controversial changes ...] Thank you! Ciao, Johannes
Hi Dscho, On Wed, Nov 8, 2023 at 1:47 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Thu, 2 Nov 2023, Christian Couder wrote: > > + ## Documentation/git-replay.txt (new) ## > > +@@ > > ++git-replay(1) > > ++============= > > ++ > > ++NAME > > ++---- > > ++git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos too > > ++ > > ++ > > ++SYNOPSIS > > ++-------- > > ++[verse] > > ++'git replay' --onto <newbase> <revision-range>... # EXPERIMENTAL > > Technically, at this stage `git replay` requires precisely 5 arguments, so > the `<revision>...` is incorrect and should be `<upstream> <branch>` > instead. But it's not worth a new iteration to fix this. It was actually: 'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL (you can see it there: https://lore.kernel.org/git/20231102135151.843758-3-christian.couder@gmail.com/) But I made a mistake in the range-diff command as I ran it against a previous development version instead of the current one. > > ++ > > ++DESCRIPTION > > ++----------- > > ++ > > ++Takes a range of commits and replays them onto a new location. > > ++ > > ++THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE. > > ++ > > ++OPTIONS > > ++------- > > ++ > > ++--onto <newbase>:: > > ++ Starting point at which to create the new commits. May be any > > ++ valid commit, and not just an existing branch name. > > ++ > > Traditionally, this would be a place to describe the `<revision>` argument > (or, in this patch, to reflect the current state of `builtin/replay.c`, > the `<upstream> <branch>` arguments instead). I have fixed that in the v7 I just sent with the following: +SYNOPSIS +-------- +[verse] +'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL + +DESCRIPTION +----------- + +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. + Thanks for your review!
Hi Elijah and Dscho, On Tue, Nov 7, 2023 at 10:43 AM Christian Couder <christian.couder@gmail.com> wrote: > 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]); > > > ++ } > > > ++ > > 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"); I have implemented this in the v7 I just sent. Thanks!
Hi Christian, On Wed, 15 Nov 2023, Christian Couder wrote: > On Wed, Nov 8, 2023 at 1:47 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > On Thu, 2 Nov 2023, Christian Couder wrote: > > > > + ## Documentation/git-replay.txt (new) ## > > > +@@ > > > ++git-replay(1) > > > ++============= > > > ++ > > > ++NAME > > > ++---- > > > ++git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos too > > > ++ > > > ++ > > > ++SYNOPSIS > > > ++-------- > > > ++[verse] > > > ++'git replay' --onto <newbase> <revision-range>... # EXPERIMENTAL > > > > Technically, at this stage `git replay` requires precisely 5 arguments, so > > the `<revision>...` is incorrect and should be `<upstream> <branch>` > > instead. But it's not worth a new iteration to fix this. > > It was actually: > > 'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL Right. > > > ++ > > > ++DESCRIPTION > > > ++----------- > > > ++ > > > ++Takes a range of commits and replays them onto a new location. > > > ++ > > > ++THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE. > > > ++ > > > ++OPTIONS > > > ++------- > > > ++ > > > ++--onto <newbase>:: > > > ++ Starting point at which to create the new commits. May be any > > > ++ valid commit, and not just an existing branch name. > > > ++ > > > > Traditionally, this would be a place to describe the `<revision>` argument > > (or, in this patch, to reflect the current state of `builtin/replay.c`, > > the `<upstream> <branch>` arguments instead). > > I have fixed that in the v7 I just sent with the following: > > +SYNOPSIS > +-------- > +[verse] > +'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL I still think that the following would serve us better: [verse] (EXPERIMENTAL!) 'git replay' --onto <newbase> <oldbase> <branch> But if nobody else feels as strongly, I won't bring this up again. Ciao, Johannes
Hi Dscho, On Thu, Nov 16, 2023 at 9:45 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Wed, 15 Nov 2023, Christian Couder wrote: > > I have fixed that in the v7 I just sent with the following: > > > > +SYNOPSIS > > +-------- > > +[verse] > > +'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL > > I still think that the following would serve us better: > > [verse] > (EXPERIMENTAL!) 'git replay' --onto <newbase> <oldbase> <branch> > > But if nobody else feels as strongly, I won't bring this up again. For the tests to pass, the SYNOPSIS should be the same as the first line of the `git replay -h` output. After this series it is: usage: git replay ([--contained] --onto <newbase> | --advance <branch>) <revision-range>... # EXPERIMENTAL As the usage is supposed to describe how the command should be used, I think it makes sense for "EXPERIMENTAL" to be in a shell comment after the command. Thanks, Christian.