On Thu, Jun 22, 2023 at 12:09 PM Toon Claes <toon@xxxxxxxxx> wrote: > > > Christian Couder <christian.couder@xxxxxxxxx> writes: > > > + /* > > + * When the user specifies e.g. > > + * git replay origin/main..mybranch > > + * git replay ^origin/next mybranch1 mybranch2 > > When I'm trying these, I'm getting the error: > error: option --onto or --advance is mandatory > > In what situation can I omit both --onto and --advance? It was possible with version 1 of this series as one of the patches allowed the command to guess the base: https://lore.kernel.org/git/20230407072415.1360068-13-christian.couder@xxxxxxxxx/ so --onto wasn't needed to specify it. Comments on that patch said that it might be better to focus on a plumbing command first and for that the patch wasn't needed, so I removed it in version 2. > > +static void determine_replay_mode(struct rev_cmdline_info *cmd_info, > > + const char *onto_name, > > + const char **advance_name, > > + struct commit **onto, > > Would it make sense to call this target? This is more the new base we are rebasing target branches onto. So if we want to change the name, `--base` or `--newbase` would make more sense. `git rebase` already has `--onto` though, so, if we want to be consistent with it, we should keep `--onto` for this. > > + struct strset **update_refs) > > +{ > > + struct ref_info rinfo; > > + > > + get_ref_information(cmd_info, &rinfo); > > + if (!rinfo.positive_refexprs) > > + die(_("need some commits to replay")); > > + if (onto_name && *advance_name) > > + die(_("--onto and --advance are incompatible")); > > Do we actually need to disallow this? I mean from git-replay's point of > view, there's no technical limitation why can cannot support both modes > at once. The update-ref commands in the output will update both target > and source branches, but it's not up to us whether that's desired. I am not sure what you call "target" and "source" branches. Anyway here is in simple terms the way the command works: 1) it takes either `--onto <newbase>` or `--advance <branch>` and then one or more <revision-range>, 2) it replays all the commits in the <revision-range> onto either <newbase> or <branch>, 3) in case of `--advance`, it outputs a single command for `git update-ref --stdin` to advance <branch> to the last commit that was just replayed, 4) in case of `--onto`, it outputs a number of commands for `git update-ref --stdin` to update the branches in <revision-range> to where the tip commits of these branches have been replayed. So `--advance` is like a cherry-pick, and `--onto` is like a rebase. It would be possible to do both a rebase onto a branch and a cherry-pick of the rebased commits onto that branch, but this is not very common and you can achieve the same result by just rebasing and then using `git reset` or `git update-ref` to make the branch point to the result of the rebase. So I don't see the point of complicating the command at this point. > > + else if (onto_name) { > > No need to 'else' here IMHO. > > > + *onto = peel_committish(onto_name); > > + if (rinfo.positive_refexprs < > > + strset_get_size(&rinfo.positive_refs)) > > + die(_("all positive revisions given must be references")); > > I tested this locally with the following command: > > $ git replay --onto main OID..OID > > This command didn't give any errors, neither did it return any > update-ref lines. I would have expected to hit this die(). Yeah, this might be unexpected. I tested it too and 'rinfo.positive_refexprs' is 1 while 'strset_get_size(&rinfo.positive_refs)' is 0 with such a command. The result does not look wrong though. Above that code there is: if (!rinfo.positive_refexprs) die(_("need some commits to replay")); so it looks like there is at least a check that the revision range passed to the command contains positive commits. It might be possible that users prefer a command that outputs nothing when there is nothing to replay instead of erroring out. > > + } else if (*advance_name) { > > + struct object_id oid; > > + char *fullname = NULL; > > + > > + *onto = peel_committish(*advance_name); > > + if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name), > > + &oid, &fullname, 0) == 1) { > > + *advance_name = fullname; > > + } else { > > + die(_("argument to --advance must be a reference")); > > + } > > + if (rinfo.positive_refexprs > 1) > > + die(_("cannot advance target with multiple source branches because ordering would be ill-defined")); > > The sources aren't always branches, so I suggest something like: > > + die(_("cannot advance target with multiple sources because ordering would be ill-defined")); Yeah, that looks reasonable. I have made this change in version 4 I will send very soon. > > + determine_replay_mode(&revs.cmdline, onto_name, &advance_name, > > + &onto, &update_refs); > > + > > + if (!onto) /* FIXME: Should handle replaying down to root commit */ > > + die("Replaying down to root commit is not supported yet!"); > > When I was testing locally I tried the following: > > $ git replay --onto main feature > > I was expecting this command to find the common ancestor automatically, > but instead I got this error. I'm fine if for now the command does not > determine the common ancestor yet, but I think we should provide a > better error for this scenario. I agree that it isn't very user friendly. We could indeed try to find if there is a common ancestor, and, if that's the case, suggest another way to call the command. This is a plumbing command in its early stage though for now. So I guess it's Ok to postpone working on nicer error messages. > > +test_expect_success 'using replay on bare repo to perform basic cherry-pick' ' > > + git -C bare replay --advance main topic1..topic2 >result-bare && > > + test_cmp expect result-bare > > +' > > + > > test_done > > Shall we add a test case when providing both --onto and --advance? And > one that omits both? Ok, I have made this change in version 4.
