stash/reset/checkout -p: optionally use the add --patch backend written in pure C
cover stash/reset/checkout -p: optionally use the add --patch backend written in pure C
At this stage on the journey to a fully built-in git add, we already have everything we need, including the --interactive and --patch options, as long as the add.interactive.useBuiltin setting is set to true. This config setting is kind of a feature flag that is currently turned off by default, and will be for a while, until we get confident enough that the built-in version does the job, and retire the Perl script.
However, the internal add--interactive helper is also used to back the --patch option of git stash, git reset, git checkout and git worktree.
This patch series (based on js/add-p-in-c) brings them "online".
Johannes Schindelin (7):
built-in add -p: prepare for patch modes other than "stage" built-in add -p: implement the "stash" and "reset" patch modes legacy stash -p: respect the add.interactive.usebuiltin setting built-in stash: use the built-in git add -p if so configured built-in add -p: implement the "checkout" patch modes built-in add -p: implement the "worktree" patch modes commit --interactive: make it work with the built-in add -i
add-interactive.c | 2 +- add-interactive.h | 12 +- add-patch.c | 353 ++++++++++++++++++++++++++++++++++++++++---- builtin/add.c | 35 ++++- builtin/commit.c | 8 +- builtin/stash.c | 25 ++-- git-legacy-stash.sh | 2 +- 7 files changed, 388 insertions(+), 49 deletions(-)
base-commit: 2e4083198d1508206488af4c82093ceb6cf20f4e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-174%2Fdscho%2Fother-command-p-in-c-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-174/dscho/other-command-p-in-c-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/174
Signature · 1 line
gitgitgadget
PATCH 1/7 built-in add -p: prepare for patch modes other than "stage"
From: Johannes Schindelin <[email protected]>
The Perl script backing git add -p is used not only for that command, but also for git stash -p, git reset -p and git checkout -p.
In preparation for teaching the C version of git add -p to support also the latter commands, let's abstract away what is "stage" specific into a dedicated data structure describing the differences between the patch modes.
Finally, please note that the Perl version tries to make sure that the diffs are only generated for the modified files. This is not actually necessary, as the calls to Git's diff machinery already perform that work, and perform it well. This makes it unnecessary to port the FILTER field of the %patch_modes struct, as well as the get_diff_reference() function.
--- add-interactive.c | 2 +- add-interactive.h | 8 ++++- add-patch.c | 88 +++++++++++++++++++++++++++++++++-------------- builtin/add.c | 10 ++++-- 4 files changed, 79 insertions(+), 29 deletions(-)
add-interactive.c +1 -1
@@ -924,7 +924,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,parse_pathspec (&ps_selected,PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL ,PATHSPEC_LITERAL_PATH ,"" , args.argv); - res =run_add_p (s->r, &ps_selected); + res =run_add_p (s->r,ADD_P_STAGE ,NULL , &ps_selected);argv_array_clear (&args);clear_pathspec (&ps_selected); }
add-interactive.h +7 -1
@@ -22,6 +22,12 @@ void init_add_i_state(struct add_i_state *s, struct repository *r);struct repository ;struct pathspec ;int run_add_i (struct repository *r,const struct pathspec *ps); -int run_add_p (struct repository *r,const struct pathspec *ps); + +enum add_p_mode { +ADD_P_STAGE , +}; + +int run_add_p (struct repository *r,enum add_p_mode mode, +const char *revision,const struct pathspec *ps);#endif
add-patch.c +63 -25
163 lines not on this page · raw message
builtin/add.c +8 -2
@@ -194,12 +194,18 @@ int run_add_interactive(const char *revision, const char *patch_mode, &use_builtin_add_i);if (use_builtin_add_i ==1 ) { +enum add_p_mode mode; +if (!patch_mode)return !!run_add_i (the_repository, pathspec); -if (strcmp (patch_mode,"--patch" )) + +if (!strcmp (patch_mode,"--patch" )) + mode =ADD_P_STAGE ; +else die ("'%s' not yet supported in the built-in add -p" , patch_mode); -return !!run_add_p (the_repository, pathspec); + +return !!run_add_p (the_repository, mode, revision, pathspec); }argv_array_push (&argv,"add--interactive" );
Signature · 1 line
gitgitgadget
Re: [PATCH 1/7] built-in add -p: prepare for patch modes other than "stage"
"Johannes Schindelin via GitGitGadget" <[email protected]>
add-interactive.h void init_add_i_state(struct add_i_state *s, struct repository *r); · line 28
Quoted 4 lines · writes:
add-interactive.h +4
-0+ +enum add_p_mode { +ADD_P_STAGE , +};
Nice to see a trailing comma here ;-)
add-interactive.h void init_add_i_state(struct add_i_state *s, struct repository *r); · line 31
Quoted 2 lines
add-interactive.h +2
-0+int run_add_p (struct repository *r,enum add_p_mode mode, +const char *revision,const struct pathspec *ps);
This makes readers wonder if "const struct object_id *" is more appropriate; "const char *revision" that holds human-readable name is better when the internal machinery uses it for reporting, so that may be what is going on here (so is the new field in add_p_state structure).
Quoted 6 lines
#endif
add-patch.c +0
-0
add-patch.c enum prompt_mode_type { · line 15
Quoted 9 lines
add-patch.c +2
-4@@ -11,10 +11,33 @@ enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK }; -static const char *prompt_mode[] = { -N_ ("Stage mode change [y,n,a,q,d%s,?]? " ), -N_ ("Stage deletion [y,n,a,q,d%s,?]? " ), - N_("Stage this hunk [y,n,a,q,d%s,?]? ") +struct patch_mode { +const char *diff[4 ], *apply[4 ], *apply_check[4 ];
Hardcoded "4" and not-quite descriptive names puzzle readers at the first glance. Let's read on to see if they need any further improvement.
add-patch.c enum prompt_mode_type { · line 17
Quoted 2 lines
add-patch.c +2
-0+ unsigned is_reverse:1, apply_for_checkout:1; +const char *prompt_mode[PROMPT_HUNK +1 ];
This relies on the enum value assignment (or listing) order to ensure that PROMPT_HUNK always comes at the end. Perhaps that deserves a comment before "enum prompt_mode_type", e.g.
+/* Keep PROMPT_HUNK at the end */
enum prompt_mode_type {
PROMPT_MODE_CHANGE = 0, ...
};add-patch.c enum prompt_mode_type { · line 22
Quoted 5 lines
add-patch.c +5
-0+const char *edit_hunk_hint, *help_patch_text; +}; + +static struct patch_mode patch_mode_stage = { + .diff = {"diff-files" ,NULL },
Nice to see designated initializers used ;-)
Mental note: the "diff" field is (probably) for "the command line to be used to generate the patch"
add-patch.c enum prompt_mode_type { · line 24
Quoted 2 lines
add-patch.c +2
-0+ .apply = { "--cached", NULL }, + .apply_check = { "--cached", NULL },
Mental note: these "apply" and "apply_check" fields are (probably) not for the command line; unlike the "diff" field, these only have arguments.
Mental note: if the three field names become confusing, perhaps we can clarify them by either (1) calling diff as diff_cmd[], or (2) calling the other as apply_args[] and apply_check_args[], or (3) rename both.
add-patch.c enum prompt_mode_type { · line 25
Quoted 1 line
add-patch.c +1
-0+ .is_reverse = 0,
Wouldn't it be sufficient to apply the default initialization, just like it is done for apply_for_checkout bitfield?
add-patch.c int run_add_p(struct repository *r, const struct pathspec *ps) · line 1349
Quoted 6 lines
add-patch.c +2
-0@@ -1310,6 +1345,9 @@ int run_add_p(struct repository *r, const struct pathspec *ps)init_add_i_state (&s.s, r); + s.mode = &patch_mode_stage; + s.revision = revision;
The phrase "mode_stage" may become problematic, as other modes that will be introduced, like "reset", "checkout" all will stage different contents to the index. The only mode the machinery knows at this point in the series is how "add" stages contents to the index, so "patch_mode_add" might turn out to be a better choice of the phrase as we read the series along. We'll see.
add-patch.c int run_add_p(struct repository *r, const struct pathspec *ps) · line 1351
Quoted 2 lines
add-patch.c +2
-0+if (!strcmp (patch_mode,"--patch" )) + mode =ADD_P_STAGE ;
The same comment applies to this enum token.
Thanks.
Re: [PATCH 1/7] built-in add -p: prepare for patch modes other than "stage"
Hi Junio,
Quoted 11 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
add-interactive.h void init_add_i_state(struct add_i_state *s, struct repository *r); · line 31
Quoted 2 lines · writes:
add-interactive.h +2
-0+int run_add_p (struct repository *r,enum add_p_mode mode, +const char *revision,const struct pathspec *ps);This makes readers wonder if "const struct
object_id*" is more appropriate; "const char *revision" that holds human-readable name is better when the internal machinery uses it for reporting, so that may be what is going on here (so is the new field inadd_p_statestructure).
The main reason why this is a string instead of an object ID is to be able to discern between the HEAD vs non-HEAD versions.
A secondary consideration was that we will want to keep the scripted version of add -p as the default for now, and that needs the value as the original string, not as the parsed object ID.
Quoted 9 lines
Quoted 9 lines
#endif
add-patch.c +0
-0@@ -11,10 +11,33 @@ enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK };
Quoted 10 lines
add-patch.c enum prompt_mode_type { · line 15
Quoted 6 lines
add-patch.c +2
-4-static const char *prompt_mode[] = { -N_ ("Stage mode change [y,n,a,q,d%s,?]? " ), -N_ ("Stage deletion [y,n,a,q,d%s,?]? " ), -N_ ("Stage this hunk [y,n,a,q,d%s,?]? " ) +struct patch_mode { +const char *diff[4 ], *apply[4 ], *apply_check[4 ];Hardcoded "4" and not-quite descriptive names puzzle readers at the first glance. Let's read on to see if they need any further improvement.
Good point. I added a code comment.
Quoted 11 lines
add-patch.c enum prompt_mode_type { · line 17
Quoted 2 lines
add-patch.c +2
-0+ unsigned is_reverse:1, apply_for_checkout:1; +const char *prompt_mode[PROMPT_HUNK +1 ];This relies on the enum value assignment (or listing) order to ensure that
PROMPT_HUNKalways comes at the end. Perhaps that deserves a comment before "enumprompt_mode_type", e.g.+/* Keep PROMPT_HUNK at the end */ enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, ... };
I agree, and introduced PROMPT_MODE_MAX for that purpose.
Quoted 19 lines
add-patch.c enum prompt_mode_type { · line 22
Quoted 6 lines
add-patch.c +5
-0+const char *edit_hunk_hint, *help_patch_text; +}; + +static struct patch_mode patch_mode_stage = { + .diff = {"diff-files" ,NULL },Nice to see designated initializers used ;-)
Mental note: the "diff" field is (probably) for "the command line to be used to generate the patch"
add-patch.c enum prompt_mode_type { · line 24
Quoted 2 lines
add-patch.c +2
-0+ .apply = { "--cached", NULL }, + .apply_check = { "--cached", NULL },Mental note: these "apply" and "
apply_check" fields are (probably) not for the command line; unlike the "diff" field, these only have arguments.
Quoted 4 lines
Mental note: if the three field names become confusing, perhaps we can clarify them by either (1) calling diff as diff_cmd[], or (2) calling the other as apply_args[] and apply_check_args[], or (3) rename both.
I renamed all three.
Quoted 4 lines
add-patch.c enum prompt_mode_type { · line 25
Quoted 1 line
add-patch.c +1
-0+ .is_reverse = 0,Wouldn't it be sufficient to apply the default initialization, just like it is done for
apply_for_checkoutbitfield?
Yep, I dropped those unnecessary initializations.
Quoted 14 lines
add-patch.c int run_add_p(struct repository *r, const struct pathspec *ps) · line 1349
Quoted 6 lines
add-patch.c +2
-0@@ -1310,6 +1345,9 @@ int run_add_p(struct repository *r, const struct pathspec *ps)init_add_i_state (&s.s, r); + s.mode = &patch_mode_stage; + s.revision = revision;The phrase "
mode_stage" may become problematic, as other modes that will be introduced, like "reset", "checkout" all will stage different contents to the index. The only mode the machinery knows at this point in the series is how "add" stages contents to the index, so "patch_mode_add" might turn out to be a better choice of the phrase as we read the series along. We'll see.
Quoted 4 lines
add-patch.c int run_add_p(struct repository *r, const struct pathspec *ps) · line 1351
Quoted 2 lines
add-patch.c +2
-0+if (!strcmp (patch_mode,"--patch" )) + mode =ADD_P_STAGE ;The same comment applies to this enum token.
I renamed both to the *_add and *_ADD version, respectively.
Thanks,
Dscho
PATCH 2/7 built-in add -p: implement the "stash" and "reset" patch modes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The git stash and git reset commands support a --patch option, and both simply hand off to git add -p to perform that work. Let's teach the built-in version of that command to be able to perform that work, too.
--- add-interactive.h | 2 ++ add-patch.c | 85 ++++++++++++++++++++++++++++++++++++++++++++--- builtin/add.c | 4 +++ 3 files changed, 87 insertions(+), 4 deletions(-)
add-interactive.h +2 -0
@@ -25,6 +25,8 @@ int run_add_i(struct repository *r, const struct pathspec *ps);enum add_p_mode {ADD_P_STAGE , +ADD_P_STASH , +ADD_P_RESET , }; int run_add_p(struct repository *r, enum add_p_mode mode,
add-patch.c +81 -4
106 lines not on this page · raw message
builtin/add.c +4 -0
@@ -201,6 +201,10 @@ int run_add_interactive(const char *revision, const char *patch_mode,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 die ("'%s' not yet supported in the built-in add -p" , patch_mode);
Signature · 1 line
gitgitgadget
Re: [PATCH 2/7] built-in add -p: implement the "stash" and "reset" patch modes
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 18 lines · writes:
From: Johannes Schindelin <[email protected]>
The
git stashandgit resetcommands support a--patchoption, and both simply hand off togit add -pto perform that work. Let's teach the built-in version of that command to be able to perform that work, too.Signed-off-by Johannes Schindelin <[email protected]>--- add-interactive.h | 2 ++ add-patch.c | 85 ++++++++++++++++++++++++++++++++++++++++++++--- builtin/add.c | 4 +++ 3 files changed, 87 insertions(+), 4 deletions(-)add-interactive.h +0
-0
add-interactive.h int run_add_i(struct repository *r, const struct pathspec *ps); · line 29
Quoted 5 lines
add-interactive.h +2
-0@@ -25,6 +25,8 @@ int run_add_i(struct repository *r, const struct pathspec *ps);enum add_p_mode {ADD_P_STAGE , +ADD_P_STASH , +ADD_P_RESET ,
As I mentioned in my review on the previous step, ADD_P_ADD would be more descriptive of what is going on when listed together with STASH and RESET here.
Quoted 35 lines
add-interactive.h +35
-0+static struct patch_mode patch_mode_reset_head = { + .diff = {"diff-index" ,"--cached" ,NULL }, + .apply = {"-R" ,"--cached" ,NULL }, + .apply_check = {"-R" ,"--cached" ,NULL }, + .is_reverse =1 , + .index_only =1 , + .prompt_mode = { +N_ ("Unstage mode change [y,n,q,a,d%s,?]? " ), +N_ ("Unstage deletion [y,n,q,a,d%s,?]? " ), +N_ ("Unstage this hunk [y,n,q,a,d%s,?]? " ), + }, + .edit_hunk_hint =N_ ("If the patch applies cleanly, the edited hunk " +"will immediately be marked for unstaging." ), + .help_patch_text = +N_ ("y - unstage this hunk\n" +"n - do not unstage this hunk\n" +"q - quit; do not unstage this hunk or any of the remaining " +"ones\n" +"a - unstage this hunk and all later hunks in the file\n" +"d - do not unstage this hunk or any of the later hunks in " +"the file\n" ), +}; + +static struct patch_mode patch_mode_reset_nothead = { + .diff = {"diff-index" ,"-R" ,"--cached" ,NULL }, + .apply = {"--cached" ,NULL }, + .apply_check = {"--cached" ,NULL }, + .is_reverse =0 , + .index_only =1 , + .prompt_mode = { +N_ ("Apply mode change to index [y,n,q,a,d%s,?]? " ), +N_ ("Apply deletion to index [y,n,q,a,d%s,?]? " ), +N_ ("Apply this hunk to index [y,n,q,a,d%s,?]? " ), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
add-interactive.h int run_add_i(struct repository *r, const struct pathspec *ps);
Quoted 10 lines
add-interactive.h +10
-0+ "will immediately be marked for applying."), + .help_patch_text = +N_ ("y - apply this hunk to index\n" +"n - do not apply this hunk to index\n" +"q - quit; do not apply this hunk or any of the remaining " +"ones\n" +"a - apply this hunk and all later hunks in the file\n" +"d - do not apply this hunk or any of the later hunks in " + "the file\n"), +};
Interesting that "reset to HEAD" and "reset to non-HEAD" would have to swap the direction to make it feel more natural to the users. This is nothing new---just re-discovering that it is/was interesting.
Re: [PATCH 2/7] built-in add -p: implement the "stash" and "reset" patch modes
Hi Junio,
Quoted 9 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
add-interactive.h +0
-0@@ -25,6 +25,8 @@ int run_add_i(struct repository *r, const struct pathspec *ps);
Quoted 8 lines
add-interactive.h int run_add_i(struct repository *r, const struct pathspec *ps); · line 29
Quoted 4 lines
add-interactive.h +2
-0enum add_p_mode {ADD_P_STAGE , +ADD_P_STASH , +ADD_P_RESET ,As I mentioned in my review on the previous step,
ADD_P_ADDwould be more descriptive of what is going on when listed together with STASH and RESET here.
I made the suggested change.
Quoted 41 lines
Quoted 41 lines
add-interactive.h +41
-0+static struct patch_mode patch_mode_reset_head = { + .diff = {"diff-index" ,"--cached" ,NULL }, + .apply = {"-R" ,"--cached" ,NULL }, + .apply_check = {"-R" ,"--cached" ,NULL }, + .is_reverse =1 , + .index_only =1 , + .prompt_mode = { +N_ ("Unstage mode change [y,n,q,a,d%s,?]? " ), +N_ ("Unstage deletion [y,n,q,a,d%s,?]? " ), +N_ ("Unstage this hunk [y,n,q,a,d%s,?]? " ), + }, + .edit_hunk_hint =N_ ("If the patch applies cleanly, the edited hunk " +"will immediately be marked for unstaging." ), + .help_patch_text = +N_ ("y - unstage this hunk\n" +"n - do not unstage this hunk\n" +"q - quit; do not unstage this hunk or any of the remaining " +"ones\n" +"a - unstage this hunk and all later hunks in the file\n" +"d - do not unstage this hunk or any of the later hunks in " +"the file\n" ), +}; + +static struct patch_mode patch_mode_reset_nothead = { + .diff = {"diff-index" ,"-R" ,"--cached" ,NULL }, + .apply = {"--cached" ,NULL }, + .apply_check = {"--cached" ,NULL }, + .is_reverse =0 , + .index_only =1 , + .prompt_mode = { +N_ ("Apply mode change to index [y,n,q,a,d%s,?]? " ), +N_ ("Apply deletion to index [y,n,q,a,d%s,?]? " ), +N_ ("Apply this hunk to index [y,n,q,a,d%s,?]? " ), + }, + .edit_hunk_hint =N_ ("If the patch applies cleanly, the edited hunk " +"will immediately be marked for applying." ), + .help_patch_text = + N_("y - apply this hunk to index\n" + "n - do not apply this hunk to index\n" + "q - quit; do not apply this hunk or any of the remaining " + "ones\n"
Quoted 8 lines
add-interactive.h
Quoted 4 lines
add-interactive.h +4
-0+"a - apply this hunk and all later hunks in the file\n" +"d - do not apply this hunk or any of the later hunks in " + "the file\n"), +};Interesting that "reset to HEAD" and "reset to non-HEAD" would have to swap the direction to make it feel more natural to the users. This is nothing new---just re-discovering that it is/was interesting.
Indeed. It matches the intuition, but looks incongruent.
Ciao,
Dscho
PATCH 3/7 legacy stash -p: respect the add.interactive.usebuiltin setting
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
As git add traditionally did not expose the --patch=<mode> modes via command-line options, the scripted version of git stash had to call git add--interactive directly.
But this prevents the built-in add -p from kicking in, as add--interactive is the scripted version (which does not have a "fall-back" to the built-in version).
So let's introduce support for internal switch for git add that the scripted git stash can use to call the appropriate backend (scripted or built-in, depending on add.interactive.useBuiltin).
--- builtin/add.c | 14 ++++++++++++++ git-legacy-stash.sh | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-)
builtin/add.c +14 -0
@@ -29,6 +29,7 @@ static const char * const builtin_add_usage[] = {static int patch_interactive, add_interactive, edit_interactive;static int take_worktree_changes;static int add_renormalize; +static int legacy_stash_p;/* support for the scripted `git stash` */ struct update_callback_data {int flags;
@@ -335,6 +336,8 @@ static struct option builtin_add_options[] = { N_("override the executable bit of the listed files")),OPT_HIDDEN_BOOL (0 ,"warn-embedded-repo" , &warn_on_embedded_repo,N_ ("warn when adding an embedded repository" )), +OPT_HIDDEN_BOOL (0 ,"legacy-stash-p" , &legacy_stash_p, +N_ ("backend for `git stash -p`" )), OPT_END(), };
@@ -431,6 +434,17 @@ int cmd_add(int argc, const char **argv, const char *prefix) add_interactive =1 ;if (add_interactive)exit (interactive_add (argc -1 , argv +1 , prefix, patch_interactive)); +if (legacy_stash_p) { +struct pathspec pathspec; + +parse_pathspec (&pathspec,0 , +PATHSPEC_PREFER_FULL | +PATHSPEC_SYMLINK_LEADING_PATH | +PATHSPEC_PREFIX_ORIGIN , + prefix, argv); + +return run_add_interactive (NULL ,"--patch=stash" , &pathspec); + }if (edit_interactive)return (edit_patch (argc, argv, prefix));
git-legacy-stash.sh +1 -1
@@ -206,7 +206,7 @@ create_stash () { # find out what the user wants GIT_INDEX_FILE="$TMP-index" \ - git add--interactive --patch=stash -- "$@" && + git add --legacy-stash-p -- "$@" && # state of the working tree w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
Signature · 1 line
gitgitgadget
PATCH 4/7 built-in stash: use the built-in `git add -p` if so configured
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The scripted version of git stash called directly into the Perl script git-add--interactive.perl, and this was faithfully converted to C.
However, we have a much better way to do this now: call the internal API directly, which will now incidentally also respect the add.interactive.useBuiltin setting. Let's just do this.
--- builtin/stash.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
builtin/stash.c +14 -11
@@ -999,9 +999,9 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, {int ret =0 ;struct child_process cp_read_tree =CHILD_PROCESS_INIT ; -struct child_process cp_add_i =CHILD_PROCESS_INIT ;struct child_process cp_diff_tree =CHILD_PROCESS_INIT ;struct index_state istate = {NULL }; +char *old_index_env =NULL , *old_repo_index_file;remove_path (stash_index_path.buf);
@@ -1015,16 +1015,19 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, }/* Find out what the user wants. */ - cp_add_i.git_cmd =1 ; -argv_array_pushl (&cp_add_i.args,"add--interactive" ,"--patch=stash" , -"--" ,NULL ); -add_pathspecs (&cp_add_i.args, ps); -argv_array_pushf (&cp_add_i.env_array,"GIT_INDEX_FILE=%s" , - stash_index_path.buf); -if (run_command (&cp_add_i)) { - ret =-1 ; - goto done; - } + old_repo_index_file = the_repository->index_file; + the_repository->index_file = stash_index_path.buf; + old_index_env =xstrdup_or_null (getenv (INDEX_ENVIRONMENT )); +setenv (INDEX_ENVIRONMENT , the_repository->index_file,1 ); + + ret =run_add_interactive (NULL ,"--patch=stash" , ps); + + the_repository->index_file = old_repo_index_file; +if (old_index_env && *old_index_env) +setenv (INDEX_ENVIRONMENT , old_index_env,1 ); +else +unsetenv (INDEX_ENVIRONMENT ); +FREE_AND_NULL (old_index_env);/* State of the working tree. */ if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0,
Signature · 1 line
gitgitgadget
Re: [PATCH 4/7] built-in stash: use the built-in `git add -p` if so configured
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
builtin/stash.c +0
-0
builtin/stash.c static int stash_patch(struct stash_info *info, const struct pathspec *ps, · line 1007
Quoted 10 lines
builtin/stash.c +1
-1@@ -999,9 +999,9 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, {int ret =0 ;struct child_process cp_read_tree =CHILD_PROCESS_INIT ; -struct child_process cp_add_i =CHILD_PROCESS_INIT ;struct child_process cp_diff_tree =CHILD_PROCESS_INIT ;struct index_state istate = {NULL }; +char *old_index_env =NULL , *old_repo_index_file;remove_path (stash_index_path.buf);
Together with the previous step, it makes sense. Earlier we always spawned the scripted version. Now we first give control to the C code and allow it to punt to the scripted version unless it is told to take control with USE_BUILTIN.
Quoted 17 lines
builtin/stash.c +3
-10@@ -1015,16 +1015,19 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, }/* Find out what the user wants. */ - cp_add_i.git_cmd =1 ; -argv_array_pushl (&cp_add_i.args,"add--interactive" ,"--patch=stash" , -"--" ,NULL ); -add_pathspecs (&cp_add_i.args, ps); -argv_array_pushf (&cp_add_i.env_array,"GIT_INDEX_FILE=%s" , - stash_index_path.buf); -if (run_command (&cp_add_i)) { - ret =-1 ; - goto done; - } + old_repo_index_file = the_repository->index_file; + the_repository->index_file = stash_index_path.buf; + old_index_env =xstrdup_or_null (getenv (INDEX_ENVIRONMENT ));
builtin/stash.c static int stash_patch(struct stash_info *info, const struct pathspec *ps, · line 1030
Quoted 10 lines
builtin/stash.c +10
-0+setenv (INDEX_ENVIRONMENT , the_repository->index_file,1 ); + + ret =run_add_interactive (NULL ,"--patch=stash" , ps); + + the_repository->index_file = old_repo_index_file; +if (old_index_env && *old_index_env) +setenv (INDEX_ENVIRONMENT , old_index_env,1 ); +else +unsetenv (INDEX_ENVIRONMENT ); +FREE_AND_NULL (old_index_env);
OK. I suspect that, as we move more stuff that used to be an external call with one-shot environment assignments like this to an internall call, we'd see patterns of "save away the state of this and that environment variables, then replace them with these values" paired with "now restore the state of those environment variables".
We might eventually want to add a helper for doing so easily, which may make the caller look like
extern void save_environment(struct saved_env *, ...);
struct saved_env env;
save_environment(&env,
INDEX_ENVIRONMENT, the_repository->index_file,
NULL /* end of "VAR, val" tuples */);
ret = run_add_interactive(NULL, "--patch=stash", ps);
restore_environment(&env);It might (or might not) be premature to introduce such a helper at this stage in the series, though.
Thanks.
Re: [PATCH 4/7] built-in stash: use the built-in `git add -p` if so configured
Hi Junio,
Quoted 72 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
builtin/stash.c +0
-0builtin/stash.c static int stash_patch(struct stash_info *info, const struct pathspec *ps, · line 1007
Quoted 10 lines
builtin/stash.c +1
-1@@ -999,9 +999,9 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, {int ret =0 ;struct child_process cp_read_tree =CHILD_PROCESS_INIT ; -struct child_process cp_add_i =CHILD_PROCESS_INIT ;struct child_process cp_diff_tree =CHILD_PROCESS_INIT ;struct index_state istate = {NULL }; +char *old_index_env =NULL , *old_repo_index_file;remove_path (stash_index_path.buf);Together with the previous step, it makes sense. Earlier we always spawned the scripted version. Now we first give control to the C code and allow it to punt to the scripted version unless it is told to take control with
USE_BUILTIN.Quoted 17 lines
builtin/stash.c +3
-10@@ -1015,16 +1015,19 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, }/* Find out what the user wants. */ - cp_add_i.git_cmd =1 ; -argv_array_pushl (&cp_add_i.args,"add--interactive" ,"--patch=stash" , -"--" ,NULL ); -add_pathspecs (&cp_add_i.args, ps); -argv_array_pushf (&cp_add_i.env_array,"GIT_INDEX_FILE=%s" , - stash_index_path.buf); -if (run_command (&cp_add_i)) { - ret =-1 ; - goto done; - } + old_repo_index_file = the_repository->index_file; + the_repository->index_file = stash_index_path.buf; + old_index_env =xstrdup_or_null (getenv (INDEX_ENVIRONMENT ));builtin/stash.c static int stash_patch(struct stash_info *info, const struct pathspec *ps, · line 1030
Quoted 10 lines
builtin/stash.c +10
-0+setenv (INDEX_ENVIRONMENT , the_repository->index_file,1 ); + + ret =run_add_interactive (NULL ,"--patch=stash" , ps); + + the_repository->index_file = old_repo_index_file; +if (old_index_env && *old_index_env) +setenv (INDEX_ENVIRONMENT , old_index_env,1 ); +else +unsetenv (INDEX_ENVIRONMENT ); +FREE_AND_NULL (old_index_env);OK. I suspect that, as we move more stuff that used to be an external call with one-shot environment assignments like this to an internall call, we'd see patterns of "save away the state of this and that environment variables, then replace them with these values" paired with "now restore the state of those environment variables".
We might eventually want to add a helper for doing so easily, which may make the caller look like
extern void save_environment(struct saved_env *, ...); struct saved_env env; save_environment(&env, INDEX_ENVIRONMENT, the_repository->index_file, NULL /* end of "VAR, val" tuples */); ret = run_add_interactive(NULL, "--patch=stash", ps); restore_environment(&env);
Quoted 2 lines
It might (or might not) be premature to introduce such a helper at this stage in the series, though.
To be honest, I would rather have this at a different layer: I'd like run_add_interactive() to take an optional path to the index, to be able to override the default <gitdir>/index.
But that requires either quite a lot of changes to the Perl script, or it needs to wait until the built-in version of git add -p stabilized enough to allow retiring the Perl script.
I kinda aimed for the latter solution.
Ciao,
Dscho
PATCH 5/7 built-in add -p: implement the "checkout" patch modes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This patch teaches the built-in git add -p machinery all the tricks it needs to know in order to act as the work horse for git checkout -p.
Apart from the minor changes (slightly reworded messages, different diff and apply --check invocations), it requires a new function to actually apply the changes, as git checkout -p is a bit special in that respect: when the desired changes do not apply to the index, but apply to the work tree, Git does not fail straight away, but asks the user whether to apply the changes to the worktree at least.
--- add-interactive.h | 1 + add-patch.c | 139 ++++++++++++++++++++++++++++++++++++++++++++-- builtin/add.c | 5 +- 3 files changed, 138 insertions(+), 7 deletions(-)
add-interactive.h +1 -0
@@ -27,6 +27,7 @@ enum add_p_mode {ADD_P_STAGE ,ADD_P_STASH ,ADD_P_RESET , + ADD_P_CHECKOUT, }; int run_add_p(struct repository *r, enum add_p_mode mode,
add-patch.c +134 -5
163 lines not on this page · raw message
builtin/add.c +3 -2
@@ -206,9 +206,10 @@ int run_add_interactive(const char *revision, const char *patch_mode, 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 - die("'%s' not yet supported in the built-in add -p", - patch_mode); + die("'%s' not supported", patch_mode);return !!run_add_p (the_repository, mode, revision, pathspec); }
Signature · 1 line
gitgitgadget
PATCH 6/7 built-in add -p: implement the "worktree" patch modes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This is a straight-forward port of 2f0896ec3ad4 (restore: support --patch, 2019-04-25) which added support for git restore -p.
--- add-interactive.h | 1 + add-patch.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ builtin/add.c | 2 ++ 3 files changed, 54 insertions(+)
add-interactive.h +1 -0
@@ -28,6 +28,7 @@ enum add_p_mode {ADD_P_STASH ,ADD_P_RESET ,ADD_P_CHECKOUT , + ADD_P_WORKTREE, }; int run_add_p(struct repository *r, enum add_p_mode mode,
add-patch.c +51 -0
63 lines not on this page · raw message
builtin/add.c +2 -0
@@ -208,6 +208,8 @@ int run_add_interactive(const char *revision, const char *patch_mode, 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);
Signature · 1 line
gitgitgadget
PATCH 7/7 commit --interactive: make it work with the built-in `add -i`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The built-in git add -i machinery obviously has its the_repository structure initialized at the point where cmd_commit() calls it, and therefore does not look at the environment variable GIT_INDEX_FILE.
But when being called from commit --interactive, it has to, because the index was already locked in that case, and we want to ask the interactive add machinery to work on the index.lock file instead of the index file.
Technically, we could teach run_add_i(), or for that matter run_add_p(), to look specifically at that environment variable, but the entire idea of passing in a parameter of type struct repository * is to allow working on multiple repositories (and their index files) independently.
So let's instead override the index_file field of that structure temporarily.
--- builtin/commit.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
builtin/commit.c +6 -2
@@ -347,7 +347,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixdie (_ ("index file corrupt" ));if (interactive) { -char *old_index_env =NULL ; +char *old_index_env =NULL , *old_repo_index_file;hold_locked_index (&index_lock,LOCK_DIE_ON_ERROR );refresh_cache_or_die (refresh_flags);
@@ -355,12 +355,16 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixif (write_locked_index (&the_index, &index_lock,0 ))die (_ ("unable to create temporary index" )); + old_repo_index_file = the_repository->index_file; + the_repository->index_file = + (char *)get_lock_file_path (&index_lock); old_index_env =xstrdup_or_null (getenv (INDEX_ENVIRONMENT )); -setenv (INDEX_ENVIRONMENT ,get_lock_file_path (&index_lock),1 ); +setenv (INDEX_ENVIRONMENT , the_repository->index_file,1 );if (interactive_add (argc, argv, prefix, patch_interactive) !=0 )die (_ ("interactive add failed" )); + the_repository->index_file = old_repo_index_file;if (old_index_env && *old_index_env)setenv (INDEX_ENVIRONMENT , old_index_env,1 );else
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
At this stage on the journey to a fully built-in git add, we already have everything we need, including the --interactive and --patch options, as long as the add.interactive.useBuiltin setting is set to true. This config setting is kind of a feature flag that is currently turned off by default, and will be for a while, until we get confident enough that the built-in version does the job, and retire the Perl script.
Except for a few things that are all minor I already mentioned, and that I'd need to think a bit more to form an opinion on 6/7 and 7/7, they all looked quite straight-forward.
Will queue.
v2 cover stash/reset/checkout -p: optionally use the add --patch backend written in pure C
In reply to Johannes Schindelin via GitGitGadgetAt this stage on the journey to a fully built-in git add, we already have everything we need, including the --interactive and --patch options, as long as the add.interactive.useBuiltin setting is set to true. This config setting is kind of a feature flag that is currently turned off by default, and will be for a while, until we get confident enough that the built-in version does the job, and retire the Perl script.
However, the internal add--interactive helper is also used to back the --patch option of git stash, git reset, git checkout and git worktree.
This patch series (based on js/add-p-in-c) brings them "online".
Changes since v1:
* Renamed patch_mode_stage to patch_mode_add (and ADD_P_STAGE to ADD_P_ADD ).
* Renamed the fields diff, apply and apply_check (that were inherited from the Perl script) to diff_cmd, apply_args and apply_check_args, respectively.
* Clarified the magic array size 4 of diff_cmd and friends.
* Introduced pseudo value PROMPT_MODE_MAX to make the array size of prompt_mode more obvious.
* Got rid of the unneeded = 0 initializers for is_reverse.
Johannes Schindelin (7):
built-in add -p: prepare for patch modes other than "stage" built-in add -p: implement the "stash" and "reset" patch modes legacy stash -p: respect the add.interactive.usebuiltin setting built-in stash: use the built-in git add -p if so configured built-in add -p: implement the "checkout" patch modes built-in add -p: implement the "worktree" patch modes commit --interactive: make it work with the built-in add -i
add-interactive.c | 2 +- add-interactive.h | 12 +- add-patch.c | 356 ++++++++++++++++++++++++++++++++++++++++---- builtin/add.c | 35 ++++- builtin/commit.c | 8 +- builtin/stash.c | 25 ++-- git-legacy-stash.sh | 2 +- 7 files changed, 390 insertions(+), 50 deletions(-)
base-commit: 2e4083198d1508206488af4c82093ceb6cf20f4e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-174%2Fdscho%2Fother-command-p-in-c-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-174/dscho/other-command-p-in-c-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/174
Range-diff vs v1:
1: 8f6139f94d ! 1: d894f8f427 built-in add -p: prepare for patch modes other than "stage"
@@ -27,7 +27,7 @@
PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
PATHSPEC_LITERAL_PATH, "", args.argv);
- res = run_add_p(s->r, &ps_selected);
-+ res = run_add_p(s->r, ADD_P_STAGE, NULL, &ps_selected);
++ res = run_add_p(s->r, ADD_P_ADD, NULL, &ps_selected);
argv_array_clear(&args);
clear_pathspec(&ps_selected);
}
@@ -42,7 +42,7 @@
-int run_add_p(struct repository *r, const struct pathspec *ps);
+
+enum add_p_mode {
-+ ADD_P_STAGE,
++ ADD_P_ADD,
+};
+
+int run_add_p(struct repository *r, enum add_p_mode mode,
@@ -54,7 +54,12 @@
--- a/add-patch.c
+++ b/add-patch.c
@@
- PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK
+ #include "diff.h"
+
+ enum prompt_mode_type {
+- PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK
++ PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK,
++ PROMPT_MODE_MAX, /* must be last */
};
-static const char *prompt_mode[] = {
@@ -62,17 +67,21 @@
- N_("Stage deletion [y,n,a,q,d%s,?]? "),
- N_("Stage this hunk [y,n,a,q,d%s,?]? ")
+struct patch_mode {
-+ const char *diff[4], *apply[4], *apply_check[4];
++ /*
++ * The magic constant 4 is chosen such that all patch modes
++ * provide enough space for three command-line arguments followed by a
++ * trailing `NULL`.
++ */
++ const char *diff_cmd[4], *apply_args[4], *apply_check_args[4];
+ unsigned is_reverse:1, apply_for_checkout:1;
-+ const char *prompt_mode[PROMPT_HUNK + 1];
++ const char *prompt_mode[PROMPT_MODE_MAX];
+ const char *edit_hunk_hint, *help_patch_text;
+};
+
-+static struct patch_mode patch_mode_stage = {
-+ .diff = { "diff-files", NULL },
-+ .apply = { "--cached", NULL },
-+ .apply_check = { "--cached", NULL },
-+ .is_reverse = 0,
++static struct patch_mode patch_mode_add = {
++ .diff_cmd = { "diff-files", NULL },
++ .apply_args = { "--cached", NULL },
++ .apply_check_args = { "--cached", NULL },
+ .prompt_mode = {
+ N_("Stage mode change [y,n,q,a,d%s,?]? "),
+ N_("Stage deletion [y,n,q,a,d%s,?]? "),
@@ -106,7 +115,7 @@
struct hunk *hunk = NULL;
int res;
-+ argv_array_pushv(&args, s->mode->diff);
++ argv_array_pushv(&args, s->mode->diff_cmd);
+ if (s->revision) {
+ struct object_id oid;
+ argv_array_push(&args,
@@ -157,7 +166,7 @@
setup_child_process(s, &cp,
- "apply", "--cached", "--check", NULL);
+ "apply", "--check", NULL);
-+ argv_array_pushv(&cp.args, s->mode->apply_check);
++ argv_array_pushv(&cp.args, s->mode->apply_check_args);
if (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0))
return error(_("'git apply --cached' failed"));
@@ -200,7 +209,7 @@
discard_index(s->s.r->index);
- setup_child_process(s, &cp, "apply", "--cached", NULL);
+ setup_child_process(s, &cp, "apply", NULL);
-+ argv_array_pushv(&cp.args, s->mode->apply);
++ argv_array_pushv(&cp.args, s->mode->apply_args);
if (pipe_command(&cp, s->buf.buf, s->buf.len,
NULL, 0, NULL, 0))
- error(_("'git apply --cached' failed"));
@@ -222,7 +231,7 @@
init_add_i_state(&s.s, r);
-+ s.mode = &patch_mode_stage;
++ s.mode = &patch_mode_add;
+ s.revision = revision;
+
if (discard_index(r->index) < 0 || repo_read_index(r) < 0 ||
@@ -243,7 +252,7 @@
- if (strcmp(patch_mode, "--patch"))
+
+ if (!strcmp(patch_mode, "--patch"))
-+ mode = ADD_P_STAGE;
++ mode = ADD_P_ADD;
+ else
die("'%s' not yet supported in the built-in add -p",
patch_mode);
2: 846cf16e77 ! 2: 6977bede86 built-in add -p: implement the "stash" and "reset" patch modes
@@ -14,7 +14,7 @@
@@
enum add_p_mode {
- ADD_P_STAGE,
+ ADD_P_ADD,
+ ADD_P_STASH,
+ ADD_P_RESET,
};
@@ -25,12 +25,12 @@
--- a/add-patch.c
+++ b/add-patch.c
@@
-
- struct patch_mode {
- const char *diff[4], *apply[4], *apply_check[4];
+ * trailing `NULL`.
+ */
+ const char *diff_cmd[4], *apply_args[4], *apply_check_args[4];
- unsigned is_reverse:1, apply_for_checkout:1;
+ unsigned is_reverse:1, index_only:1, apply_for_checkout:1;
- const char *prompt_mode[PROMPT_HUNK + 1];
+ const char *prompt_mode[PROMPT_MODE_MAX];
const char *edit_hunk_hint, *help_patch_text;
};
@@
@@ -38,10 +38,9 @@
};
+static struct patch_mode patch_mode_stash = {
-+ .diff = { "diff-index", "HEAD", NULL },
-+ .apply = { "--cached", NULL },
-+ .apply_check = { "--cached", NULL },
-+ .is_reverse = 0,
++ .diff_cmd = { "diff-index", "HEAD", NULL },
++ .apply_args = { "--cached", NULL },
++ .apply_check_args = { "--cached", NULL },
+ .prompt_mode = {
+ N_("Stash mode change [y,n,q,a,d%s,?]? "),
+ N_("Stash deletion [y,n,q,a,d%s,?]? "),
@@ -60,9 +59,9 @@
+};
+
+static struct patch_mode patch_mode_reset_head = {
-+ .diff = { "diff-index", "--cached", NULL },
-+ .apply = { "-R", "--cached", NULL },
-+ .apply_check = { "-R", "--cached", NULL },
++ .diff_cmd = { "diff-index", "--cached", NULL },
++ .apply_args = { "-R", "--cached", NULL },
++ .apply_check_args = { "-R", "--cached", NULL },
+ .is_reverse = 1,
+ .index_only = 1,
+ .prompt_mode = {
@@ -83,10 +82,9 @@
+};
+
+static struct patch_mode patch_mode_reset_nothead = {
-+ .diff = { "diff-index", "-R", "--cached", NULL },
-+ .apply = { "--cached", NULL },
-+ .apply_check = { "--cached", NULL },
-+ .is_reverse = 0,
++ .diff_cmd = { "diff-index", "-R", "--cached", NULL },
++ .apply_args = { "--cached", NULL },
++ .apply_check_args = { "--cached", NULL },
+ .index_only = 1,
+ .prompt_mode = {
+ N_("Apply mode change to index [y,n,q,a,d%s,?]? "),
@@ -112,7 +110,7 @@
init_add_i_state(&s.s, r);
-- s.mode = &patch_mode_stage;
+- s.mode = &patch_mode_add;
+ if (mode == ADD_P_STASH)
+ s.mode = &patch_mode_stash;
+ else if (mode == ADD_P_RESET) {
@@ -121,7 +119,7 @@
+ else
+ s.mode = &patch_mode_reset_nothead;
+ } else
-+ s.mode = &patch_mode_stage;
++ s.mode = &patch_mode_add;
s.revision = revision;
if (discard_index(r->index) < 0 || repo_read_index(r) < 0 ||
@@ -140,7 +138,7 @@
@@
if (!strcmp(patch_mode, "--patch"))
- mode = ADD_P_STAGE;
+ mode = ADD_P_ADD;
+ else if (!strcmp(patch_mode, "--patch=stash"))
+ mode = ADD_P_STASH;
+ else if (!strcmp(patch_mode, "--patch=reset"))
3: cddc2cb9de = 3: bf9b7f897a legacy stash -p: respect the add.interactive.usebuiltin setting
4: 40c00302f6 = 4: 5051f37559 built-in stash: use the built-in `git add -p` if so configured
5: 3c55f106c7 ! 5: b3672b35e6 built-in add -p: implement the "checkout" patch modes
@@ -18,7 +18,7 @@
--- a/add-interactive.h
+++ b/add-interactive.h
@@
- ADD_P_STAGE,
+ ADD_P_ADD,
ADD_P_STASH,
ADD_P_RESET,
+ ADD_P_CHECKOUT,
@@ -34,9 +34,9 @@
};
+static struct patch_mode patch_mode_checkout_index = {
-+ .diff = { "diff-files", NULL },
-+ .apply = { "-R", NULL },
-+ .apply_check = { "-R", NULL },
++ .diff_cmd = { "diff-files", NULL },
++ .apply_args = { "-R", NULL },
++ .apply_check_args = { "-R", NULL },
+ .is_reverse = 1,
+ .prompt_mode = {
+ N_("Discard mode change from worktree [y,n,q,a,d%s,?]? "),
@@ -56,9 +56,9 @@
+};
+
+static struct patch_mode patch_mode_checkout_head = {
-+ .diff = { "diff-index", NULL },
++ .diff_cmd = { "diff-index", NULL },
+ .apply_for_checkout = 1,
-+ .apply_check = { "-R", NULL },
++ .apply_check_args = { "-R", NULL },
+ .is_reverse = 1,
+ .prompt_mode = {
+ N_("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "),
@@ -78,10 +78,9 @@
+};
+
+static struct patch_mode patch_mode_checkout_nothead = {
-+ .diff = { "diff-index", "-R", NULL },
++ .diff_cmd = { "diff-index", "-R", NULL },
+ .apply_for_checkout = 1,
-+ .apply_check = { NULL },
-+ .is_reverse = 0,
++ .apply_check_args = { NULL },
+ .prompt_mode = {
+ N_("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "),
+ N_("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "),
@@ -165,7 +164,7 @@
discard_index(s->s.r->index);
- setup_child_process(s, &cp, "apply", NULL);
-- argv_array_pushv(&cp.args, s->mode->apply);
+- argv_array_pushv(&cp.args, s->mode->apply_args);
- if (pipe_command(&cp, s->buf.buf, s->buf.len,
- NULL, 0, NULL, 0))
- error(_("'git apply' failed"));
@@ -174,7 +173,7 @@
+ s->mode->is_reverse);
+ else {
+ setup_child_process(s, &cp, "apply", NULL);
-+ argv_array_pushv(&cp.args, s->mode->apply);
++ argv_array_pushv(&cp.args, s->mode->apply_args);
+ if (pipe_command(&cp, s->buf.buf, s->buf.len,
+ NULL, 0, NULL, 0))
+ error(_("'git apply' failed"));
@@ -194,7 +193,7 @@
+ else
+ s.mode = &patch_mode_checkout_nothead;
} else
- s.mode = &patch_mode_stage;
+ s.mode = &patch_mode_add;
s.revision = revision;
diff --git a/builtin/add.c b/builtin/add.c
6: b63fca6dab ! 6: 83e2ac6e67 built-in add -p: implement the "worktree" patch modes
@@ -27,9 +27,9 @@
};
+static struct patch_mode patch_mode_worktree_head = {
-+ .diff = { "diff-index", NULL },
-+ .apply = { "-R", NULL },
-+ .apply_check = { "-R", NULL },
++ .diff_cmd = { "diff-index", NULL },
++ .apply_args = { "-R", NULL },
++ .apply_check_args = { "-R", NULL },
+ .is_reverse = 1,
+ .prompt_mode = {
+ N_("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "),
@@ -49,10 +49,9 @@
+};
+
+static struct patch_mode patch_mode_worktree_nothead = {
-+ .diff = { "diff-index", "-R", NULL },
-+ .apply = { NULL },
-+ .apply_check = { NULL },
-+ .is_reverse = 0,
++ .diff_cmd = { "diff-index", "-R", NULL },
++ .apply_args = { NULL },
++ .apply_check_args = { NULL },
+ .prompt_mode = {
+ N_("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "),
+ N_("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "),
@@ -85,7 +84,7 @@
+ else
+ s.mode = &patch_mode_worktree_nothead;
} else
- s.mode = &patch_mode_stage;
+ s.mode = &patch_mode_add;
s.revision = revision;
diff --git a/builtin/add.c b/builtin/add.c 7: 7a4c330d03 = 7: ecb22159d8 commit --interactive: make it work with the built-in add -i
Signature · 1 line
gitgitgadget
PATCH v2 1/7 built-in add -p: prepare for patch modes other than "stage"
From: Johannes Schindelin <[email protected]>
The Perl script backing git add -p is used not only for that command, but also for git stash -p, git reset -p and git checkout -p.
In preparation for teaching the C version of git add -p to support also the latter commands, let's abstract away what is "stage" specific into a dedicated data structure describing the differences between the patch modes.
Finally, please note that the Perl version tries to make sure that the diffs are only generated for the modified files. This is not actually necessary, as the calls to Git's diff machinery already perform that work, and perform it well. This makes it unnecessary to port the FILTER field of the %patch_modes struct, as well as the get_diff_reference() function.
--- add-interactive.c | 2 +- add-interactive.h | 8 +++- add-patch.c | 95 ++++++++++++++++++++++++++++++++++------------- builtin/add.c | 10 ++++- 4 files changed, 85 insertions(+), 30 deletions(-)
add-interactive.c +1 -1
@@ -924,7 +924,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,parse_pathspec (&ps_selected,PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL ,PATHSPEC_LITERAL_PATH ,"" , args.argv); - res =run_add_p (s->r, &ps_selected); + res =run_add_p (s->r,ADD_P_ADD ,NULL , &ps_selected);argv_array_clear (&args);clear_pathspec (&ps_selected); }
add-interactive.h +7 -1
@@ -22,6 +22,12 @@ void init_add_i_state(struct add_i_state *s, struct repository *r);struct repository ;struct pathspec ;int run_add_i (struct repository *r,const struct pathspec *ps); -int run_add_p (struct repository *r,const struct pathspec *ps); + +enum add_p_mode { +ADD_P_ADD , +}; + +int run_add_p (struct repository *r,enum add_p_mode mode, +const char *revision,const struct pathspec *ps);#endif
add-patch.c +69 -26
172 lines not on this page · raw message
builtin/add.c +8 -2
@@ -194,12 +194,18 @@ int run_add_interactive(const char *revision, const char *patch_mode, &use_builtin_add_i);if (use_builtin_add_i ==1 ) { +enum add_p_mode mode; +if (!patch_mode)return !!run_add_i (the_repository, pathspec); -if (strcmp (patch_mode,"--patch" )) + +if (!strcmp (patch_mode,"--patch" )) + mode =ADD_P_ADD ; +else die ("'%s' not yet supported in the built-in add -p" , patch_mode); -return !!run_add_p (the_repository, pathspec); + +return !!run_add_p (the_repository, mode, revision, pathspec); }argv_array_push (&argv,"add--interactive" );
Signature · 1 line
gitgitgadget
PATCH v2 2/7 built-in add -p: implement the "stash" and "reset" patch modes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The git stash and git reset commands support a --patch option, and both simply hand off to git add -p to perform that work. Let's teach the built-in version of that command to be able to perform that work, too.
--- add-interactive.h | 2 ++ add-patch.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--- builtin/add.c | 4 +++ 3 files changed, 85 insertions(+), 4 deletions(-)
add-interactive.h +2 -0
@@ -25,6 +25,8 @@ int run_add_i(struct repository *r, const struct pathspec *ps);enum add_p_mode {ADD_P_ADD , +ADD_P_STASH , +ADD_P_RESET , }; int run_add_p(struct repository *r, enum add_p_mode mode,
add-patch.c +79 -4
104 lines not on this page · raw message
builtin/add.c +4 -0
@@ -201,6 +201,10 @@ int run_add_interactive(const char *revision, const char *patch_mode,if (!strcmp (patch_mode,"--patch" )) mode =ADD_P_ADD ; +else if (!strcmp (patch_mode,"--patch=stash" )) + mode =ADD_P_STASH ; +else if (!strcmp (patch_mode,"--patch=reset" )) + mode =ADD_P_RESET ;else die ("'%s' not yet supported in the built-in add -p" , patch_mode);
Signature · 1 line
gitgitgadget
PATCH v2 3/7 legacy stash -p: respect the add.interactive.usebuiltin setting
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
As git add traditionally did not expose the --patch=<mode> modes via command-line options, the scripted version of git stash had to call git add--interactive directly.
But this prevents the built-in add -p from kicking in, as add--interactive is the scripted version (which does not have a "fall-back" to the built-in version).
So let's introduce support for internal switch for git add that the scripted git stash can use to call the appropriate backend (scripted or built-in, depending on add.interactive.useBuiltin).
--- builtin/add.c | 14 ++++++++++++++ git-legacy-stash.sh | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-)
builtin/add.c +14 -0
@@ -29,6 +29,7 @@ static const char * const builtin_add_usage[] = {static int patch_interactive, add_interactive, edit_interactive;static int take_worktree_changes;static int add_renormalize; +static int legacy_stash_p;/* support for the scripted `git stash` */ struct update_callback_data {int flags;
@@ -335,6 +336,8 @@ static struct option builtin_add_options[] = { N_("override the executable bit of the listed files")),OPT_HIDDEN_BOOL (0 ,"warn-embedded-repo" , &warn_on_embedded_repo,N_ ("warn when adding an embedded repository" )), +OPT_HIDDEN_BOOL (0 ,"legacy-stash-p" , &legacy_stash_p, +N_ ("backend for `git stash -p`" )), OPT_END(), };
@@ -431,6 +434,17 @@ int cmd_add(int argc, const char **argv, const char *prefix) add_interactive =1 ;if (add_interactive)exit (interactive_add (argc -1 , argv +1 , prefix, patch_interactive)); +if (legacy_stash_p) { +struct pathspec pathspec; + +parse_pathspec (&pathspec,0 , +PATHSPEC_PREFER_FULL | +PATHSPEC_SYMLINK_LEADING_PATH | +PATHSPEC_PREFIX_ORIGIN , + prefix, argv); + +return run_add_interactive (NULL ,"--patch=stash" , &pathspec); + }if (edit_interactive)return (edit_patch (argc, argv, prefix));
git-legacy-stash.sh +1 -1
@@ -206,7 +206,7 @@ create_stash () { # find out what the user wants GIT_INDEX_FILE="$TMP-index" \ - git add--interactive --patch=stash -- "$@" && + git add --legacy-stash-p -- "$@" && # state of the working tree w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
Signature · 1 line
gitgitgadget
PATCH v2 4/7 built-in stash: use the built-in `git add -p` if so configured
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The scripted version of git stash called directly into the Perl script git-add--interactive.perl, and this was faithfully converted to C.
However, we have a much better way to do this now: call the internal API directly, which will now incidentally also respect the add.interactive.useBuiltin setting. Let's just do this.
--- builtin/stash.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
builtin/stash.c +14 -11
@@ -999,9 +999,9 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, {int ret =0 ;struct child_process cp_read_tree =CHILD_PROCESS_INIT ; -struct child_process cp_add_i =CHILD_PROCESS_INIT ;struct child_process cp_diff_tree =CHILD_PROCESS_INIT ;struct index_state istate = {NULL }; +char *old_index_env =NULL , *old_repo_index_file;remove_path (stash_index_path.buf);
@@ -1015,16 +1015,19 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, }/* Find out what the user wants. */ - cp_add_i.git_cmd =1 ; -argv_array_pushl (&cp_add_i.args,"add--interactive" ,"--patch=stash" , -"--" ,NULL ); -add_pathspecs (&cp_add_i.args, ps); -argv_array_pushf (&cp_add_i.env_array,"GIT_INDEX_FILE=%s" , - stash_index_path.buf); -if (run_command (&cp_add_i)) { - ret =-1 ; - goto done; - } + old_repo_index_file = the_repository->index_file; + the_repository->index_file = stash_index_path.buf; + old_index_env =xstrdup_or_null (getenv (INDEX_ENVIRONMENT )); +setenv (INDEX_ENVIRONMENT , the_repository->index_file,1 ); + + ret =run_add_interactive (NULL ,"--patch=stash" , ps); + + the_repository->index_file = old_repo_index_file; +if (old_index_env && *old_index_env) +setenv (INDEX_ENVIRONMENT , old_index_env,1 ); +else +unsetenv (INDEX_ENVIRONMENT ); +FREE_AND_NULL (old_index_env);/* State of the working tree. */ if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0,
Signature · 1 line
gitgitgadget
PATCH v2 5/7 built-in add -p: implement the "checkout" patch modes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This patch teaches the built-in git add -p machinery all the tricks it needs to know in order to act as the work horse for git checkout -p.
Apart from the minor changes (slightly reworded messages, different diff and apply --check invocations), it requires a new function to actually apply the changes, as git checkout -p is a bit special in that respect: when the desired changes do not apply to the index, but apply to the work tree, Git does not fail straight away, but asks the user whether to apply the changes to the worktree at least.
--- add-interactive.h | 1 + add-patch.c | 138 ++++++++++++++++++++++++++++++++++++++++++++-- builtin/add.c | 5 +- 3 files changed, 137 insertions(+), 7 deletions(-)
add-interactive.h +1 -0
@@ -27,6 +27,7 @@ enum add_p_mode {ADD_P_ADD ,ADD_P_STASH ,ADD_P_RESET , + ADD_P_CHECKOUT, }; int run_add_p(struct repository *r, enum add_p_mode mode,
add-patch.c +133 -5
162 lines not on this page · raw message
builtin/add.c +3 -2
@@ -206,9 +206,10 @@ int run_add_interactive(const char *revision, const char *patch_mode, 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 - die("'%s' not yet supported in the built-in add -p", - patch_mode); + die("'%s' not supported", patch_mode);return !!run_add_p (the_repository, mode, revision, pathspec); }
Signature · 1 line
gitgitgadget
PATCH v2 6/7 built-in add -p: implement the "worktree" patch modes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This is a straight-forward port of 2f0896ec3ad4 (restore: support --patch, 2019-04-25) which added support for git restore -p.
--- add-interactive.h | 1 + add-patch.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ builtin/add.c | 2 ++ 3 files changed, 53 insertions(+)
add-interactive.h +1 -0
@@ -28,6 +28,7 @@ enum add_p_mode {ADD_P_STASH ,ADD_P_RESET ,ADD_P_CHECKOUT , + ADD_P_WORKTREE, }; int run_add_p(struct repository *r, enum add_p_mode mode,
add-patch.c +50 -0
62 lines not on this page · raw message
builtin/add.c +2 -0
@@ -208,6 +208,8 @@ int run_add_interactive(const char *revision, const char *patch_mode, 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);
Signature · 1 line
gitgitgadget
PATCH v2 7/7 commit --interactive: make it work with the built-in `add -i`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The built-in git add -i machinery obviously has its the_repository structure initialized at the point where cmd_commit() calls it, and therefore does not look at the environment variable GIT_INDEX_FILE.
But when being called from commit --interactive, it has to, because the index was already locked in that case, and we want to ask the interactive add machinery to work on the index.lock file instead of the index file.
Technically, we could teach run_add_i(), or for that matter run_add_p(), to look specifically at that environment variable, but the entire idea of passing in a parameter of type struct repository * is to allow working on multiple repositories (and their index files) independently.
So let's instead override the index_file field of that structure temporarily.
--- builtin/commit.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
builtin/commit.c +6 -2
@@ -347,7 +347,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixdie (_ ("index file corrupt" ));if (interactive) { -char *old_index_env =NULL ; +char *old_index_env =NULL , *old_repo_index_file;hold_locked_index (&index_lock,LOCK_DIE_ON_ERROR );refresh_cache_or_die (refresh_flags);
@@ -355,12 +355,16 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixif (write_locked_index (&the_index, &index_lock,0 ))die (_ ("unable to create temporary index" )); + old_repo_index_file = the_repository->index_file; + the_repository->index_file = + (char *)get_lock_file_path (&index_lock); old_index_env =xstrdup_or_null (getenv (INDEX_ENVIRONMENT )); -setenv (INDEX_ENVIRONMENT ,get_lock_file_path (&index_lock),1 ); +setenv (INDEX_ENVIRONMENT , the_repository->index_file,1 );if (interactive_add (argc, argv, prefix, patch_interactive) !=0 )die (_ ("interactive add failed" )); + the_repository->index_file = old_repo_index_file;if (old_index_env && *old_index_env)setenv (INDEX_ENVIRONMENT , old_index_env,1 );else
Signature · 1 line
gitgitgadget