| Message ID | pull.171.git.1573821382.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | build-in add -i: implement all commands in the main loop | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > Based on the js/builtin-add-i branch, this patch series implements the rest > of the commands in git add -i's main loop: update, revert, add_untracked, > patch, diff, and quit. Apart from quit, these commands are all very similar > in that they first build a list of files, display it, let the user choose > which ones to act on, and then perform the action. So, this obviously depends on the previous "let's start the main loop" series, in which I recall you found at least one remaining bug after sending the latest round. I'd prefer to queue this on top of the iteration of that series, which would hopefully become the final one, which would let me avoid one rebase of this series ;-) Thanks.
Junio C Hamano <gitster@pobox.com> writes: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > >> Based on the js/builtin-add-i branch, this patch series implements the rest >> of the commands in git add -i's main loop: update, revert, add_untracked, >> patch, diff, and quit. Apart from quit, these commands are all very similar >> in that they first build a list of files, display it, let the user choose >> which ones to act on, and then perform the action. > > So, this obviously depends on the previous "let's start the main > loop" series, in which I recall you found at least one remaining bug > after sending the latest round. > > I'd prefer to queue this on top of the iteration of that series, > which would hopefully become the final one, which would let me avoid > one rebase of this series ;-) > > Thanks. I found the v7 on the list, which is what I wanted to queue this on top of. So my wait is over ;-) Thanks.
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > ... there is so little overlap between the git add --patch part > and the rest of git add --interactive that I could put the former > into a totally different file: add-patch.c. We would want to call, after "add -i in C" stabilizes, directly into "add -p" machinery from different subcommands such as "reset", "checkout", etc., and bypass the rest of "add -i" when we do so. We can credit the lack of overlap to the design that supports such usage while it is still in a scripted Porcelain. Preparing the "add -p" machinery in such a way that "add -i" merely is one of the users would make a lot of sense from organizational point of view. Thanks.
Hi Junio, On Mon, 18 Nov 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > ... there is so little overlap between the git add --patch part > > and the rest of git add --interactive that I could put the former > > into a totally different file: add-patch.c. > > We would want to call, after "add -i in C" stabilizes, directly into > "add -p" machinery from different subcommands such as "reset", > "checkout", etc., and bypass the rest of "add -i" when we do so. We > can credit the lack of overlap to the design that supports such > usage while it is still in a scripted Porcelain. > > Preparing the "add -p" machinery in such a way that "add -i" merely > is one of the users would make a lot of sense from organizational > point of view. If I understand you correctly, you mean that e.g. `git checkout -p` will call the code from `add-patch.c` directly? That is indeed the case. Already now, `git checkout -p` calls `run_add_interactive()` (see https://github.com/git/git/blob/v2.24.0/builtin/checkout.c#L463): if (opts->patch_mode) { const char *patch_mode; if (opts->checkout_index && opts->checkout_worktree) patch_mode = "--patch=checkout"; else if (opts->checkout_index && !opts->checkout_worktree) patch_mode = "--patch=reset"; else if (!opts->checkout_index && opts->checkout_worktree) patch_mode = "--patch=worktree"; else BUG("either flag must have been set, worktree=%d, index=%d", opts->checkout_worktree, opts->checkout_index); return run_add_interactive(revision, patch_mode, &opts->pathspec); } And with the changes in PR #174 (i.e. three patch series later, see https://github.com/gitgitgadget/git/pull/174/files#diff-1f1eb09e4703764f9f9b9f1f1e67eb97R205-R218 for details), this will call `run_add_p()`, i.e. the newly-added code in `add-patch.c` directly: if (!strcmp(patch_mode, "--patch")) mode = ADD_P_STAGE; else if (!strcmp(patch_mode, "--patch=stash")) mode = ADD_P_STASH; else if (!strcmp(patch_mode, "--patch=reset")) mode = ADD_P_RESET; else if (!strcmp(patch_mode, "--patch=checkout")) mode = ADD_P_CHECKOUT; else if (!strcmp(patch_mode, "--patch=worktree")) mode = ADD_P_WORKTREE; else die("'%s' not supported", patch_mode); return !!run_add_p(the_repository, mode, revision, pathspec); Now that I see it, it does not look the most elegant that the `patch_mode` is a string instead of that enum, but I think that will be relatively easy to fix once the legacy `git-add--interactive.perl` will have been decomissioned. Ciao, Dscho
Hi Junio, On Mon, 18 Nov 2019, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > > writes: > > > >> Based on the js/builtin-add-i branch, this patch series implements the rest > >> of the commands in git add -i's main loop: update, revert, add_untracked, > >> patch, diff, and quit. Apart from quit, these commands are all very similar > >> in that they first build a list of files, display it, let the user choose > >> which ones to act on, and then perform the action. > > > > So, this obviously depends on the previous "let's start the main > > loop" series, in which I recall you found at least one remaining bug > > after sending the latest round. > > > > I'd prefer to queue this on top of the iteration of that series, > > which would hopefully become the final one, which would let me avoid > > one rebase of this series ;-) > > > > Thanks. > > I found the v7 on the list, which is what I wanted to queue this on > top of. So my wait is over ;-) Yes, sorry, I did not think that I was opaque but I was. So to clarify: I will always rebase the built-in add -i/-p patch series on top of what you put on gitster/git. That is, this here patch series will always be rebased on top of js/builtin-add-i, the next patch series will always be rebased on top of js/builtin-add-i-cmds, etc Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Preparing the "add -p" machinery in such a way that "add -i" merely >> is one of the users would make a lot of sense from organizational >> point of view. > > If I understand you correctly, you mean that e.g. `git checkout -p` will > call the code from `add-patch.c` directly?... Yup, and it is a good organization that the "add -p" machinery is held not too intimate with the rest of "add -i" but treats "add -i" as just one of its users from the beginning, which is what you did here. In case it wasn't clear, I am saying that I like the resulting organization of these 8 patches.