Re: [PATCH 0/1] rebase: understand -C again, refactor
Hi Peff, On Wed, 14 Nov 2018, Jeff King wrote: > On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via > GitGitGadget wrote: > > > Phillip Wood reported a problem where the built-in rebase did not understand > > options like -C1, i.e. it did not expect the option argument. > > > > While investigating how to address this best, I stumbled upon > > OPT_PASSTHRU_ARGV (which I was so far happily unaware of). > > I was unaware of it, too. Thanks, that makes me feel better ;-) > Looking at the OPT_PASSTHRU and its ARGV counterpart, I think the > original intent was that you'd pass through normal last-one-wins > individual options with OPT_PASSTHRU, and then list-like options with > OPT_PASSTHRU_ARGV. But here you've used the latter to pass sets of > individual last-one-wins options. > > That said, I think what you've done here is way simpler and more > readable than using a bunch of OPT_PASSTHRUs would have been. And even > if it was not the original intent of the ARGV variant, I can't see any > reason to avoid doing it this way. Thank you, that makes me feel *even* better ;-) Together with the extra-early error checking for incorrect -C and --whitespace options, I think we're in a much better place now. Ciao, Dscho
Re: [PATCH 0/1] rebase: understand -C again, refactor
On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via GitGitGadget wrote: > Phillip Wood reported a problem where the built-in rebase did not understand > options like -C1, i.e. it did not expect the option argument. > > While investigating how to address this best, I stumbled upon > OPT_PASSTHRU_ARGV (which I was so far happily unaware of). I was unaware of it, too. Looking at the OPT_PASSTHRU and its ARGV counterpart, I think the original intent was that you'd pass through normal last-one-wins individual options with OPT_PASSTHRU, and then list-like options with OPT_PASSTHRU_ARGV. But here you've used the latter to pass sets of individual last-one-wins options. That said, I think what you've done here is way simpler and more readable than using a bunch of OPT_PASSTHRUs would have been. And even if it was not the original intent of the ARGV variant, I can't see any reason to avoid doing it this way. -Peff
[PATCH 0/1] rebase: understand -C again, refactor
Phillip Wood reported a problem where the built-in rebase did not understand options like -C1, i.e. it did not expect the option argument. While investigating how to address this best, I stumbled upon OPT_PASSTHRU_ARGV (which I was so far happily unaware of). Instead of just fixing the -C bug, I decided to simply convert all of the options intended for git am (or, eventually, for git apply). This happens to fix that bug, and does so much more: it simplifies the entire logic (and removes more lines than it adds). Johannes Schindelin (1): rebase: really just passthru the `git am` options builtin/rebase.c | 98 +--- 1 file changed, 35 insertions(+), 63 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/76 -- gitgitgadget
