Turn git add-i into built-in
PATCH 1/7 diff: export diffstat interface
In reply to Johannes SchindelinFrom: Daniel Ferreira <[email protected]>
Make the diffstat interface (namely, the diffstat_t struct and compute_diffstat) no longer be internal to diff.c and allow it to be used by other parts of git.
This is helpful for code that may want to easily extract information from files using the diff machinery, while flushing it differently from how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status.
--- diff.c | 36 ++++++++++++++---------------------- diff.h | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 22 deletions(-)
diff.c +14 -22
@@ -2421,22 +2421,6 @@ static void pprint_rename(struct strbuf *name, const char *a, const char *b) } } -struct diffstat_t { -int nr; -int alloc; -struct diffstat_file { -char *from_name; -char *name; -char *print_name; -const char *comments; -unsigned is_unmerged:1 ; -unsigned is_binary:1 ; -unsigned is_renamed:1 ; -unsigned is_interesting:1 ; -uintmax_t added, deleted; - } **files; -}; -static struct diffstat_file *diffstat_add (struct diffstat_t *diffstat,const char *name_a,const char *name_b)
@@ -5922,12 +5906,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat; -memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )
@@ -6227,6 +6206,19 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)return ignored; } +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff; + +memset (diffstat,0 ,sizeof (struct diffstat_t )); +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +if (check_pair_status (p)) +diff_flush_stat (p, options, diffstat); + } +} + void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid,
diff.h +18 -0
@@ -239,6 +239,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);void diff_emit_submodule_pipethrough (struct diff_options *o,const char *line,int len); +struct diffstat_t { +int nr; +int alloc; +struct diffstat_file { +char *from_name; +char *name; +char *print_name; +const char *comments; +unsigned is_unmerged:1 ; +unsigned is_binary:1 ; +unsigned is_renamed:1 ; +unsigned is_interesting:1 ; +uintmax_t added, deleted; + } **files; +}; +enum color_diff {DIFF_RESET =0 ,DIFF_CONTEXT =1 ,
@@ -327,6 +343,8 @@ void diff_change(struct diff_options *,struct diff_filepair *diff_unmerge (struct diff_options *,const char *path); +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat); +#define DIFF_SETUP_REVERSE 1#define DIFF_SETUP_USE_SIZE_CACHE 4
Signature · 1 line
gitgitgadget
PATCH 2/7 add--helper: create builtin helper for interactive add
In reply to Johannes SchindelinFrom: Daniel Ferreira <[email protected]>
Create a builtin helper for git-add--interactive, which right now is not able to do anything.
This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and performance (specially on non-POSIX systems like Windows).
Additionally, an eventual complete port of git-add--interactive would remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses.
--- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/add--helper.c | 6 ++++++ git.c | 1 + 5 files changed, 10 insertions(+) create mode 100644 builtin/add--helper.c
.gitignore +1 -0
@@ -15,6 +15,7 @@ /git /git-add /git-add--interactive +/git-add--helper /git-am /git-annotate /git-apply
Makefile +1 -0
@@ -1023,6 +1023,7 @@ LIB_OBJS += xdiff-interface.o LIB_OBJS += zlib.o BUILTIN_OBJS += builtin/add.o +BUILTIN_OBJS += builtin/add--helper.o BUILTIN_OBJS += builtin/am.o BUILTIN_OBJS += builtin/annotate.o BUILTIN_OBJS += builtin/apply.o
builtin.h +1 -0
@@ -128,6 +128,7 @@ extern void setup_auto_pager(const char *cmd, int def);extern int is_builtin (const char *s);extern int cmd_add (int argc,const char **argv,const char *prefix); +extern int cmd_add__helper (int argc,const char **argv,const char *prefix);extern int cmd_am (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix);
builtin/add--helper.c new file +6 -0
@@ -0,0 +1,6 @@ +#include "builtin.h" + +int cmd_add__helper (int argc,const char **argv,const char *prefix) +{ +return 0 ; +}
git.c +1 -0
@@ -443,6 +443,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },
Signature · 1 line
gitgitgadget
PATCH 4/7 add--interactive.perl: use add--helper --status for status_cmd
In reply to Johannes SchindelinFrom: Daniel Ferreira <[email protected]>
Call the newly introduced add--helper builtin on status_cmd() instead of relying on add--interactive's Perl functions to build print the numstat.
--- git-add--interactive.perl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
git-add--interactive.perl +1 -3
@@ -597,9 +597,7 @@ EOF } sub status_cmd { - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, - list_modified()); - print "\n"; + system(qw(git add--helper --status)); } sub say_n_paths {
Signature · 1 line
gitgitgadget
PATCH 3/7 add-interactive.c: implement status command
In reply to Johannes SchindelinFrom: Daniel Ferreira <[email protected]>
Add new files: add-interactive.c and add-interactive.h, which will be used for implementing "application logic" of git add -i, whereas add--helper.c will be used mostly for parsing the command line. We're a bit lax with the command-line parsing, as the command is intended to be called only by one internal user: the add--interactive script.
Implement add --interactive's status command in add-interactive.c and use it in builtin add--helper.c.
It prints a numstat comparing changed files between a) the worktree and the index; b) the index and the HEAD.
To do so, we use run_diff_index() and run_diff_files() to get changed files, use the diffstat API on them to get the numstat and use a combination of a hashmap and qsort() to print the result in O(n) + O(n lg n) complexity.
This is the first interactive add command implemented in C of those anticipated by the previous commit, which introduced
the add--helper built-in.
--- Makefile | 1 + add-interactive.c | 246 ++++++++++++++++++++++++++++++++++++++++++ add-interactive.h | 8 ++ builtin/add--helper.c | 32 ++++++ 4 files changed, 287 insertions(+) create mode 100644 add-interactive.c create mode 100644 add-interactive.h
Makefile +1 -0
@@ -827,6 +827,7 @@ LIB_H = $(shell $(FIND) . \ -name '*.h' -print) LIB_OBJS += abspath.o +LIB_OBJS += add-interactive.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o
add-interactive.c new file +246 -0
246 lines not on this page · raw message
add-interactive.h new file +8 -0
@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_print_modified (void ); + +#endif
builtin/add--helper.c +32 -0
@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_print_modified (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Signature · 1 line
gitgitgadget
PATCH 5/7 add-interactive.c: implement show-help command
In reply to Johannes SchindelinFrom: Slavica Djukic <[email protected]>
Implement show-help command in add-interactive.c and use it in builtin add--helper.c.
Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by handle_builtin and re-routed to the help command, without ever calling cmd_add__helper().
--- add-interactive.c | 19 +++++++++++++++++++ add-interactive.h | 2 ++ builtin/add--helper.c | 7 ++++++- 3 files changed, 27 insertions(+), 1 deletion(-)
add-interactive.c +19 -0
@@ -8,6 +8,16 @@#define HEADER_INDENT " " +/* TRANSLATORS: please do not translate the command names +'status', 'update', 'revert', etc. */ +static const char help_info[] = +N_ ("status - show paths with changes\n" +"update - add working tree state to the staged set of changes\n" +"revert - revert staged set of changes back to the HEAD version\n" +"patch - pick hunks and update selectively\n" +"diff - view diff between HEAD and index\n" +"add untracked - add contents of untracked files to the staged set of changes" ); +enum collection_phase {WORKTREE ,INDEX
@@ -244,3 +254,12 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +const char *modified_fmt =_ ("%s" ); +printf ("\n" ); +color_fprintf (stdout, help_color, modified_fmt,_ (help_info)); +printf ("\n" ); +}
add-interactive.h +2 -0
@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); +void show_help (void ); +#endif
builtin/add--helper.c +6 -1
@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)
@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };
@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +show_help ();else usage_with_options (builtin_add_helper_usage, options);
Signature · 1 line
gitgitgadget
PATCH 6/7 Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
In reply to Johannes SchindelinFrom: Slavica Djukic <[email protected]>
To enable testing the colored output on Windows, enable TTY by using environment variable GIT_TEST_PRETEND_TTY.
This is the original idea by Johannes Schindelin.
--- color.c | 4 ++++ perl/Git.pm | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)
color.c +4 -0
@@ -323,6 +323,10 @@ static int check_auto_color(int fd) {static int color_stderr_is_tty =-1 ;int *is_tty_p = fd ==1 ? &color_stdout_is_tty : &color_stderr_is_tty; + +if (git_env_bool ("GIT_TEST_PRETEND_TTY" ,0 )) +return 1 ; +if (*is_tty_p <0 ) *is_tty_p =isatty (fd);if (*is_tty_p || (fd ==1 &&pager_in_use () && pager_use_color)) {
perl/Git.pm +1 -1
@@ -757,7 +757,7 @@ and returns boolean (true for "use color", false for "do not use color"). sub get_colorbool { my ($self, $var) = @_; - my $stdout_to_tty = (-t STDOUT) ? "true" : "false"; + my $stdout_to_tty = $ENV{GIT_TEST_PRETEND_TTY} || (-t STDOUT) ? "true" : "false"; my $use_color = $self->command_oneline('config', '--get-colorbool', $var, $stdout_to_tty); return ($use_color eq 'true');
Signature · 1 line
gitgitgadget
PATCH 7/7 add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Johannes SchindelinFrom: Slavica Djukic <[email protected]>
Change help_cmd sub in git-add--interactive.perl to use show-help command from builtin add--helper.
Add test to t3701-add-interactive to verify that show-help outputs expected content. Use GIT_PRETENT_TTY introduced in earlier commit to be able to test output color on Windows.
--- git-add--interactive.perl | 11 +---------- t/t3701-add-interactive.sh | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 10 deletions(-)
git-add--interactive.perl +1 -10
@@ -1717,16 +1717,7 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes -update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + system(qw(git add--helper --show-help)); } sub process_args {
t/t3701-add-interactive.sh +25 -0
@@ -639,4 +639,29 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + <BOLD;RED>status - show paths with changes + update - add working tree state to the staged set of changes + revert - revert staged set of changes back to the HEAD version + patch - pick hunks and update selectively + diff - view diff between HEAD and index + add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_TEST_PRETEND_TTY=1 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
cover Turn git add-i into built-in
From: "Slavica Đukić via GitGitGadget" <[email protected]>
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#u
Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive add add-interactive.c: implement status command add--interactive.perl: use add--helper --status for status_cmd
Slavica Djukic (3):
add-interactive.c: implement show-help command Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY add--interactive.perl: use add--helper --show-help for help_cmd
.gitignore | 1 + Makefile | 2 + add-interactive.c | 265 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ color.c | 4 + diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 15 +-- git.c | 1 + perl/Git.pm | 2 +- t/t3701-add-interactive.sh | 25 ++++ 13 files changed, 387 insertions(+), 36 deletions(-) create mode 100644 add-interactive.c create mode 100644 add-interactive.h create mode 100644 builtin/add--helper.c
base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-103%2FslavicaDj%2Fadd-i-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/103
Signature · 1 line
gitgitgadget
Hi,
just a note: this mail had not been sent by GitGitGadget (and hence has me as sender, even if the email address is GitGitGadget's) because it still has problems with From: and To: and Cc: headers that contain non-ASCII characters. I hope to find the time soon to work on this.
Ciao,
Johannes
Quoted 44 lines · Johannes Schindelin
From: "Slavica Đukić via GitGitGadget" <[email protected]>
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#uDaniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive addadd-interactive.c: implement status command add--interactive.perl: use add--helper --status forstatus_cmdSlavica Djukic (3):
add-interactive.c: implement show-help command Git.pm: introduce environment variableGIT_TEST_PRETEND_TTYadd--interactive.perl: use add--helper --show-help forhelp_cmd.gitignore | 1 + Makefile | 2 + add-interactive.c | 265 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ color.c | 4 + diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 15 +-- git.c | 1 + perl/Git.pm | 2 +- t/t3701-add-interactive.sh | 25 ++++ 13 files changed, 387 insertions(+), 36 deletions(-) create mode 100644 add-interactive.c create mode 100644 add-interactive.h create mode 100644 builtin/add--helper.cbase-commit:
b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-103%2FslavicaDj%2Fadd-i-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/103Signature · 1 line
gitgitgadget
Quoted 6 lines · Johannes Schindelin
From: "Slavica Đukić via GitGitGadget" <[email protected]>
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#u
Ping? :)
Quoted 35 lines
Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive addadd-interactive.c: implement status command add--interactive.perl: use add--helper --status forstatus_cmdSlavica Djukic (3):
add-interactive.c: implement show-help command Git.pm: introduce environment variableGIT_TEST_PRETEND_TTYadd--interactive.perl: use add--helper --show-help forhelp_cmd.gitignore | 1 + Makefile | 2 + add-interactive.c | 265 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ color.c | 4 + diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 15 +-- git.c | 1 + perl/Git.pm | 2 +- t/t3701-add-interactive.sh | 25 ++++ 13 files changed, 387 insertions(+), 36 deletions(-)create mode 100644
add-interactive.c
create mode 100644add-interactive.h
create mode 100644builtin/add--helper.cbase-commit:
b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-103%2FslavicaDj%2Fadd-i-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/103
Re: [PATCH 5/7] add-interactive.c: implement show-help command
In reply to Slavica Djukic via GitGitGadgetHi Slavica
I think the basics of this patch are fine, I've got a few comments below
Quoted 24 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Implement show-help command in
add-interactive.cand use it in builtinadd--helper.c.Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by
handle_builtinand re-routed to the help command, without ever callingcmd_add__helper().Signed-off-by Slavica Djukic <[email protected]>--- add-interactive.c | 19 +++++++++++++++++++ add-interactive.h | 2 ++ builtin/add--helper.c | 7 ++++++- 3 files changed, 27 insertions(+), 1 deletion(-)add-interactive.c +0
-0@@ -8,6 +8,16 @@#define HEADER_INDENT " "
add-interactive.c line 19
Quoted 10 lines
add-interactive.c +9
-0+/* TRANSLATORS: please do not translate the command names +'status', 'update', 'revert', etc. */ +static const char help_info[] = +N_ ("status - show paths with changes\n" +"update - add working tree state to the staged set of changes\n" +"revert - revert staged set of changes back to the HEAD version\n" +"patch - pick hunks and update selectively\n" +"diff - view diff between HEAD and index\n" +"add untracked - add contents of untracked files to the staged set of changes" );
If we were starting from scratch I'd suggest only translating the help text, not the command names as this would avoid any possible problems with the command names and indentation. As we already have this translated it is easier to leave it as is unless the translators think it would be useful to change it. I'd be inclined to put this definition next to the function that uses it (or possibly in that function) so it is easy to see what text will be printed when reading the function.
add-interactive.c void add_i_print_modified(void) · line 261
Quoted 13 lines
add-interactive.c +6
-0+enum collection_phase {WORKTREE ,INDEX @@ -244,3 +254,12 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +const char *modified_fmt =_ ("%s" );
This does not need to be translated, also I'm not sure we need a separate variable (unless something funny is happening - why has it got 'modified' in its name?)
add-interactive.h int add_i_config(const char *var, const char *value, void *cbdata); · line 8
Quoted 13 lines
add-interactive.c +4
-0+printf ("\n" ); +color_fprintf (stdout, help_color, modified_fmt,_ (help_info)); +printf ("\n" ); +}add-interactive.h +1
-0@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); +void show_help (void );
As this is a global function I think it needs a more specific name to avoid clashing with a function that shows the help for a different command. Maybe add_i_show_help() to match add_i_print_modified()?
add-interactive.h int add_i_config(const char *var, const char *value, void *cbdata); · line 10
Quoted 3 lines
add-interactive.h +1
-0+#endif
C files should end with a new line (it is actually undefined behavior if they don't!)
Best Wishes
Phillip
Quoted 33 lines
builtin/add--helper.c +6
-1@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +show_help ();else usage_with_options (builtin_add_helper_usage, options);
Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
In reply to Slavica Djukic via GitGitGadgetHi Salvica/Johannes
Quoted 6 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
To enable testing the colored output on Windows, enable TTY by using environment variable
GIT_TEST_PRETEND_TTY.This is the original idea by Johannes Schindelin.
I normally use GIT_PAGER_IN_USE=1 to force colored output, is there some reason that does not work here?
Best Wishes
Phillip
Quoted 36 lines
Signed-off-by Slavica Djukic <[email protected]>--- color.c | 4 ++++ perl/Git.pm | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)color.c +4
-0@@ -323,6 +323,10 @@ static int check_auto_color(int fd) {static int color_stderr_is_tty =-1 ;int *is_tty_p = fd ==1 ? &color_stdout_is_tty : &color_stderr_is_tty; + +if (git_env_bool ("GIT_TEST_PRETEND_TTY" ,0 )) +return 1 ; +if (*is_tty_p <0 ) *is_tty_p =isatty (fd);if (*is_tty_p || (fd ==1 &&pager_in_use () && pager_use_color)) {perl/Git.pm +1
-1@@ -757,7 +757,7 @@ and returns boolean (true for "use color", false for "do not use color"). sub get_colorbool { my ($self, $var) = @_; - my $stdout_to_tty = (-t STDOUT) ? "true" : "false"; + my $stdout_to_tty = $ENV{GIT_TEST_PRETEND_TTY} || (-t STDOUT) ? "true" : "false"; my $use_color = $self->command_oneline('config', '--get-colorbool', $var, $stdout_to_tty); return ($use_color eq 'true');
Re: [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Slavica Djukic via GitGitGadgetHi Slavica
Quoted 10 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Change
help_cmdsub in git-add--interactive.perl to use show-help command from builtin add--helper.Add test to t3701-add-interactive to verify that show-help outputs expected content. Use
GIT_PRETENT_TTYintroduced in earlier commit to be able to test output color on Windows.
It's great to see you adding a test for this, if it isn't too much work perhaps you could add it before changing the implementation to demonstrate that there are no changes introduced by the conversion to C.
Best Wishes
Phillip
Quoted 63 lines
Signed-off-by Slavica Djukic <[email protected]>--- git-add--interactive.perl | 11 +---------- t/t3701-add-interactive.sh | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 10 deletions(-)git-add--interactive.perl +1
-10@@ -1717,16 +1717,7 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes -update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + system(qw(git add--helper --show-help)); } sub process_args {t/t3701-add-interactive.sh +25
-0@@ -639,4 +639,29 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + <BOLD;RED>status - show paths with changes + update - add working tree state to the staged set of changes + revert - revert staged set of changes back to the HEAD version + patch - pick hunks and update selectively + diff - view diff between HEAD and index + add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_TEST_PRETEND_TTY=1 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
In reply to Phillip WoodHi Phillip,
First of all, thank you for taking your time to review this patch series.
Quoted 11 lines · Phillip Wood
Hi Salvica/Johannes
Quoted 6 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
To enable testing the colored output on Windows, enable TTY by using environment variable
GIT_TEST_PRETEND_TTY.This is the original idea by Johannes Schindelin.
I normally use GIT_PAGER_IN_USE=1 to force colored output, is there some reason that does not work here?
I tried that approach, and it turns out GIT_PAGER_IN_USE=1 does force colored output.
As for the changes suggested in the other two e-mails -- I will apply them as well and send re-roll.
Thank you,
Slavica
Quoted 40 lines
Best Wishes
Phillip
Quoted 35 lines
Signed-off-by Slavica Djukic <[email protected]>--- color.c | 4 ++++ perl/Git.pm | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)color.c +4
-0@@ -323,6 +323,10 @@ static int check_auto_color(int fd) {static int color_stderr_is_tty =-1 ;int *is_tty_p = fd ==1 ? &color_stdout_is_tty : &color_stderr_is_tty; + +if (git_env_bool ("GIT_TEST_PRETEND_TTY" ,0 )) +return 1 ; +if (*is_tty_p <0 ) *is_tty_p =isatty (fd);if (*is_tty_p || (fd ==1 &&pager_in_use () && pager_use_color)) {perl/Git.pm +1
-1@@ -757,7 +757,7 @@ and returns boolean (true for "use color", false for "do not use color"). sub get_colorbool { my ($self, $var) = @_; - my $stdout_to_tty = (-t STDOUT) ? "true" : "false"; + my $stdout_to_tty = $ENV{GIT_TEST_PRETEND_TTY} || (-t STDOUT) ? "true" : "false"; my $use_color = $self->command_oneline('config', '--get-colorbool', $var, $stdout_to_tty); return ($use_color eq 'true');
Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
In reply to Phillip WoodHi Phillip,
Quoted 1 line · Phillip Wood
Hi Salvica/Johannes
Close ;-)
Quoted 10 lines · Slavica Djukic via GitGitGadget
Quoted 6 lines
From: Slavica Djukic <[email protected]>
To enable testing the colored output on Windows, enable TTY by using environment variable
GIT_TEST_PRETEND_TTY.This is the original idea by Johannes Schindelin.
I normally use GIT_PAGER_IN_USE=1 to force colored output, is there some reason that does not work here?
As Slavica found out, you need to set TERM, too, for that to work. And even then, it is more of a "happens to do what we want" rather than a "this is what we want to happen"...
But I'm fine either way, the color code does not really *need* more complexity, as it were...
Ciao,
Dscho
Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
Quoted 7 lines · Johannes Schindelin
Hi Phillip,
Quoted 1 line · Phillip Wood
Hi Salvica/Johannes
Close ;-)
Sorry Slavica
Quoted 12 lines
Quoted 11 lines · Slavica Djukic via GitGitGadget
Quoted 6 lines
From: Slavica Djukic <[email protected]>
To enable testing the colored output on Windows, enable TTY by using environment variable
GIT_TEST_PRETEND_TTY.This is the original idea by Johannes Schindelin.
I normally use GIT_PAGER_IN_USE=1 to force colored output, is there some reason that does not work here?
Quoted 6 lines
As Slavica found out, you need to set
TERM, too, for that to work. And even then, it is more of a "happens to do what we want" rather than a "this is what we want to happen"...But I'm fine either way, the color code does not really *need* more complexity, as it were...
Thanks for explaining, I didn't know the TERM needed to be set as well. If GIT_PAGER_IN_USE=1 works then maybe it is better to avoid adding any more complexity.
Best Wishes
Phillip
Quoted 4 lines
Ciao,
Dscho
v2 cover Turn git add-i into built-in
In reply to Johannes SchindelinThis is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#u
Changes since v1:
* rename show_help() to add_i_show_help()
* move static const char help_info[] to add_i_show_help() and translate only the help text
* add newline at the end of add-interactive.h
* modify commits so that the test is introduced before making git-add--interactive.perl's help_cmd to use add_i_show_help
* use variables GIT_PAGER_IN_USE=true and TERM=vt100 in test as alternative for GIT_PRETEND_TTY
Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive add add-interactive.c: implement status command add--interactive.perl: use add--helper --status for status_cmd
Slavica Djukic (3):
add-interactive.c: implement show-help command
t3701-add-interactive: test add_i_show_help() add--interactive.perl: use add--helper --show-help for help_cmd
.gitignore | 1 + Makefile | 2 + add-interactive.c | 269 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 15 +-- git.c | 1 + t/t3701-add-interactive.sh | 24 ++++ 11 files changed, 385 insertions(+), 35 deletions(-) create mode 100644 add-interactive.c create mode 100644 add-interactive.h create mode 100644 builtin/add--helper.c
base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/103
Range-diff vs v1:
1: 737767b6f4 = 1: 737767b6f4 diff: export diffstat interface
2: 91b1963125 = 2: 91b1963125 add--helper: create builtin helper for interactive add
3: d247ef69fe = 3: d247ef69fe add-interactive.c: implement status command
4: 4950c889aa = 4: 4950c889aa add--interactive.perl: use add--helper --status for status_cmd
5: 19fdea5db1 ! 5: cf4e913a5a add-interactive.c: implement show-help command
@@ -16,33 +16,30 @@
--- a/add-interactive.c
+++ b/add-interactive.c
@@
-
- #define HEADER_INDENT " "
-
-+/* TRANSLATORS: please do not translate the command names
-+ 'status', 'update', 'revert', etc. */
-+static const char help_info[] =
-+ N_("status - show paths with changes\n"
-+ "update - add working tree state to the staged set of changes\n"
-+ "revert - revert staged set of changes back to the HEAD version\n"
-+ "patch - pick hunks and update selectively\n"
-+ "diff - view diff between HEAD and index\n"
-+ "add untracked - add contents of untracked files to the staged set of changes");
-+
- enum collection_phase {
- WORKTREE,
- INDEX
-@@
free(files);
hashmap_free(&s.file_map, 1);
}
+
-+void show_help(void)
++void add_i_show_help(void)
+{
+ const char *help_color = get_color(COLOR_HELP);
-+ const char *modified_fmt = _("%s");
++ color_fprintf(stdout, help_color, "%s%s", _("status"),
++ N_(" - show paths with changes"));
++ printf("\n");
++ color_fprintf(stdout, help_color, "%s%s", _("update"),
++ N_(" - add working tree state to the staged set of changes"));
++ printf("\n");
++ color_fprintf(stdout, help_color, "%s%s", _("revert"),
++ N_(" - revert staged set of changes back to the HEAD version"));
+ printf("\n");
-+ color_fprintf(stdout, help_color, modified_fmt, _(help_info));
++ color_fprintf(stdout, help_color, "%s%s", _("patch"),
++ N_(" - pick hunks and update selectively"));
++ printf("\n");
++ color_fprintf(stdout, help_color, "%s%s", _("diff"),
++ N_(" - view diff between HEAD and index"));
++ printf("\n");
++ color_fprintf(stdout, help_color, "%s%s", _("add untracked"),
++ N_(" - add contents of untracked files to the staged set of changes"));
+ printf("\n");
+}
@@ -53,10 +50,11 @@
void add_i_print_modified(void);
-+void show_help(void);
-+
- #endif
+-#endif
\ No newline at end of file
++void add_i_show_help(void);
++
++#endif
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
--- a/builtin/add--helper.c
@@ -85,7 +83,7 @@
if (mode == STATUS)
add_i_print_modified();
+ else if (mode == HELP)
-+ show_help();
++ add_i_show_help();
else
usage_with_options(builtin_add_helper_usage,
options);
6: 86d85face8 < -: ---------- Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
-: ---------- > 6: 2b4714b8d0 t3701-add-interactive: test add_i_show_help()
7: 060806010e ! 7: 6ede6d9251 add--interactive.perl: use add--helper --show-help for help_cmd
@@ -5,11 +5,6 @@
Change help_cmd sub in git-add--interactive.perl to use
show-help command from builtin add--helper.
- Add test to t3701-add-interactive to verify that show-help
- outputs expected content. Use GIT_PRETENT_TTY
- introduced in earlier commit to be able to test output color
- on Windows.
-
Signed-off-by: Slavica Djukic <[email protected]>
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
@@ -33,37 +28,3 @@
}
sub process_args {
-
- diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
- --- a/t/t3701-add-interactive.sh
- +++ b/t/t3701-add-interactive.sh
-@@
- test_cmp expected-2 actual
- '
-
-+test_expect_success 'show help from add--helper' '
-+ git reset --hard &&
-+ cat >expect <<-\EOF &&
-+
-+ <BOLD>*** Commands ***<RESET>
-+ 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked
-+ 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp
-+ <BOLD;BLUE>What now<RESET>>
-+ <BOLD;RED>status - show paths with changes
-+ update - add working tree state to the staged set of changes
-+ revert - revert staged set of changes back to the HEAD version
-+ patch - pick hunks and update selectively
-+ diff - view diff between HEAD and index
-+ add untracked - add contents of untracked files to the staged set of changes<RESET>
-+ <BOLD>*** Commands ***<RESET>
-+ 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked
-+ 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp
-+ <BOLD;BLUE>What now<RESET>>
-+ Bye.
-+ EOF
-+ test_write_lines h | GIT_TEST_PRETEND_TTY=1 git add -i >actual.colored &&
-+ test_decode_color <actual.colored >actual &&
-+ test_i18ncmp expect actual
-+'
-+
- test_doneSignature · 1 line
gitgitgadget
PATCH v2 1/7 diff: export diffstat interface
From: Daniel Ferreira <[email protected]>
Make the diffstat interface (namely, the diffstat_t struct and compute_diffstat) no longer be internal to diff.c and allow it to be used by other parts of git.
This is helpful for code that may want to easily extract information from files using the diff machinery, while flushing it differently from how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status.
--- diff.c | 36 ++++++++++++++---------------------- diff.h | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 22 deletions(-)
diff.c +14 -22
@@ -2421,22 +2421,6 @@ static void pprint_rename(struct strbuf *name, const char *a, const char *b) } } -struct diffstat_t { -int nr; -int alloc; -struct diffstat_file { -char *from_name; -char *name; -char *print_name; -const char *comments; -unsigned is_unmerged:1 ; -unsigned is_binary:1 ; -unsigned is_renamed:1 ; -unsigned is_interesting:1 ; -uintmax_t added, deleted; - } **files; -}; -static struct diffstat_file *diffstat_add (struct diffstat_t *diffstat,const char *name_a,const char *name_b)
@@ -5922,12 +5906,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat; -memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )
@@ -6227,6 +6206,19 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)return ignored; } +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff; + +memset (diffstat,0 ,sizeof (struct diffstat_t )); +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +if (check_pair_status (p)) +diff_flush_stat (p, options, diffstat); + } +} + void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid,
diff.h +18 -0
@@ -239,6 +239,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);void diff_emit_submodule_pipethrough (struct diff_options *o,const char *line,int len); +struct diffstat_t { +int nr; +int alloc; +struct diffstat_file { +char *from_name; +char *name; +char *print_name; +const char *comments; +unsigned is_unmerged:1 ; +unsigned is_binary:1 ; +unsigned is_renamed:1 ; +unsigned is_interesting:1 ; +uintmax_t added, deleted; + } **files; +}; +enum color_diff {DIFF_RESET =0 ,DIFF_CONTEXT =1 ,
@@ -327,6 +343,8 @@ void diff_change(struct diff_options *,struct diff_filepair *diff_unmerge (struct diff_options *,const char *path); +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat); +#define DIFF_SETUP_REVERSE 1#define DIFF_SETUP_USE_SIZE_CACHE 4
Signature · 1 line
gitgitgadget
PATCH v2 2/7 add--helper: create builtin helper for interactive add
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Create a builtin helper for git-add--interactive, which right now is not able to do anything.
This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and performance (specially on non-POSIX systems like Windows).
Additionally, an eventual complete port of git-add--interactive would remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses.
--- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/add--helper.c | 6 ++++++ git.c | 1 + 5 files changed, 10 insertions(+) create mode 100644 builtin/add--helper.c
.gitignore +1 -0
@@ -15,6 +15,7 @@ /git /git-add /git-add--interactive +/git-add--helper /git-am /git-annotate /git-apply
Makefile +1 -0
@@ -1023,6 +1023,7 @@ LIB_OBJS += xdiff-interface.o LIB_OBJS += zlib.o BUILTIN_OBJS += builtin/add.o +BUILTIN_OBJS += builtin/add--helper.o BUILTIN_OBJS += builtin/am.o BUILTIN_OBJS += builtin/annotate.o BUILTIN_OBJS += builtin/apply.o
builtin.h +1 -0
@@ -128,6 +128,7 @@ extern void setup_auto_pager(const char *cmd, int def);extern int is_builtin (const char *s);extern int cmd_add (int argc,const char **argv,const char *prefix); +extern int cmd_add__helper (int argc,const char **argv,const char *prefix);extern int cmd_am (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix);
builtin/add--helper.c new file +6 -0
@@ -0,0 +1,6 @@ +#include "builtin.h" + +int cmd_add__helper (int argc,const char **argv,const char *prefix) +{ +return 0 ; +}
git.c +1 -0
@@ -443,6 +443,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },
Signature · 1 line
gitgitgadget
PATCH v2 4/7 add--interactive.perl: use add--helper --status for status_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Call the newly introduced add--helper builtin on status_cmd() instead of relying on add--interactive's Perl functions to build print the numstat.
--- git-add--interactive.perl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
git-add--interactive.perl +1 -3
@@ -597,9 +597,7 @@ sub prompt_help_cmd { } sub status_cmd { - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, - list_modified()); - print "\n"; + system(qw(git add--helper --status)); } sub say_n_paths {
Signature · 1 line
gitgitgadget
PATCH v2 5/7 add-interactive.c: implement show-help command
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Implement show-help command in add-interactive.c and use it in builtin add--helper.c.
Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by handle_builtin and re-routed to the help command, without ever calling cmd_add__helper().
--- add-interactive.c | 23 +++++++++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 32 insertions(+), 2 deletions(-)
add-interactive.c +23 -0
@@ -244,3 +244,26 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("status" ), +N_ (" - show paths with changes" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("update" ), +N_ (" - add working tree state to the staged set of changes" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("revert" ), +N_ (" - revert staged set of changes back to the HEAD version" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("patch" ), +N_ (" - pick hunks and update selectively" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("diff" ), +N_ (" - view diff between HEAD and index" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("add untracked" ), +N_ (" - add contents of untracked files to the staged set of changes" )); +printf ("\n" ); +}
add-interactive.h +3 -1
@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif
builtin/add--helper.c +6 -1
@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)
@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };
@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Signature · 1 line
gitgitgadget
PATCH v2 3/7 add-interactive.c: implement status command
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Add new files: add-interactive.c and add-interactive.h, which will be used for implementing "application logic" of git add -i, whereas add--helper.c will be used mostly for parsing the command line. We're a bit lax with the command-line parsing, as the command is intended to be called only by one internal user: the add--interactive script.
Implement add --interactive's status command in add-interactive.c and use it in builtin add--helper.c.
It prints a numstat comparing changed files between a) the worktree and the index; b) the index and the HEAD.
To do so, we use run_diff_index() and run_diff_files() to get changed files, use the diffstat API on them to get the numstat and use a combination of a hashmap and qsort() to print the result in O(n) + O(n lg n) complexity.
This is the first interactive add command implemented in C of those anticipated by the previous commit, which introduced
the add--helper built-in.
--- Makefile | 1 + add-interactive.c | 246 ++++++++++++++++++++++++++++++++++++++++++ add-interactive.h | 8 ++ builtin/add--helper.c | 32 ++++++ 4 files changed, 287 insertions(+) create mode 100644 add-interactive.c create mode 100644 add-interactive.h
Makefile +1 -0
@@ -827,6 +827,7 @@ LIB_H = $(shell $(FIND) . \ -name '*.h' -print) LIB_OBJS += abspath.o +LIB_OBJS += add-interactive.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o
add-interactive.c new file +246 -0
246 lines not on this page · raw message
add-interactive.h new file +8 -0
@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_print_modified (void ); + +#endif
builtin/add--helper.c +32 -0
@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_print_modified (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Signature · 1 line
gitgitgadget
PATCH v2 7/7 add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Change help_cmd sub in git-add--interactive.perl to use show-help command from builtin add--helper.
--- git-add--interactive.perl | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
git-add--interactive.perl +1 -10
@@ -1717,16 +1717,7 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes -update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + system(qw(git add--helper --show-help)); } sub process_args {
Signature · 1 line
gitgitgadget
PATCH v2 6/7 t3701-add-interactive: test add_i_show_help()
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that add_i_show_help() outputs expected content.
Also, add it before changing git-add--interactive.perl's help_cmd to demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.
--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
t/t3701-add-interactive.sh +24 -0
@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
In reply to Slavica Djukic via GitGitGadgetHi Slavica
I think this round is looking good I've got a couple of comments about the translation of the help text but everything else looks fine to me now. In future when you're posting a new version it's helpful CC the people who commented on the previous version(s).
Quoted 22 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Implement show-help command in
add-interactive.cand use it in builtinadd--helper.c.Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by
handle_builtinand re-routed to the help command, without ever callingcmd_add__helper().Signed-off-by Slavica Djukic <[email protected]>--- add-interactive.c | 23 +++++++++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 32 insertions(+), 2 deletions(-)add-interactive.c +0
-0
add-interactive.c void add_i_print_modified(void) · line 253
Quoted 10 lines
add-interactive.c +7
-0@@ -244,3 +244,26 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("status" ), +N_ (" - show paths with changes" )); +printf ("\n" );
There seems to be a bit of confusion with the translation of these messages. "status" does not want to be translated so it shouldn't be in _() - it can just go in the format string as can the indentation and the "\n" (or we could use color_fprintf_ln() to automatically add a newline at the end. N_() is used to mark static strings for translation so the gettext utilities pick up the text to be translated but (because initializes for static variables must be compile-time constants) does not do anything when the program runs - if you have 'const char *s = N_(hello);' you have to do '_(s)' to get the translated version. Here we can just pass the untranslated string directly to gettext so it should be _("show paths with changes"). Putting all that together we get
color_fprintf(stdout, help_color, "status - %s\n",
_("show paths with changes");Best Wishes
Phillip
Quoted 62 lines
add-interactive.c +16
-0+color_fprintf (stdout, help_color,"%s%s" ,_ ("update" ), +N_ (" - add working tree state to the staged set of changes" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("revert" ), +N_ (" - revert staged set of changes back to the HEAD version" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("patch" ), +N_ (" - pick hunks and update selectively" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("diff" ), +N_ (" - view diff between HEAD and index" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("add untracked" ), +N_ (" - add contents of untracked files to the staged set of changes" )); +printf ("\n" ); +}add-interactive.h +3
-1@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif builtin/add--helper.c +6
-1@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Re: [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help()
In reply to Slavica Djukic via GitGitGadgetHi Slavica
Thanks for moving this up, it's really good to add the test before the conversion to C.
Best Wishes
Phillip
Quoted 49 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that
add_i_show_help()outputs expected content.
Also, add it before changing git-add--interactive.perl'shelp_cmdto demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.Signed-off-by Slavica Djukic <[email protected]>--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)t/t3701-add-interactive.sh +24
-0@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
In reply to Phillip WoodHi Phillip,
Quoted 6 lines · Phillip Wood
Hi Slavica
I think this round is looking good I've got a couple of comments about the translation of the help text but everything else looks fine to me now. In future when you're posting a new version it's helpful CC the people who commented on the previous version(s).
Thanks for taking your time to review patches again. I'm sorry for omitting you
in CC, but I've sent re-roll through GitGitGadget, and I guess I thought it would pick it up.
I'll see what happened and keep that in mind.
Quoted 46 lines · Slavica Djukic via GitGitGadget
Quoted 22 lines
From: Slavica Djukic <[email protected]>
Implement show-help command in
add-interactive.cand use it in builtinadd--helper.c.Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by
handle_builtinand re-routed to the help command, without ever callingcmd_add__helper().Signed-off-by Slavica Djukic <[email protected]>--- add-interactive.c | 23 +++++++++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 32 insertions(+), 2 deletions(-)add-interactive.c +0
-0add-interactive.c void add_i_print_modified(void) · line 253
Quoted 10 lines
add-interactive.c +7
-0@@ -244,3 +244,26 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("status" ), +N_ (" - show paths with changes" )); +printf ("\n" );There seems to be a bit of confusion with the translation of these messages. "status" does not want to be translated so it shouldn't be in
_()- it can just go in the format string as can the indentation and the "\n" (or we could usecolor_fprintf_ln()to automatically add a newline at the end.N_()is used to mark static strings for translation so the gettext utilities pick up the text to be translated but (because initializes for static variables must be compile-time constants) does not do anything when the program runs - if you have 'const char *s =N_(hello);' you have to do '_(s)' to get the translated version. Here we can just pass the untranslated string directly to gettext so it should be_("show paths with changes"). Putting all that together we get
Quoted 2 lines
color_fprintf(stdout, help_color, "status - %s\n",
_("show paths with changes");I thought _() was for strings that were already translated,
and N_() for strings that weren't. And I now see that I also tried to translate command
names as well, just the opposite of what you suggested... Thanks for clarifying this.
Quoted 68 lines
Best Wishes
Phillip
Quoted 62 lines
add-interactive.c +16
-0+color_fprintf (stdout, help_color,"%s%s" ,_ ("update" ), +N_ (" - add working tree state to the staged set of changes" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("revert" ), +N_ (" - revert staged set of changes back to the HEAD version" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("patch" ), +N_ (" - pick hunks and update selectively" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("diff" ), +N_ (" - view diff between HEAD and index" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("add untracked" ), +N_ (" - add contents of untracked files to the staged set of changes" )); +printf ("\n" ); +}add-interactive.h +3
-1@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif builtin/add--helper.c +6
-1@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
In reply to a message not in the archiveHi Slavica
Quoted 11 lines · Slavica Đukić
Hi Phillip,
Quoted 6 lines · Phillip Wood
Hi Slavica
I think this round is looking good I've got a couple of comments about the translation of the help text but everything else looks fine to me now. In future when you're posting a new version it's helpful CC the people who commented on the previous version(s).
Quoted 7 lines
Thanks for taking your time to review patches again. I'm sorry for omitting you
in CC, but I've sent re-roll through GitGitGadget, and I guess I thought it would pick it up.
I'll see what happened and keep that in mind.
I'm not sure what GitGitGadget does about CC'ing people but Johannes will know
Quoted 45 lines
Quoted 45 lines · Slavica Djukic via GitGitGadget
Quoted 22 lines
From: Slavica Djukic <[email protected]>
Implement show-help command in
add-interactive.cand use it in builtinadd--helper.c.Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by
handle_builtinand re-routed to the help command, without ever callingcmd_add__helper().Signed-off-by Slavica Djukic <[email protected]>--- add-interactive.c | 23 +++++++++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 32 insertions(+), 2 deletions(-)add-interactive.c +0
-0add-interactive.c void add_i_print_modified(void) · line 253
Quoted 10 lines
add-interactive.c +7
-0@@ -244,3 +244,26 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("status" ), +N_ (" - show paths with changes" )); +printf ("\n" );There seems to be a bit of confusion with the translation of these messages. "status" does not want to be translated so it shouldn't be in
_()- it can just go in the format string as can the indentation and the "\n" (or we could usecolor_fprintf_ln()to automatically add a newline at the end.N_()is used to mark static strings for translation so the gettext utilities pick up the text to be translated but (because initializes for static variables must be compile-time constants) does not do anything when the program runs - if you have 'const char *s =N_(hello);' you have to do '_(s)' to get the translated version. Here we can just pass the untranslated string directly to gettext so it should be_("show paths with changes"). Putting all that together we get
Quoted 8 lines
Quoted 2 lines
color_fprintf(stdout, help_color, "status - %s\n", _("show paths with changes");I thought
_()was for strings that were already translated,
andN_()for strings that weren't. And I now see that I also tried to translate command names as well, just the opposite of what you suggested... Thanks for clarifying this.
I hope my explanation made sense, feel free to email if you want to check anything.
Having thought about it, I don't think we should add "\n" to the format string as it means the color will be reset after the new line, it should use color_fprintf_ln() instead which adds a new line after it has reset the color.
Best Wishes
Phillip
Quoted 66 lines
Quoted 66 lines
Best Wishes
Phillip
Quoted 62 lines
add-interactive.c +16
-0+color_fprintf (stdout, help_color,"%s%s" ,_ ("update" ), +N_ (" - add working tree state to the staged set of changes" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("revert" ), +N_ (" - revert staged set of changes back to the HEAD version" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("patch" ), +N_ (" - pick hunks and update selectively" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("diff" ), +N_ (" - view diff between HEAD and index" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("add untracked" ), +N_ (" - add contents of untracked files to the staged set of changes" )); +printf ("\n" ); +}add-interactive.h +3
-1@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif builtin/add--helper.c +6
-1@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
Hi Phillip,
Quoted 13 lines · Phillip Wood
Quoted 12 lines · Slavica Đukić
Quoted 5 lines · Phillip Wood
I think this round is looking good I've got a couple of comments about the translation of the help text but everything else looks fine to me now. In future when you're posting a new version it's helpful CC the people who commented on the previous version(s).
Thanks for taking your time to review patches again. I'm sorry for omitting you
Quoted 7 lines
Quoted 4 lines
in CC, but I've sent re-roll through GitGitGadget, and I guess I thought it would pick it up.
I'll see what happened and keep that in mind.
I'm not sure what GitGitGadget does about CC'ing people but Johannes will know
The idea is to have a footer (read: last line) of the PR description of the form:
i.e. a comma-separated list of recipients to Cc.
Yes, it is under-documented, but I still need to implement more features before I can start to polish documentation.
Ciao,
Dscho
Quoted 142 lines
Quoted 46 lines
Quoted 46 lines · Slavica Djukic via GitGitGadget
Quoted 22 lines
From: Slavica Djukic <[email protected]>
Implement show-help command in
add-interactive.cand use it in builtinadd--helper.c.Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by
handle_builtinand re-routed to the help command, without ever callingcmd_add__helper().Signed-off-by Slavica Djukic <[email protected]>--- add-interactive.c | 23 +++++++++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 32 insertions(+), 2 deletions(-)add-interactive.c +0
-0add-interactive.c void add_i_print_modified(void) · line 253
Quoted 10 lines
add-interactive.c +7
-0@@ -244,3 +244,26 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("status" ), +N_ (" - show paths with changes" )); +printf ("\n" );There seems to be a bit of confusion with the translation of these messages. "status" does not want to be translated so it shouldn't be in
_()- it can just go in the format string as can the indentation and the "\n" (or we could usecolor_fprintf_ln()to automatically add a newline at the end.N_()is used to mark static strings for translation so the gettext utilities pick up the text to be translated but (because initializes for static variables must be compile-time constants) does not do anything when the program runs - if you have 'const char *s =N_(hello);' you have to do '_(s)' to get the translated version. Here we can just pass the untranslated string directly to gettext so it should be_("show paths with changes"). Putting all that together we getQuoted 8 lines
Quoted 2 lines
color_fprintf(stdout,
help_color, "status - %s\n",
_("show paths with changes");I thought
_()was for strings that were already translated,
andN_()for strings that weren't. And I now see that I also tried to translate command names as well, just the opposite of what you suggested... Thanks for clarifying this.I hope my explanation made sense, feel free to email if you want to check anything.
Having thought about it, I don't think we should add "\n" to the format string as it means the color will be reset after the new line, it should use
color_fprintf_ln()instead which adds a new line after it has reset the color.Best Wishes
Phillip
Quoted 73 lines
Quoted 73 lines
Best Wishes
Phillip
Quoted 69 lines
add-interactive.c +2
-0+ color_fprintf(stdout, help_color, "%s%s", _("update"), + N_(" - add working tree state to the staged set ofchanges"));
add-interactive.c +3
-0+printf ("\n" ); + color_fprintf(stdout, help_color, "%s%s", _("revert"), + N_(" - revert staged set of changes back to the HEADversion"));
add-interactive.c +9
-0+printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("patch" ), +N_ (" - pick hunks and update selectively" )); +printf ("\n" ); +color_fprintf (stdout, help_color,"%s%s" ,_ ("diff" ), +N_ (" - view diff between HEAD and index" )); +printf ("\n" ); + color_fprintf(stdout, help_color, "%s%s", _("add untracked"), + N_(" - add contents of untracked files to the staged set ofchanges"));
add-interactive.c +2
-0+printf ("\n" ); +}add-interactive.h +3
-1@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif builtin/add--helper.c +2
-1@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int
cmd_add__helper(int argc, const char **argv, const char *prefix)builtin/add--helper.c +2
-0@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END ()};@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv,
const char *prefix)if (mode == STATUS)
add_i_print_modified();builtin/add--helper.c +2
-0+else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
v3 cover Turn git add-i into built-in
In reply to Slavica Đukić via GitGitGadgetThis is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#u
Changes since v2:
* fixed translation misuse and avoided command names translation in add_i_show_help
Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive add add-interactive.c: implement status command add--interactive.perl: use add--helper --status for status_cmd
Slavica Djukic (3):
add-interactive.c: implement show-help command
t3701-add-interactive: test add_i_show_help() add--interactive.perl: use add--helper --show-help for help_cmd
.gitignore | 1 + Makefile | 2 + add-interactive.c | 263 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 15 +-- git.c | 1 + t/t3701-add-interactive.sh | 24 ++++ 11 files changed, 379 insertions(+), 35 deletions(-) create mode 100644 add-interactive.c create mode 100644 add-interactive.h create mode 100644 builtin/add--helper.c
base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/103
Range-diff vs v2:
1: 737767b6f4 = 1: 737767b6f4 diff: export diffstat interface
2: 91b1963125 = 2: 91b1963125 add--helper: create builtin helper for interactive add
3: d247ef69fe = 3: d247ef69fe add-interactive.c: implement status command
4: 4950c889aa = 4: 4950c889aa add--interactive.perl: use add--helper --status for status_cmd
5: cf4e913a5a ! 5: 581b108c9c add-interactive.c: implement show-help command
@@ -23,24 +23,18 @@
+void add_i_show_help(void)
+{
+ const char *help_color = get_color(COLOR_HELP);
-+ color_fprintf(stdout, help_color, "%s%s", _("status"),
-+ N_(" - show paths with changes"));
-+ printf("\n");
-+ color_fprintf(stdout, help_color, "%s%s", _("update"),
-+ N_(" - add working tree state to the staged set of changes"));
-+ printf("\n");
-+ color_fprintf(stdout, help_color, "%s%s", _("revert"),
-+ N_(" - revert staged set of changes back to the HEAD version"));
-+ printf("\n");
-+ color_fprintf(stdout, help_color, "%s%s", _("patch"),
-+ N_(" - pick hunks and update selectively"));
-+ printf("\n");
-+ color_fprintf(stdout, help_color, "%s%s", _("diff"),
-+ N_(" - view diff between HEAD and index"));
-+ printf("\n");
-+ color_fprintf(stdout, help_color, "%s%s", _("add untracked"),
-+ N_(" - add contents of untracked files to the staged set of changes"));
-+ printf("\n");
++ color_fprintf_ln(stdout, help_color, "status - %s",
++ _("show paths with changes"));
++ color_fprintf_ln(stdout, help_color, "update - %s",
++ _("add working tree state to the staged set of changes"));
++ color_fprintf_ln(stdout, help_color, "revert - %s",
++ _("revert staged set of changes back to the HEAD version"));
++ color_fprintf_ln(stdout, help_color, "patch - %s",
++ _("pick hunks and update selectively"));
++ color_fprintf_ln(stdout, help_color, "diff - %s",
++ _("view diff between HEAD and index"));
++ color_fprintf_ln(stdout, help_color, "add untracked - %s",
++ _("add contents of untracked files to the staged set of changes"));
+}
diff --git a/add-interactive.h b/add-interactive.h
6: 2b4714b8d0 = 6: aede733318 t3701-add-interactive: test add_i_show_help()
7: 6ede6d9251 = 7: b9a1a7e37a add--interactive.perl: use add--helper --show-help for help_cmdSignature · 1 line
gitgitgadget
PATCH v3 1/7 diff: export diffstat interface
From: Daniel Ferreira <[email protected]>
Make the diffstat interface (namely, the diffstat_t struct and compute_diffstat) no longer be internal to diff.c and allow it to be used by other parts of git.
This is helpful for code that may want to easily extract information from files using the diff machinery, while flushing it differently from how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status.
--- diff.c | 36 ++++++++++++++---------------------- diff.h | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 22 deletions(-)
diff.c +14 -22
@@ -2421,22 +2421,6 @@ static void pprint_rename(struct strbuf *name, const char *a, const char *b) } } -struct diffstat_t { -int nr; -int alloc; -struct diffstat_file { -char *from_name; -char *name; -char *print_name; -const char *comments; -unsigned is_unmerged:1 ; -unsigned is_binary:1 ; -unsigned is_renamed:1 ; -unsigned is_interesting:1 ; -uintmax_t added, deleted; - } **files; -}; -static struct diffstat_file *diffstat_add (struct diffstat_t *diffstat,const char *name_a,const char *name_b)
@@ -5922,12 +5906,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat; -memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )
@@ -6227,6 +6206,19 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)return ignored; } +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff; + +memset (diffstat,0 ,sizeof (struct diffstat_t )); +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +if (check_pair_status (p)) +diff_flush_stat (p, options, diffstat); + } +} + void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid,
diff.h +18 -0
@@ -239,6 +239,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);void diff_emit_submodule_pipethrough (struct diff_options *o,const char *line,int len); +struct diffstat_t { +int nr; +int alloc; +struct diffstat_file { +char *from_name; +char *name; +char *print_name; +const char *comments; +unsigned is_unmerged:1 ; +unsigned is_binary:1 ; +unsigned is_renamed:1 ; +unsigned is_interesting:1 ; +uintmax_t added, deleted; + } **files; +}; +enum color_diff {DIFF_RESET =0 ,DIFF_CONTEXT =1 ,
@@ -327,6 +343,8 @@ void diff_change(struct diff_options *,struct diff_filepair *diff_unmerge (struct diff_options *,const char *path); +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat); +#define DIFF_SETUP_REVERSE 1#define DIFF_SETUP_USE_SIZE_CACHE 4
Signature · 1 line
gitgitgadget
PATCH v3 2/7 add--helper: create builtin helper for interactive add
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Create a builtin helper for git-add--interactive, which right now is not able to do anything.
This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and performance (specially on non-POSIX systems like Windows).
Additionally, an eventual complete port of git-add--interactive would remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses.
--- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/add--helper.c | 6 ++++++ git.c | 1 + 5 files changed, 10 insertions(+) create mode 100644 builtin/add--helper.c
.gitignore +1 -0
@@ -15,6 +15,7 @@ /git /git-add /git-add--interactive +/git-add--helper /git-am /git-annotate /git-apply
Makefile +1 -0
@@ -1023,6 +1023,7 @@ LIB_OBJS += xdiff-interface.o LIB_OBJS += zlib.o BUILTIN_OBJS += builtin/add.o +BUILTIN_OBJS += builtin/add--helper.o BUILTIN_OBJS += builtin/am.o BUILTIN_OBJS += builtin/annotate.o BUILTIN_OBJS += builtin/apply.o
builtin.h +1 -0
@@ -128,6 +128,7 @@ extern void setup_auto_pager(const char *cmd, int def);extern int is_builtin (const char *s);extern int cmd_add (int argc,const char **argv,const char *prefix); +extern int cmd_add__helper (int argc,const char **argv,const char *prefix);extern int cmd_am (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix);
builtin/add--helper.c new file +6 -0
@@ -0,0 +1,6 @@ +#include "builtin.h" + +int cmd_add__helper (int argc,const char **argv,const char *prefix) +{ +return 0 ; +}
git.c +1 -0
@@ -443,6 +443,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },
Signature · 1 line
gitgitgadget
PATCH v3 4/7 add--interactive.perl: use add--helper --status for status_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Call the newly introduced add--helper builtin on status_cmd() instead of relying on add--interactive's Perl functions to build print the numstat.
--- git-add--interactive.perl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
git-add--interactive.perl +1 -3
@@ -597,9 +597,7 @@ sub prompt_help_cmd { } sub status_cmd { - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, - list_modified()); - print "\n"; + system(qw(git add--helper --status)); } sub say_n_paths {
Signature · 1 line
gitgitgadget
PATCH v3 5/7 add-interactive.c: implement show-help command
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Implement show-help command in add-interactive.c and use it in builtin add--helper.c.
Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by handle_builtin and re-routed to the help command, without ever calling cmd_add__helper().
--- add-interactive.c | 17 +++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 26 insertions(+), 2 deletions(-)
add-interactive.c +17 -0
@@ -244,3 +244,20 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf_ln (stdout, help_color,"status - %s" , +_ ("show paths with changes" )); +color_fprintf_ln (stdout, help_color,"update - %s" , +_ ("add working tree state to the staged set of changes" )); +color_fprintf_ln (stdout, help_color,"revert - %s" , +_ ("revert staged set of changes back to the HEAD version" )); +color_fprintf_ln (stdout, help_color,"patch - %s" , +_ ("pick hunks and update selectively" )); +color_fprintf_ln (stdout, help_color,"diff - %s" , +_ ("view diff between HEAD and index" )); +color_fprintf_ln (stdout, help_color,"add untracked - %s" , +_ ("add contents of untracked files to the staged set of changes" )); +}
add-interactive.h +3 -1
@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif
builtin/add--helper.c +6 -1
@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)
@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };
@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Signature · 1 line
gitgitgadget
PATCH v3 6/7 t3701-add-interactive: test add_i_show_help()
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that add_i_show_help() outputs expected content.
Also, add it before changing git-add--interactive.perl's help_cmd to demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.
--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
t/t3701-add-interactive.sh +24 -0
@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH v3 7/7 add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Change help_cmd sub in git-add--interactive.perl to use show-help command from builtin add--helper.
--- git-add--interactive.perl | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
git-add--interactive.perl +1 -10
@@ -1717,16 +1717,7 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes -update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + system(qw(git add--helper --show-help)); } sub process_args {
Signature · 1 line
gitgitgadget
PATCH v3 3/7 add-interactive.c: implement status command
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Add new files: add-interactive.c and add-interactive.h, which will be used for implementing "application logic" of git add -i, whereas add--helper.c will be used mostly for parsing the command line. We're a bit lax with the command-line parsing, as the command is intended to be called only by one internal user: the add--interactive script.
Implement add --interactive's status command in add-interactive.c and use it in builtin add--helper.c.
It prints a numstat comparing changed files between a) the worktree and the index; b) the index and the HEAD.
To do so, we use run_diff_index() and run_diff_files() to get changed files, use the diffstat API on them to get the numstat and use a combination of a hashmap and qsort() to print the result in O(n) + O(n lg n) complexity.
This is the first interactive add command implemented in C of those anticipated by the previous commit, which introduced
the add--helper built-in.
--- Makefile | 1 + add-interactive.c | 246 ++++++++++++++++++++++++++++++++++++++++++ add-interactive.h | 8 ++ builtin/add--helper.c | 32 ++++++ 4 files changed, 287 insertions(+) create mode 100644 add-interactive.c create mode 100644 add-interactive.h
Makefile +1 -0
@@ -827,6 +827,7 @@ LIB_H = $(shell $(FIND) . \ -name '*.h' -print) LIB_OBJS += abspath.o +LIB_OBJS += add-interactive.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o
add-interactive.c new file +246 -0
246 lines not on this page · raw message
add-interactive.h new file +8 -0
@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_print_modified (void ); + +#endif
builtin/add--helper.c +32 -0
@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_print_modified (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Signature · 1 line
gitgitgadget
Re: [PATCH v3 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Slavica Djukic via GitGitGadgetQuoted 22 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Change
help_cmdsub in git-add--interactive.perl to use show-help command from builtin add--helper.Signed-off-by Slavica Djukic <[email protected]>--- git-add--interactive.perl | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)git-add--interactive.perl +0
-4@@ -1717,16 +1717,7 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes
git-add--interactive.perl sub quit_cmd { · line 1723
Quoted 10 lines
git-add--interactive.perl +1
-6-update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + system(qw(git add--helper --show-help)); } sub process_args {
Both this and an earlier change in this series replaces a callback command with an invocation of system() without any error checking. So if this add-helper fails for whatever reason we'll silently fail to report it.
I think it makes sense to put something like the following WIP code earlier in the series. Then if the command was e.g. ["false", "git", "status"] we'd see:
What now> s
oh noes when running 'false git status': returned error '1'
WIP patch. Obviously not ready as-is, but feel free to consider this to have my SOB & adapt it.
git-add--interactive.perl +18 -6
@@ -1777,7 +1777,7 @@ sub process_args { } sub main_loop { - my @cmd = ([ 'status', \&status_cmd, ], + my @cmd = ([ 'status', ["git", "status"] ], [ 'update', \&update_cmd, ], [ 'revert', \&revert_cmd, ], [ 'add untracked', \&add_untracked_cmd, ],
@@ -1794,11 +1794,23 @@ sub main_loop { ON_EOF => \&quit_cmd, IMMEDIATE => 1 }, @cmd); if ($it) { - eval { - $it->[1]->(); - }; - if ($@) { - print "$@"; + my $cb = $it->[1]; + if (ref $cb eq 'CODE') { + eval { + $cb->(); + 1; + } or do { + print "$@"; + }; + } else { + if (system(@$cb) != 0) { + if ($? == -1 || $? & 127) { + print STDERR "oh noes when running '@$cb': unexpected '$?'\n"; + } else { + my $ret = $? >> 8; + print STDERR "oh noes when running '@$cb': returned error '$ret'\n"; + } + } } } }
Re: [PATCH v3 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
Hello Ævar,
thanks for taking time and making review.
Quoted 43 lines · Ævar Arnfjörð Bjarmason
Quoted 22 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Change
help_cmdsub in git-add--interactive.perl to use show-help command from builtin add--helper.Signed-off-by Slavica Djukic <[email protected]>--- git-add--interactive.perl | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)git-add--interactive.perl +0
-4@@ -1717,16 +1717,7 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changesgit-add--interactive.perl sub quit_cmd { · line 1723
Quoted 10 lines
git-add--interactive.perl +1
-6-update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + system(qw(git add--helper --show-help)); } sub process_args {Both this and an earlier change in this series replaces a callback command with an invocation of
system()without any error checking. So if this add-helper fails for whatever reason we'll silently fail to report it.I think it makes sense to put something like the following WIP code earlier in the series. Then if the command was e.g. ["false", "git", "status"] we'd see:
Quoted 5 lines
What now> s
oh noes when running 'false git status': returned error '1'WIP patch. Obviously not ready as-is, but feel free to consider this to have my SOB & adapt it.
And thank you for writing up this WIP patch. I will adapt it and have your SOB.
-Slavica
Quoted 44 lines
git-add--interactive.perl +18
-6@@ -1777,7 +1777,7 @@ sub process_args { } sub main_loop { - my @cmd = ([ 'status', \&status_cmd, ], + my @cmd = ([ 'status', ["git", "status"] ], [ 'update', \&update_cmd, ], [ 'revert', \&revert_cmd, ], [ 'add untracked', \&add_untracked_cmd, ],@@ -1794,11 +1794,23 @@ sub main_loop { ON_EOF => \&quit_cmd, IMMEDIATE => 1 }, @cmd); if ($it) { - eval { - $it->[1]->(); - }; - if ($@) { - print "$@"; + my $cb = $it->[1]; + if (ref $cb eq 'CODE') { + eval { + $cb->(); + 1; + } or do { + print "$@"; + }; + } else { + if (system(@$cb) != 0) { + if ($? == -1 || $? & 127) { + print STDERR "oh noes when running '@$cb': unexpected '$?'\n"; + } else { + my $ret = $? >> 8; + print STDERR "oh noes when running '@$cb': returned error '$ret'\n"; + } + } } } }
Re: [PATCH v3 6/7] t3701-add-interactive: test add_i_show_help()
In reply to Slavica Djukic via GitGitGadgetHi Slavica
Having suggested that you move this patch up the series, I thought I should check that it does actually pass before the rewrite. Unfortunately when I run it it fails, but it is due to a missing trailing space in the expected file. I get
actual renamed from expect +0 -0
@@ -11,5 +11,5 @@ <BOLD>*** Commands ***<RESET> 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3:
<BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked
5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7:
<BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp
-<BOLD;BLUE>What now<RESET>>
+<BOLD;BLUE>What now<RESET>>
Bye.
There should be a space after '<BOLD;BLUE>What now<RESET>>' as the prompt has a space after it. The space is actually in the patch but it got eaten by 'git am' (I have core.whitespace set but I think cleaning up trailing whitespace is on by default anyway). I think the best way to make this patch easier to apply would be to use '$SP' which is already set up in earlier tests to avoid trailing whitespace in here documents. You'll need to change
cat >expect <<-\EOF
to
cat >expect <<-EOF
to enable variable interpolation in the here document as well.
Best Wishes
Phillip
Quoted 49 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that
add_i_show_help()outputs expected content.
Also, add it before changing git-add--interactive.perl'shelp_cmdto demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.Signed-off-by Slavica Djukic <[email protected]>--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)t/t3701-add-interactive.sh +24
-0@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Re: [PATCH v3 6/7] t3701-add-interactive: test add_i_show_help()
Hi Phillip,
Quoted 19 lines · Phillip Wood
Hi Slavica
Having suggested that you move this patch up the series, I thought I should check that it does actually pass before the rewrite. Unfortunately when I run it it fails, but it is due to a missing trailing space in the expected file. I get
actual renamed from expect +0
-0@@ -11,5 +11,5 @@ <BOLD>*** Commands ***<RESET> 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3:<BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked
5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7:<BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp
-<BOLD;BLUE>What now<RESET>>
+<BOLD;BLUE>What now<RESET>>
Bye.
Quoted 11 lines
There should be a space after '<BOLD;BLUE>What now<RESET>>' as the prompt has a space after it. The space is actually in the patch but it got eaten by 'git am' (I have core.whitespace set but I think cleaning up trailing whitespace is on by default anyway). I think the best way to make this patch easier to apply would be to use '$SP' which is already set up in earlier tests to avoid trailing whitespace in here documents. You'll need to change
cat >expect <<-\EOF
to
cat >expect <<-EOF
to enable variable interpolation in the here document as well.
Thanks for checking the test. I had no idea that this might happen.
I'll send new iteration soon, along with this changes.
-Slavica
Quoted 56 lines
Best Wishes
Phillip
Quoted 49 lines · Slavica Djukic via GitGitGadget
From: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that
add_i_show_help()outputs expected content.
Also, add it before changing git-add--interactive.perl'shelp_cmdto demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.Signed-off-by Slavica Djukic <[email protected]>--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)t/t3701-add-interactive.sh +24
-0@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-\EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
PATCH v4 1/7 diff: export diffstat interface
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Make the diffstat interface (namely, the diffstat_t struct and compute_diffstat) no longer be internal to diff.c and allow it to be used by other parts of git.
This is helpful for code that may want to easily extract information from files using the diff machinery, while flushing it differently from how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status.
--- diff.c | 36 ++++++++++++++---------------------- diff.h | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 22 deletions(-)
diff.c +14 -22
@@ -2421,22 +2421,6 @@ static void pprint_rename(struct strbuf *name, const char *a, const char *b) } } -struct diffstat_t { -int nr; -int alloc; -struct diffstat_file { -char *from_name; -char *name; -char *print_name; -const char *comments; -unsigned is_unmerged:1 ; -unsigned is_binary:1 ; -unsigned is_renamed:1 ; -unsigned is_interesting:1 ; -uintmax_t added, deleted; - } **files; -}; -static struct diffstat_file *diffstat_add (struct diffstat_t *diffstat,const char *name_a,const char *name_b)
@@ -5922,12 +5906,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat; -memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )
@@ -6227,6 +6206,19 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)return ignored; } +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff; + +memset (diffstat,0 ,sizeof (struct diffstat_t )); +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +if (check_pair_status (p)) +diff_flush_stat (p, options, diffstat); + } +} + void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid,
diff.h +18 -0
@@ -239,6 +239,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);void diff_emit_submodule_pipethrough (struct diff_options *o,const char *line,int len); +struct diffstat_t { +int nr; +int alloc; +struct diffstat_file { +char *from_name; +char *name; +char *print_name; +const char *comments; +unsigned is_unmerged:1 ; +unsigned is_binary:1 ; +unsigned is_renamed:1 ; +unsigned is_interesting:1 ; +uintmax_t added, deleted; + } **files; +}; +enum color_diff {DIFF_RESET =0 ,DIFF_CONTEXT =1 ,
@@ -327,6 +343,8 @@ void diff_change(struct diff_options *,struct diff_filepair *diff_unmerge (struct diff_options *,const char *path); +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat); +#define DIFF_SETUP_REVERSE 1#define DIFF_SETUP_USE_SIZE_CACHE 4
Signature · 1 line
gitgitgadget
v4 cover Turn git add-i into built-in
In reply to Slavica Đukić via GitGitGadgetThis is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#u
Changes since v3:
* add error check when calling add--helper's functions from git-add--interactive.perl
* replace trailing whitespace in the test with variable $SP used in earlier tests
Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive add add-interactive.c: implement status command add--interactive.perl: use add--helper --status for status_cmd
Slavica Djukic (3):
add-interactive.c: implement show-help command
t3701-add-interactive: test add_i_show_help() add--interactive.perl: use add--helper --show-help for help_cmd
.gitignore | 1 + Makefile | 2 + add-interactive.c | 263 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 17 +-- git.c | 1 + t/t3701-add-interactive.sh | 24 ++++ 11 files changed, 381 insertions(+), 35 deletions(-) create mode 100644 add-interactive.c create mode 100644 add-interactive.h create mode 100644 builtin/add--helper.c
base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/103
Range-diff vs v3:
1: 737767b6f4 = 1: 737767b6f4 diff: export diffstat interface
2: 91b1963125 = 2: 91b1963125 add--helper: create builtin helper for interactive add
3: d247ef69fe = 3: d247ef69fe add-interactive.c: implement status command
4: 4950c889aa ! 4: fb3f9378ac add--interactive.perl: use add--helper --status for status_cmd
@@ -2,12 +2,21 @@
add--interactive.perl: use add--helper --status for status_cmd
- Call the newly introduced add--helper builtin on
+ Call the newly introduced add--helper builtin in
status_cmd() instead of relying on add--interactive's Perl
- functions to build print the numstat.
+ functions to print the numstat.
+
+ If an error occurs, it will be reported, but the Perl script will
+ not exit, since the add--helper is called within an eval block.
+
+ As the Perl script will go away soon, so will this scenario, where
+ the built-in helper is called from the Perl script. Combined with
+ the fact that it would be hard to test, we'll pass on adding
+ a regression test for this.
Signed-off-by: Daniel Ferreira <[email protected]>
Signed-off-by: Slavica Djukic <[email protected]>
+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
--- a/git-add--interactive.perl
@@ -19,7 +28,8 @@
- list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
- list_modified());
- print "\n";
-+ system(qw(git add--helper --status));
++ my @status_cmd = ("git", "add--helper", "--status");
++ !system(@status_cmd) or die "@status_cmd exited with code $?";
}
sub say_n_paths {
5: 581b108c9c = 5: ab16afd1d5 add-interactive.c: implement show-help command
6: aede733318 ! 6: 0a27304a84 t3701-add-interactive: test add_i_show_help()
@@ -7,6 +7,7 @@
Also, add it before changing git-add--interactive.perl's help_cmd
to demonstrate that there are no changes introduced by the
conversion to C.
+
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100
to force colored output on Windows.
@@ -21,7 +22,7 @@
+test_expect_success 'show help from add--helper' '
+ git reset --hard &&
-+ cat >expect <<-\EOF &&
++ cat >expect <<-EOF &&
+
+ <BOLD>*** Commands ***<RESET>
+ 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked
@@ -35,7 +36,7 @@
+ <BOLD>*** Commands ***<RESET>
+ 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked
+ 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp
-+ <BOLD;BLUE>What now<RESET>>
++ <BOLD;BLUE>What now<RESET>>$SP
+ Bye.
+ EOF
+ test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && 7: b9a1a7e37a ! 7: ca2a7c4375 add--interactive.perl: use add--helper --show-help for help_cmd
@@ -5,7 +5,15 @@
Change help_cmd sub in git-add--interactive.perl to use
show-help command from builtin add--helper.
+ If an error occurs, it will be reported, but the Perl script will
+ not exit, since the add--helper is called within an eval block.
+
+ Just like the change where the Perl script calls the add--helper
+ to print the numstat, also here we forgo adding a regression test:
+ the Perl script is on its way out (and this patch is part of that journey).
+
+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
diff --git a/git-add--interactive.perl b/git-add--interactive.perl --- a/git-add--interactive.perl
@@ -24,7 +32,8 @@
-diff - view diff between HEAD and index
-add untracked - add contents of untracked files to the staged set of changes -EOF
-+ system(qw(git add--helper --show-help));
++ my @help_cmd = ("git", "add--helper", "--show-help");
++ !system(@help_cmd) or die "@help_cmd exited with code $?";
}
sub process_args {
Signature · 1 line
gitgitgadget
PATCH v4 2/7 add--helper: create builtin helper for interactive add
From: Daniel Ferreira <[email protected]>
Create a builtin helper for git-add--interactive, which right now is not able to do anything.
This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and performance (specially on non-POSIX systems like Windows).
Additionally, an eventual complete port of git-add--interactive would remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses.
--- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/add--helper.c | 6 ++++++ git.c | 1 + 5 files changed, 10 insertions(+) create mode 100644 builtin/add--helper.c
.gitignore +1 -0
@@ -15,6 +15,7 @@ /git /git-add /git-add--interactive +/git-add--helper /git-am /git-annotate /git-apply
Makefile +1 -0
@@ -1023,6 +1023,7 @@ LIB_OBJS += xdiff-interface.o LIB_OBJS += zlib.o BUILTIN_OBJS += builtin/add.o +BUILTIN_OBJS += builtin/add--helper.o BUILTIN_OBJS += builtin/am.o BUILTIN_OBJS += builtin/annotate.o BUILTIN_OBJS += builtin/apply.o
builtin.h +1 -0
@@ -128,6 +128,7 @@ extern void setup_auto_pager(const char *cmd, int def);extern int is_builtin (const char *s);extern int cmd_add (int argc,const char **argv,const char *prefix); +extern int cmd_add__helper (int argc,const char **argv,const char *prefix);extern int cmd_am (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix);
builtin/add--helper.c new file +6 -0
@@ -0,0 +1,6 @@ +#include "builtin.h" + +int cmd_add__helper (int argc,const char **argv,const char *prefix) +{ +return 0 ; +}
git.c +1 -0
@@ -443,6 +443,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },
Signature · 1 line
gitgitgadget
PATCH v4 3/7 add-interactive.c: implement status command
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Add new files: add-interactive.c and add-interactive.h, which will be used for implementing "application logic" of git add -i, whereas add--helper.c will be used mostly for parsing the command line. We're a bit lax with the command-line parsing, as the command is intended to be called only by one internal user: the add--interactive script.
Implement add --interactive's status command in add-interactive.c and use it in builtin add--helper.c.
It prints a numstat comparing changed files between a) the worktree and the index; b) the index and the HEAD.
To do so, we use run_diff_index() and run_diff_files() to get changed files, use the diffstat API on them to get the numstat and use a combination of a hashmap and qsort() to print the result in O(n) + O(n lg n) complexity.
This is the first interactive add command implemented in C of those anticipated by the previous commit, which introduced
the add--helper built-in.
--- Makefile | 1 + add-interactive.c | 246 ++++++++++++++++++++++++++++++++++++++++++ add-interactive.h | 8 ++ builtin/add--helper.c | 32 ++++++ 4 files changed, 287 insertions(+) create mode 100644 add-interactive.c create mode 100644 add-interactive.h
Makefile +1 -0
@@ -827,6 +827,7 @@ LIB_H = $(shell $(FIND) . \ -name '*.h' -print) LIB_OBJS += abspath.o +LIB_OBJS += add-interactive.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o
add-interactive.c new file +246 -0
246 lines not on this page · raw message
add-interactive.h new file +8 -0
@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_print_modified (void ); + +#endif
builtin/add--helper.c +32 -0
@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_print_modified (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Signature · 1 line
gitgitgadget
PATCH v4 6/7 t3701-add-interactive: test add_i_show_help()
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that add_i_show_help() outputs expected content.
Also, add it before changing git-add--interactive.perl's help_cmd to demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.
--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
t/t3701-add-interactive.sh +24 -0
@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>>$SP + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH v4 5/7 add-interactive.c: implement show-help command
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Implement show-help command in add-interactive.c and use it in builtin add--helper.c.
Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by handle_builtin and re-routed to the help command, without ever calling cmd_add__helper().
--- add-interactive.c | 17 +++++++++++++++++ add-interactive.h | 4 +++- builtin/add--helper.c | 7 ++++++- 3 files changed, 26 insertions(+), 2 deletions(-)
add-interactive.c +17 -0
@@ -244,3 +244,20 @@ void add_i_print_modified(void)free (files);hashmap_free (&s.file_map,1 ); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf_ln (stdout, help_color,"status - %s" , +_ ("show paths with changes" )); +color_fprintf_ln (stdout, help_color,"update - %s" , +_ ("add working tree state to the staged set of changes" )); +color_fprintf_ln (stdout, help_color,"revert - %s" , +_ ("revert staged set of changes back to the HEAD version" )); +color_fprintf_ln (stdout, help_color,"patch - %s" , +_ ("pick hunks and update selectively" )); +color_fprintf_ln (stdout, help_color,"diff - %s" , +_ ("view diff between HEAD and index" )); +color_fprintf_ln (stdout, help_color,"add untracked - %s" , +_ ("add contents of untracked files to the staged set of changes" )); +}
add-interactive.h +3 -1
@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_print_modified (void ); -#endif +void add_i_show_help (void ); + +#endif
builtin/add--helper.c +6 -1
@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)
@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };
@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_print_modified (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Signature · 1 line
gitgitgadget
PATCH v4 4/7 add--interactive.perl: use add--helper --status for status_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Call the newly introduced add--helper builtin in status_cmd() instead of relying on add--interactive's Perl functions to print the numstat.
If an error occurs, it will be reported, but the Perl script will not exit, since the add--helper is called within an eval block.
As the Perl script will go away soon, so will this scenario, where the built-in helper is called from the Perl script. Combined with the fact that it would be hard to test, we'll pass on adding a regression test for this.
--- git-add--interactive.perl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
git-add--interactive.perl +2 -3
@@ -597,9 +597,8 @@ sub prompt_help_cmd { } sub status_cmd { - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, - list_modified()); - print "\n"; + my @status_cmd = ("git", "add--helper", "--status"); + !system(@status_cmd) or die "@status_cmd exited with code $?"; } sub say_n_paths {
Signature · 1 line
gitgitgadget
PATCH v4 7/7 add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Change help_cmd sub in git-add--interactive.perl to use show-help command from builtin add--helper.
If an error occurs, it will be reported, but the Perl script will not exit, since the add--helper is called within an eval block.
Just like the change where the Perl script calls the add--helper to print the numstat, also here we forgo adding a regression test:
the Perl script is on its way out (and this patch is part of that journey).
--- git-add--interactive.perl | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
git-add--interactive.perl +2 -10
@@ -1718,16 +1718,8 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes -update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + my @help_cmd = ("git", "add--helper", "--show-help"); + !system(@help_cmd) or die "@help_cmd exited with code $?"; } sub process_args {
Signature · 1 line
gitgitgadget
Hi Phillip,
Sorry for omitting you in Cc, again.
I accidentally wrote yours and Ævar's email prefixed with Cc:, rather then separating them with comma, like Johannes said I should.
-Slavica
Quoted 143 lines · Slavica Đukić via GitGitGadget
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#uChanges since v3:
* add error check when calling add--helper's functions from git-add--interactive.perl
* replace trailing whitespace in the test with variable $SP used in earlier testsCc Phillip Wood [email protected] [[email protected]]Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive addadd-interactive.c: implement status command add--interactive.perl: use add--helper --status forstatus_cmdSlavica Djukic (3):
add-interactive.c: implement show-help command t3701-add-interactive: test add_i_show_help() add--interactive.perl: use add--helper --show-help for help_cmd.gitignore | 1 + Makefile | 2 + add-interactive.c | 263 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++ builtin.h | 1 + builtin/add--helper.c | 43 ++++++ diff.c | 36 ++--- diff.h | 18 +++ git-add--interactive.perl | 17 +-- git.c | 1 + t/t3701-add-interactive.sh | 24 ++++ 11 files changed, 381 insertions(+), 35 deletions(-)create mode 100644
add-interactive.c
create mode 100644add-interactive.h
create mode 100644builtin/add--helper.cbase-commit:
b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/103Range-diff vs v3:
1: 737767b6f4 = 1: 737767b6f4 diff: export diffstat interface 2: 91b1963125 = 2: 91b1963125 add--helper: create builtin helper for interactive add 3: d247ef69fe = 3: d247ef69fe add-interactive.c: implement status command 4: 4950c889aa ! 4: fb3f9378ac add--interactive.perl: use add--helper --status for status_cmd @@ -2,12 +2,21 @@ add--interactive.perl: use add--helper --status for status_cmd - Call the newly introduced add--helper builtin on + Call the newly introduced add--helper builtin in status_cmd() instead of relying on add--interactive's Perl - functions to build print the numstat. + functions to print the numstat. + + If an error occurs, it will be reported, but the Perl script will + not exit, since the add--helper is called within an eval block. + + As the Perl script will go away soon, so will this scenario, where + the built-in helper is called from the Perl script. Combined with + the fact that it would be hard to test, we'll pass on adding + a regression test for this. Signed-off-by: Daniel Ferreira <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> + Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> diff --git a/git-add--interactive.perl b/git-add--interactive.perl --- a/git-add--interactive.perl @@ -19,7 +28,8 @@ - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, - list_modified()); - print "\n"; -+ system(qw(git add--helper --status)); ++ my @status_cmd = ("git", "add--helper", "--status"); ++ !system(@status_cmd) or die "@status_cmd exited with code $?"; } sub say_n_paths { 5: 581b108c9c = 5: ab16afd1d5 add-interactive.c: implement show-help command 6: aede733318 ! 6: 0a27304a84 t3701-add-interactive: test add_i_show_help() @@ -7,6 +7,7 @@ Also, add it before changing git-add--interactive.perl's help_cmd to demonstrate that there are no changes introduced by the conversion to C. + Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows. @@ -21,7 +22,7 @@ +test_expect_success 'show help from add--helper' ' + git reset --hard && -+ cat >expect <<-\EOF && ++ cat >expect <<-EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked @@ -35,7 +36,7 @@ + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp -+ <BOLD;BLUE>What now<RESET>> ++ <BOLD;BLUE>What now<RESET>>$SP + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored &&7:
b9a1a7e37a! 7:ca2a7c4375add--interactive.perl: use add--helper --show-help forhelp_cmd
@@ -5,7 +5,15 @@
Changehelp_cmdsub in git-add--interactive.perl to use
show-help command from builtin add--helper.+ If an error occurs, it will be reported, but the Perl script will
+ not exit, since the add--helper is called within an eval block.
+
+ Just like the change where the Perl script calls the add--helper
+ to print the numstat, also here we forgo adding a regression test:
+ the Perl script is on its way out (and this patch is part of that journey).
+Signed-off-by Slavica Djukic <[email protected]>+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
diff --git a/git-add--interactive.perl b/git-add--interactive.perl --- a/git-add--interactive.perl
@@ -24,7 +32,8 @@
-diff - view diff between HEAD and index
-add untracked - add contents of untracked files to the staged set of changes-EOF
-+ system(qw(git add--helper --show-help));
++ my @help_cmd = ("git", "add--helper", "--show-help");
++ !system(@help_cmd) or die "@help_cmd exited with code $?";
}sub
process_args{
Hi Slavica
Quoted 5 lines · Slavica Djukic
Hi Phillip,
Sorry for omitting you in Cc, again.
I accidentally wrote yours and Ævar's email prefixed with Cc:, rather then separating them with comma, like Johannes said I should.
Don't worry, thanks for emailing here. I've looked at the range diff and I think your patches are good.
Best Wishes
Phillip
Quoted 139 lines
-Slavica
Quoted 135 lines · Slavica Đukić via GitGitGadget
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#uChanges since v3:
* add error check when calling add--helper's functions from git-add--interactive.perl
* replace trailing whitespace in the test with variable $SP used in earlier testsCc Phillip Wood [email protected] [[email protected]]Daniel Ferreira (4):
diff: export diffstat interface
add--helper: create builtin helper for interactive addadd-interactive.c: implement status command add--interactive.perl: use add--helper --status forstatus_cmdSlavica Djukic (3):
add-interactive.c: implement show-help command
t3701-add-interactive: testadd_i_show_help()add--interactive.perl: use add--helper --show-help forhelp_cmd.gitignore | 1 +
Makefile | 2 +
add-interactive.c | 263 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 ++
builtin.h | 1 +
builtin/add--helper.c | 43 ++++++
diff.c | 36 ++---
diff.h | 18 +++ git-add--interactive.perl | 17 +--
git.c | 1 +t/t3701-add-interactive.sh| 24 ++++11 files changed, 381 insertions(+), 35 deletions(-)create mode 100644
add-interactive.c
create mode 100644add-interactive.h
create mode 100644builtin/add--helper.cbase-commit:
b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v4Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/103Range-diff vs v3:
1:
737767b6f4= 1:737767b6f4diff: export diffstat interface
2:91b1963125= 2:91b1963125add--helper: create builtin helper for interactive add 3:d247ef69fe= 3:d247ef69feadd-interactive.c: implement status command
4:4950c889aa! 4:fb3f9378acadd--interactive.perl: use add--helper --status forstatus_cmd@@ -2,12 +2,21 @@
add--interactive.perl: use add--helper --status forstatus_cmd
- Call the newly introduced add--helper builtin on
+ Call the newly introduced add--helper builtin in
status_cmd()instead of relying on add--interactive's Perl
- functions to build print the numstat.
+ functions to print the numstat.
+
+ If an error occurs, it will be reported, but the Perl script will
+ not exit, since the add--helper is called within an eval block.
+
+ As the Perl script will go away soon, so will this scenario, where
+ the built-in helper is called from the Perl script. Combined with
+ the fact that it would be hard to test, we'll pass on adding
+ a regression test for this.Signed-off-by Daniel Ferreira <[email protected]>Signed-off-by Slavica Djukic <[email protected]>+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
--- a/git-add--interactive.perl
@@ -19,7 +28,8 @@
- list_and_choose({LIST_ONLY=> 1, HEADER => $status_head },
-list_modified());
- print "\n";
-+ system(qw(git add--helper --status));
++ my @status_cmd = ("git", "add--helper", "--status");
++ !system(@status_cmd) or die "@status_cmd exited with code $?";
}
subsay_n_paths{
5:581b108c9c= 5:ab16afd1d5add-interactive.c: implement show-help command
6:aede733318! 6:0a27304a84t3701-add-interactive: testadd_i_show_help()
@@ -7,6 +7,7 @@
Also, add it before changing git-add--interactive.perl'shelp_cmd
to demonstrate that there are no changes introduced by the
conversion to C.
+
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100
to force colored output on Windows.
@@ -21,7 +22,7 @@
+test_expect_success 'show help from add--helper' '
+ git reset --hard &&
-+ cat >expect <<-\EOF &&
++ cat >expect <<-EOF&&
+
+ <BOLD>*** Commands ***<RESET>
+ 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked @@ -35,7 +36,7 @@
+ <BOLD>*** Commands ***<RESET>
+ 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp -+ <BOLD;BLUE>What now<RESET>>
++ <BOLD;BLUE>What now<RESET>>$SP
+ Bye.
+ EOF
+test_write_linesh | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored &&
7:b9a1a7e37a! 7:ca2a7c4375add--interactive.perl: use add--helper --show-help forhelp_cmd
@@ -5,7 +5,15 @@
Changehelp_cmdsub in git-add--interactive.perl to use
show-help command from builtin add--helper.
+ If an error occurs, it will be reported, but the Perl script will
+ not exit, since the add--helper is called within an eval block.
+
+ Just like the change where the Perl script calls the add--helper
+ to print the numstat, also here we forgo adding a regression test:
+ the Perl script is on its way out (and this patch is part of that journey).
+Signed-off-by Slavica Djukic <[email protected]>+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> diff --git a/git-add--interactive.perl b/git-add--interactive.perl --- a/git-add--interactive.perl
@@ -24,7 +32,8 @@
-diff - view diff between HEAD and index
-add untracked - add contents of untracked files to the staged set of changes-EOF
-+ system(qw(git add--helper --show-help));
++ my @help_cmd = ("git", "add--helper", "--show-help");
++ !system(@help_cmd) or die "@help_cmd exited with code $?"; }
subprocess_args{
PATCH v5 1/10 diff: export diffstat interface
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Make the diffstat interface (namely, the diffstat_t struct and compute_diffstat) no longer be internal to diff.c and allow it to be used by other parts of git.
This is helpful for code that may want to easily extract information from files using the diff machinery, while flushing it differently from how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status.
--- diff.c | 36 ++++++++++++++---------------------- diff.h | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 22 deletions(-)
diff.c +14 -22
@@ -2489,22 +2489,6 @@ static void pprint_rename(struct strbuf *name, const char *a, const char *b) } } -struct diffstat_t { -int nr; -int alloc; -struct diffstat_file { -char *from_name; -char *name; -char *print_name; -const char *comments; -unsigned is_unmerged:1 ; -unsigned is_binary:1 ; -unsigned is_renamed:1 ; -unsigned is_interesting:1 ; -uintmax_t added, deleted; - } **files; -}; -static struct diffstat_file *diffstat_add (struct diffstat_t *diffstat,const char *name_a,const char *name_b)
@@ -6001,12 +5985,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat; -memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )
@@ -6306,6 +6285,19 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)return ignored; } +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff; + +memset (diffstat,0 ,sizeof (struct diffstat_t )); +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +if (check_pair_status (p)) +diff_flush_stat (p, options, diffstat); + } +} + void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid,
diff.h +18 -0
@@ -240,6 +240,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);void diff_emit_submodule_pipethrough (struct diff_options *o,const char *line,int len); +struct diffstat_t { +int nr; +int alloc; +struct diffstat_file { +char *from_name; +char *name; +char *print_name; +const char *comments; +unsigned is_unmerged:1 ; +unsigned is_binary:1 ; +unsigned is_renamed:1 ; +unsigned is_interesting:1 ; +uintmax_t added, deleted; + } **files; +}; +enum color_diff {DIFF_RESET =0 ,DIFF_CONTEXT =1 ,
@@ -328,6 +344,8 @@ void diff_change(struct diff_options *,struct diff_filepair *diff_unmerge (struct diff_options *,const char *path); +void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat); +#define DIFF_SETUP_REVERSE 1#define DIFF_SETUP_USE_SIZE_CACHE 4
Signature · 1 line
gitgitgadget
PATCH v5 2/10 add--helper: create builtin helper for interactive add
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Create a builtin helper for git-add--interactive, which at this point is not doing anything.
This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and performance (specially on non-POSIX systems like Windows).
Additionally, an eventual complete port of git-add--interactive would remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses.
--- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/add--helper.c | 6 ++++++ git.c | 1 + 5 files changed, 10 insertions(+) create mode 100644 builtin/add--helper.c
.gitignore +1 -0
@@ -16,6 +16,7 @@ /git /git-add /git-add--interactive +/git-add--helper /git-am /git-annotate /git-apply
Makefile +1 -0
@@ -1043,6 +1043,7 @@ LIB_OBJS += xdiff-interface.o LIB_OBJS += zlib.o BUILTIN_OBJS += builtin/add.o +BUILTIN_OBJS += builtin/add--helper.o BUILTIN_OBJS += builtin/am.o BUILTIN_OBJS += builtin/annotate.o BUILTIN_OBJS += builtin/apply.o
builtin.h +1 -0
@@ -128,6 +128,7 @@ extern void setup_auto_pager(const char *cmd, int def);extern int is_builtin (const char *s);extern int cmd_add (int argc,const char **argv,const char *prefix); +extern int cmd_add__helper (int argc,const char **argv,const char *prefix);extern int cmd_am (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix);
builtin/add--helper.c new file +6 -0
@@ -0,0 +1,6 @@ +#include "builtin.h" + +int cmd_add__helper (int argc,const char **argv,const char *prefix) +{ +return 0 ; +}
git.c +1 -0
@@ -444,6 +444,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },
Signature · 1 line
gitgitgadget
v5 cover Turn git add-i into built-in
In reply to Slavica Đukić via GitGitGadgetThis is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#u
Changes since v4:
* rename print_modified to list_modifed
* the big change was implementing list_and_choose, which resulted in code refactoring, i.e. separating list_modified and status_cmd and making status_cmd use both list_modified and list_and_choose
* implement struct choice instead of struct stuff_item as main data structure for list_and_choose
* introduce list_only option and implement support for !list_only users
* introduce highlighting of unique prefixes
Note that authorship handling is slightly changed. In some of the commits, I used Original-patch-by instead of listing Daniel Ferreira as author.
Also, I would like to point out that my Outreachy internship is going to finish on March 4 and I would really appreciate reviews before it does.
Daniel Ferreira (3):
diff: export diffstat interface
add--helper: create builtin helper for interactive add add--interactive.perl: use add--helper --status for status_cmd
Slavica Djukic (7):
add-interactive.c: implement list_modified add-interactive.c: implement list_and_choose add-interactive.c: implement status command add-interactive.c: add support for list_only option add-interactive.c: implement show-help command
t3701-add-interactive: test add_i_show_help() add--interactive.perl: use add--helper --show-help for help_cmd
.gitignore | 1 + Makefile | 2 + add-interactive.c | 819 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 + builtin.h | 1 + builtin/add--helper.c | 43 ++ diff.c | 36 +- diff.h | 18 + git-add--interactive.perl | 17 +- git.c | 1 + t/t3701-add-interactive.sh | 24 ++ 11 files changed, 937 insertions(+), 35 deletions(-) create mode 100644 add-interactive.c create mode 100644 add-interactive.h create mode 100644 builtin/add--helper.c
base-commit: ca1b4116483b397e78483376296bcd23916ab553
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/103
Range-diff vs v4:
1: 737767b6f4 ! 1: d839f0c082 diff: export diffstat interface
@@ -11,6 +11,7 @@
how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status.
+ Mentored-by: Johannes Schindelin <[email protected]>
2: 91b1963125 ! 2: 304c3863b1 add--helper: create builtin helper for interactive add
@@ -2,8 +2,8 @@
add--helper: create builtin helper for interactive add
- Create a builtin helper for git-add--interactive, which right now is not
- able to do anything.
+ Create a builtin helper for git-add--interactive, which at this point
+ is not doing anything.
This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and
@@ -13,6 +13,7 @@
remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses.
+ Mentored-by: Johannes Schindelin <[email protected]>
3: d247ef69fe ! 3: 8790ffaa39 add-interactive.c: implement status command
@@ -1,17 +1,12 @@
-Author: Daniel Ferreira <[email protected]>
+Author: Slavica Djukic <[email protected]>
- add-interactive.c: implement status command
+ add-interactive.c: implement list_modified
- Add new files: add-interactive.c and add-interactive.h, which
- will be used for implementing "application logic" of git add -i,
- whereas add--helper.c will be used mostly for parsing the command line.
- We're a bit lax with the command-line parsing, as the command is
- intended to be called only by one internal user: the add--interactive script.
+ Implement list_modified from Perl, which will be used
+ by most of the *_cmd functions, including status in the
+ following commit.
- Implement add --interactive's status command in add-interactive.c and
- use it in builtin add--helper.c.
-
- It prints a numstat comparing changed files between a) the worktree and
+ It lists a numstat comparing changed files between a) the worktree and
the index; b) the index and the HEAD.
To do so, we use run_diff_index() and run_diff_files() to get changed
@@ -19,11 +14,13 @@
combination of a hashmap and qsort() to print the result in
O(n) + O(n lg n) complexity.
- This is the first interactive add command implemented in C of those
- anticipated by the previous commit, which introduced
- the add--helper built-in.
+ Add new file: add-interactive.c which will be used for implementing
+ "application logic" of git add -i (alongside with add-interactive.h,
+ added in later commit), whereas add--helper.c will be used mostly
+ for parsing the command line.
- Signed-off-by: Daniel Ferreira <[email protected]>
+ Mentored-by: Johannes Schindelin <[email protected]>
+ Original-patch-by: Daniel Ferreira <[email protected]>
Signed-off-by: Slavica Djukic <[email protected]>
diff --git a/Makefile b/Makefile
@@ -43,7 +40,6 @@
--- /dev/null
+++ b/add-interactive.c
@@
-+#include "add-interactive.h"
+#include "cache.h"
+#include "commit.h"
+#include "color.h"
@@ -51,8 +47,6 @@
+#include "diffcore.h"
+#include "revision.h"
+
-+#define HEADER_INDENT " "
-+
+enum collection_phase {
+ WORKTREE,
+ INDEX
@@ -75,66 +69,8 @@
+ struct hashmap file_map;
+};
+
-+static int use_color = -1;
-+enum color_add_i {
-+ COLOR_PROMPT,
-+ COLOR_HEADER,
-+ COLOR_HELP,
-+ COLOR_ERROR
-+};
-+
-+static char colors[][COLOR_MAXLEN] = {
-+ GIT_COLOR_BOLD_BLUE, /* Prompt */
-+ GIT_COLOR_BOLD, /* Header */
-+ GIT_COLOR_BOLD_RED, /* Help */
-+ GIT_COLOR_BOLD_RED /* Error */
-+};
-+
-+static const char *get_color(enum color_add_i ix)
-+{
-+ if (want_color(use_color))
-+ return colors[ix];
-+ return "";
-+}
-+
-+static int parse_color_slot(const char *slot)
-+{
-+ if (!strcasecmp(slot, "prompt"))
-+ return COLOR_PROMPT;
-+ if (!strcasecmp(slot, "header"))
-+ return COLOR_HEADER;
-+ if (!strcasecmp(slot, "help"))
-+ return COLOR_HELP;
-+ if (!strcasecmp(slot, "error"))
-+ return COLOR_ERROR;
-+
-+ return -1;
-+}
-+
-+int add_i_config(const char *var,
-+ const char *value, void *cbdata)
-+{
-+ const char *name;
-+
-+ if (!strcmp(var, "color.interactive")) {
-+ use_color = git_config_colorbool(var, value);
-+ return 0;
-+ }
-+
-+ if (skip_prefix(var, "color.interactive.", &name)) {
-+ int slot = parse_color_slot(name);
-+ if (slot < 0)
-+ return 0;
-+ if (!value)
-+ return config_error_nonbool(var);
-+ return color_parse(value, colors[slot]);
-+ }
-+
-+ return git_default_config(var, value, cbdata);
-+}
-+
+static int hash_cmp(const void *unused_cmp_data, const void *entry,
-+ const void *entry_or_key, const void *keydata)
++ const void *entry_or_key, const void *keydata)
+{
+ const struct file_stat *e1 = entry, *e2 = entry_or_key;
+ const char *name = keydata ? keydata : e2->name;
@@ -151,8 +87,8 @@
+}
+
+static void collect_changes_cb(struct diff_queue_struct *q,
-+ struct diff_options *options,
-+ void *data)
++ struct diff_options *options,
++ void *data)
+{
+ struct collection_status *s = data;
+ struct diffstat_t stat = { 0 };
@@ -222,128 +158,73 @@
+ run_diff_index(&rev, 1);
+}
+
-+void add_i_print_modified(void)
++static int is_inital_commit(void)
+{
-+ int i = 0;
-+ struct collection_status s;
-+ /* TRANSLATORS: you can adjust this to align "git add -i" status menu */
-+ const char *modified_fmt = _("%12s %12s %s");
-+ const char *header_color = get_color(COLOR_HEADER);
+ struct object_id sha1;
++ if (get_oid("HEAD", &sha1))
++ return 1;
++ return 0;
++}
++
++static const char *get_diff_reference(void)
++{
++ if(is_inital_commit())
++ return empty_tree_oid_hex();
++ return "HEAD";
++}
++
++static void filter_files(const char *filter, struct hashmap *file_map,
++ struct file_stat **files)
++{
++
++ for (int i = 0; i < hashmap_get_size(file_map); i++) {
++ struct file_stat *f = files[i];
++
++ if ((!(f->worktree.added || f->worktree.deleted)) &&
++ (!strcmp(filter, "file-only")))
++ hashmap_remove(file_map, f, NULL);
+
++ if ((!(f->index.added || f->index.deleted)) &&
++ (!strcmp(filter, "index-only")))
++ hashmap_remove(file_map, f, NULL);
++ }
++}
++
++static struct collection_status *list_modified(struct repository *r, const char *filter)
++{
++ int i = 0;
++ struct collection_status *s = xcalloc(1, sizeof(*s));
+ struct hashmap_iter iter;
+ struct file_stat **files;
+ struct file_stat *entry;
+
-+ if (read_cache() < 0)
-+ return;
++ if (repo_read_index(r) < 0) {
++ printf("\n");
++ return NULL;
++ }
+
-+ s.reference = !get_oid("HEAD", &sha1) ? "HEAD": empty_tree_oid_hex();
-+ hashmap_init(&s.file_map, hash_cmp, NULL, 0);
++ s->reference = get_diff_reference();
++ hashmap_init(&s->file_map, hash_cmp, NULL, 0);
+
-+ collect_changes_worktree(&s);
-+ collect_changes_index(&s);
++ collect_changes_worktree(s);
++ collect_changes_index(s);
+
-+ if (hashmap_get_size(&s.file_map) < 1) {
++ if (hashmap_get_size(&s->file_map) < 1) {
+ printf("\n");
-+ return;
++ return NULL;
+ }
+
-+ printf(HEADER_INDENT);
-+ color_fprintf(stdout, header_color, modified_fmt, _("staged"),
-+ _("unstaged"), _("path"));
-+ printf("\n");
-+
-+ hashmap_iter_init(&s.file_map, &iter);
++ hashmap_iter_init(&s->file_map, &iter);
+
-+ files = xcalloc(hashmap_get_size(&s.file_map), sizeof(struct file_stat *));
++ files = xcalloc(hashmap_get_size(&s->file_map), sizeof(struct file_stat *));
+ while ((entry = hashmap_iter_next(&iter))) {
+ files[i++] = entry;
+ }
-+ QSORT(files, hashmap_get_size(&s.file_map), alphabetical_cmp);
++ QSORT(files, hashmap_get_size(&s->file_map), alphabetical_cmp);
+
-+ for (i = 0; i < hashmap_get_size(&s.file_map); i++) {
-+ struct file_stat *f = files[i];
-+
-+ char worktree_changes[50];
-+ char index_changes[50];
-+
-+ if (f->worktree.added || f->worktree.deleted)
-+ snprintf(worktree_changes, 50, "+%"PRIuMAX"/-%"PRIuMAX, f->worktree.added,
-+ f->worktree.deleted);
-+ else
-+ snprintf(worktree_changes, 50, "%s", _("nothing"));
-+
-+ if (f->index.added || f->index.deleted)
-+ snprintf(index_changes, 50, "+%"PRIuMAX"/-%"PRIuMAX, f->index.added,
-+ f->index.deleted);
-+ else
-+ snprintf(index_changes, 50, "%s", _("unchanged"));
-+
-+ printf(" %2d: ", i + 1);
-+ printf(modified_fmt, index_changes, worktree_changes, f->name);
-+ printf("\n");
-+ }
-+ printf("\n");
++ if (filter)
++ filter_files(filter, &s->file_map, files);
+
+ free(files);
-+ hashmap_free(&s.file_map, 1);
++ return s;
+}
-
- diff --git a/add-interactive.h b/add-interactive.h
- new file mode 100644
- --- /dev/null
- +++ b/add-interactive.h
-@@
-+#ifndef ADD_INTERACTIVE_H
-+#define ADD_INTERACTIVE_H
-+
-+int add_i_config(const char *var, const char *value, void *cbdata);
-+
-+void add_i_print_modified(void);
-+
-+#endif
- \ No newline at end of file
-
- diff --git a/builtin/add--helper.c b/builtin/add--helper.c
- --- a/builtin/add--helper.c
- +++ b/builtin/add--helper.c
-@@
-+#include "add-interactive.h"
- #include "builtin.h"
-+#include "config.h"
-+#include "revision.h"
-+
-+static const char * const builtin_add_helper_usage[] = {
-+ N_("git add-interactive--helper <command>"),
-+ NULL
-+};
-+
-+enum cmd_mode {
-+ DEFAULT = 0,
-+ STATUS
-+};
-
- int cmd_add__helper(int argc, const char **argv, const char *prefix)
- {
-+ enum cmd_mode mode = DEFAULT;
-+
-+ struct option options[] = {
-+ OPT_CMDMODE(0, "status", &mode,
-+ N_("print status information with diffstat"), STATUS),
-+ OPT_END()
-+ };
-+
-+ git_config(add_i_config, NULL);
-+ argc = parse_options(argc, argv, NULL, options,
-+ builtin_add_helper_usage,
-+ PARSE_OPT_KEEP_ARGV0);
-+
-+ if (mode == STATUS)
-+ add_i_print_modified();
-+ else
-+ usage_with_options(builtin_add_helper_usage,
-+ options);
-+
- return 0;
- }
-: ---------- > 4: a97b29d274 add-interactive.c: implement list_and_choose
-: ---------- > 5: 9a72aabe6c add-interactive.c: implement status command
4: fb3f9378ac ! 6: 883963ee6e add--interactive.perl: use add--helper --status for status_cmd @@ -14,6 +14,7 @@
the fact that it would be hard to test, we'll pass on adding a regression test for this.
+ Mentored-by: Johannes Schindelin <[email protected]>
-: ---------- > 7: 7912f37517 add-interactive.c: add support for list_only option
5: ab16afd1d5 ! 8: 441321fc3d add-interactive.c: implement show-help command
@@ -10,31 +10,32 @@
handle_builtin and re-routed to the help command, without ever
calling cmd_add__helper().
+ Mentored-by: Johannes Schindelin <[email protected]>
Signed-off-by: Slavica Djukic <[email protected]>
diff --git a/add-interactive.c b/add-interactive.c
--- a/add-interactive.c
+++ b/add-interactive.c
@@
- free(files);
- hashmap_free(&s.file_map, 1);
+ free(s);
+ free_choices(&choices);
}
+
+void add_i_show_help(void)
+{
+ const char *help_color = get_color(COLOR_HELP);
+ color_fprintf_ln(stdout, help_color, "status - %s",
-+ _("show paths with changes"));
++ _("show paths with changes"));
+ color_fprintf_ln(stdout, help_color, "update - %s",
-+ _("add working tree state to the staged set of changes"));
++ _("add working tree state to the staged set of changes"));
+ color_fprintf_ln(stdout, help_color, "revert - %s",
-+ _("revert staged set of changes back to the HEAD version"));
++ _("revert staged set of changes back to the HEAD version"));
+ color_fprintf_ln(stdout, help_color, "patch - %s",
-+ _("pick hunks and update selectively"));
++ _("pick hunks and update selectively"));
+ color_fprintf_ln(stdout, help_color, "diff - %s",
-+ _("view diff between HEAD and index"));
++ _("view diff between HEAD and index"));
+ color_fprintf_ln(stdout, help_color, "add untracked - %s",
-+ _("add contents of untracked files to the staged set of changes"));
++ _("add contents of untracked files to the staged set of changes"));
+}
diff --git a/add-interactive.h b/add-interactive.h
@@ -42,13 +43,11 @@
+++ b/add-interactive.h
@@
- void add_i_print_modified(void);
+ void add_i_status(void);
--#endif
- \ No newline at end of file
+void add_i_show_help(void);
+
-+#endif
+ #endif
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
--- a/builtin/add--helper.c
@@ -66,16 +65,16 @@
@@
struct option options[] = {
OPT_CMDMODE(0, "status", &mode,
- N_("print status information with diffstat"), STATUS),
+ N_("print status information with diffstat"), STATUS),
+ OPT_CMDMODE(0, "show-help", &mode,
-+ N_("show help"), HELP),
++ N_("show help"), HELP),
OPT_END()
};
@@
if (mode == STATUS)
- add_i_print_modified();
+ add_i_status();
+ else if (mode == HELP)
+ add_i_show_help();
else 6: 0a27304a84 ! 9: 315ae8b211 t3701-add-interactive: test add_i_show_help()
@@ -11,6 +11,7 @@
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.
+ Mentored-by: Johannes Schindelin <[email protected]>
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
7: ca2a7c4375 ! 10: 2b4bdce730 add--interactive.perl: use add--helper --show-help for help_cmd
@@ -12,6 +12,7 @@
to print the numstat, also here we forgo adding a regression test:
the Perl script is on its way out (and this patch is part of that journey).
+ Mentored-by: Johannes Schindelin <[email protected]>
Signature · 1 line
gitgitgadget
PATCH v5 8/10 add-interactive.c: implement show-help command
From: Slavica Djukic <[email protected]>
Implement show-help command in add-interactive.c and use it in builtin add--helper.c.
Use command name "show-help" instead of "help": add--helper is builtin, hence add--helper --help would be intercepted by handle_builtin and re-routed to the help command, without ever calling cmd_add__helper().
--- add-interactive.c | 17 +++++++++++++++++ add-interactive.h | 2 ++ builtin/add--helper.c | 7 ++++++- 3 files changed, 25 insertions(+), 1 deletion(-)
add-interactive.c +17 -0
@@ -800,3 +800,20 @@ void add_i_status(void)free (s);free_choices (&choices); } + +void add_i_show_help (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf_ln (stdout, help_color,"status - %s" , +_ ("show paths with changes" )); +color_fprintf_ln (stdout, help_color,"update - %s" , +_ ("add working tree state to the staged set of changes" )); +color_fprintf_ln (stdout, help_color,"revert - %s" , +_ ("revert staged set of changes back to the HEAD version" )); +color_fprintf_ln (stdout, help_color,"patch - %s" , +_ ("pick hunks and update selectively" )); +color_fprintf_ln (stdout, help_color,"diff - %s" , +_ ("view diff between HEAD and index" )); +color_fprintf_ln (stdout, help_color,"add untracked - %s" , +_ ("add contents of untracked files to the staged set of changes" )); +}
add-interactive.h +2 -0
@@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata);void add_i_status (void ); +void add_i_show_help (void ); +#endif
builtin/add--helper.c +6 -1
@@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = {enum cmd_mode {DEFAULT =0 , -STATUS +STATUS , +HELP };int cmd_add__helper (int argc,const char **argv,const char *prefix)
@@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_CMDMODE (0 ,"status" , &mode,N_ ("print status information with diffstat" ),STATUS ), +OPT_CMDMODE (0 ,"show-help" , &mode, +N_ ("show help" ),HELP ),OPT_END () };
@@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix)if (mode ==STATUS )add_i_status (); +else if (mode ==HELP ) +add_i_show_help ();else usage_with_options (builtin_add_helper_usage, options);
Signature · 1 line
gitgitgadget
PATCH v5 10/10 add--interactive.perl: use add--helper --show-help for help_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Change help_cmd sub in git-add--interactive.perl to use show-help command from builtin add--helper.
If an error occurs, it will be reported, but the Perl script will not exit, since the add--helper is called within an eval block.
Just like the change where the Perl script calls the add--helper to print the numstat, also here we forgo adding a regression test:
the Perl script is on its way out (and this patch is part of that journey).
--- git-add--interactive.perl | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
git-add--interactive.perl +2 -10
@@ -1718,16 +1718,8 @@ sub quit_cmd { } sub help_cmd { -# TRANSLATORS: please do not translate the command names -# 'status', 'update', 'revert', etc. - print colored $help_color, __ <<'EOF' ; -status - show paths with changes -update - add working tree state to the staged set of changes -revert - revert staged set of changes back to the HEAD version -patch - pick hunks and update selectively -diff - view diff between HEAD and index -add untracked - add contents of untracked files to the staged set of changes -EOF + my @help_cmd = ("git", "add--helper", "--show-help"); + !system(@help_cmd) or die "@help_cmd exited with code $?"; } sub process_args {
Signature · 1 line
gitgitgadget
PATCH v5 9/10 t3701-add-interactive: test add_i_show_help()
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Add test to t3701-add-interactive to verify that add_i_show_help() outputs expected content.
Also, add it before changing git-add--interactive.perl's help_cmd to demonstrate that there are no changes introduced by the conversion to C.
Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows.
--- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
t/t3701-add-interactive.sh +24 -0
@@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-EOF && + + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>> <BOLD;RED>status - show paths with changes<RESET> + <BOLD;RED>update - add working tree state to the staged set of changes<RESET> + <BOLD;RED>revert - revert staged set of changes back to the HEAD version<RESET> + <BOLD;RED>patch - pick hunks and update selectively<RESET> + <BOLD;RED>diff - view diff between HEAD and index<RESET> + <BOLD;RED>add untracked - add contents of untracked files to the staged set of changes<RESET> + <BOLD>*** Commands ***<RESET> + 1: <BOLD;BLUE>s<RESET>tatus 2: <BOLD;BLUE>u<RESET>pdate 3: <BOLD;BLUE>r<RESET>evert 4: <BOLD;BLUE>a<RESET>dd untracked + 5: <BOLD;BLUE>p<RESET>atch 6: <BOLD;BLUE>d<RESET>iff 7: <BOLD;BLUE>q<RESET>uit 8: <BOLD;BLUE>h<RESET>elp + <BOLD;BLUE>What now<RESET>>$SP + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color <actual.colored >actual && + test_i18ncmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH v5 4/10 add-interactive.c: implement list_and_choose
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
This is one of the two functions translated from Perl script, besides list_modified that will be used by *_cmd functions (including status) to collect index/worktree changes, list those changes and let user make a choice.
At this point, it only prints worktree and index changes, but later on in this patch series there will be upgraded to highlighting unique prefixes of list items and let user pick those items.
--- add-interactive.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+)
add-interactive.c +127 -0
@@ -5,6 +5,8 @@#include "diffcore.h" #include "revision.h" +#define HEADER_INDENT " " +enum collection_phase {WORKTREE ,INDEX
@@ -27,6 +29,61 @@ struct collection_status {struct hashmap file_map; }; +struct list_and_choose_options { +int column_n; +unsigned singleton:1 ; +unsigned list_flat:1 ; +unsigned list_only:1 ; +unsigned list_only_file_names:1 ; +unsigned immediate:1 ; +char *header; +const char *prompt; +void (*on_eof_fn)(void ); +}; + +struct choice { +struct hashmap_entry e; +char type; +union { +void (*command_fn)(void ); +struct { +struct { +uintmax_t added, deleted; + } index, worktree; + } file; + } u; +size_t prefix_length; +const char *name; +}; + +struct choices { +struct choice **choices; +size_t alloc, nr; +}; +#define CHOICES_INIT { NULL, 0, 0 } + +static int use_color =-1 ; +enum color_add_i { +COLOR_PROMPT , +COLOR_HEADER , +COLOR_HELP , +COLOR_ERROR +}; + +static char colors[][COLOR_MAXLEN ] = { +GIT_COLOR_BOLD_BLUE ,/* Prompt */ +GIT_COLOR_BOLD ,/* Header */ +GIT_COLOR_BOLD_RED ,/* Help */ +GIT_COLOR_RESET /* Reset */ +}; + +static const char *get_color (enum color_add_i ix) +{ +if (want_color (use_color)) +return colors[ix]; +return "" ; +} +static int hash_cmp (const void *unused_cmp_data,const void *entry,const void *entry_or_key,const void *keydata) {
@@ -186,3 +243,73 @@ static struct collection_status *list_modified(struct repository *r, const charfree (files);return s; } + +static struct choices *list_and_choose (struct choices *data, +struct list_and_choose_options *opts) +{ +if (!data) +return NULL ; + +while (1 ) { +int last_lf =0 ; + +if (opts->header) { +const char *header_color =get_color (COLOR_HEADER ); +if (!opts->list_flat) +printf (HEADER_INDENT ); +color_fprintf_ln (stdout, header_color,"%s" , opts->header); + } + +for (int i =0 ; i < data->nr; i++) { +struct choice *c = data->choices[i]; +char *print; +const char *modified_fmt =_ ("%12s %12s %s" ); +char worktree_changes[50 ]; +char index_changes[50 ]; +char print_buf[100 ]; + + print = (char *)c->name; + +if ((data->choices[i]->type =='f' ) && (!opts->list_only_file_names)) { +uintmax_t worktree_added = c->u.file.worktree.added; +uintmax_t worktree_deleted = c->u.file.worktree.deleted; +uintmax_t index_added = c->u.file.index.added; +uintmax_t index_deleted = c->u.file.index.deleted; + +if (worktree_added || worktree_deleted) +snprintf (worktree_changes,50 ,"+%" PRIuMAX"/-%" PRIuMAX, + worktree_added, worktree_deleted); +else +snprintf (worktree_changes,50 ,"%s" ,_ ("nothing" )); + +if (index_added || index_deleted) +snprintf (index_changes,50 ,"+%" PRIuMAX"/-%" PRIuMAX, + index_added, index_deleted); +else +snprintf (index_changes,50 ,"%s" ,_ ("unchanged" )); + +snprintf (print_buf,100 , modified_fmt, index_changes, + worktree_changes, print); + print =xmalloc (strlen (print_buf) +1 ); +snprintf (print,100 ,"%s" , print_buf); + } + +printf (" %2d: %s" , i +1 , print); + +if ((opts->list_flat) && ((i +1 ) % (opts->column_n))) { +printf ("\t" ); + last_lf =0 ; + } +else { +printf ("\n" ); + last_lf =1 ; + } + + } + +if (!last_lf) +printf ("\n" ); + +return NULL ; + } +}
Signature · 1 line
gitgitgadget
PATCH v5 6/10 add--interactive.perl: use add--helper --status for status_cmd
In reply to Slavica Đukić via GitGitGadgetFrom: Daniel Ferreira <[email protected]>
Call the newly introduced add--helper builtin in status_cmd() instead of relying on add--interactive's Perl functions to print the numstat.
If an error occurs, it will be reported, but the Perl script will not exit, since the add--helper is called within an eval block.
As the Perl script will go away soon, so will this scenario, where the built-in helper is called from the Perl script. Combined with the fact that it would be hard to test, we'll pass on adding a regression test for this.
--- git-add--interactive.perl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
git-add--interactive.perl +2 -3
@@ -597,9 +597,8 @@ sub prompt_help_cmd { } sub status_cmd { - list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, - list_modified()); - print "\n"; + my @status_cmd = ("git", "add--helper", "--status"); + !system(@status_cmd) or die "@status_cmd exited with code $?"; } sub say_n_paths {
Signature · 1 line
gitgitgadget
PATCH v5 5/10 add-interactive.c: implement status command
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Implement add --interactive's status command in add-interactive.c and use it in builtin add--helper.c.
This is the first interactive add command implemented in C of those anticipated by the previous commit, which introduced
the add--helper built-in.
Implement additional helper functions dealing with struct choice and colors.
Use list_modified to get the data, and then pass it to list_and_choose to show it in appropriate format.
We're a bit lax with the command-line parsing, as the command is intended to be called only by one internal user: the add--interactive script.
--- add-interactive.c | 119 ++++++++++++++++++++++++++++++++++++++++++ add-interactive.h | 8 +++ builtin/add--helper.c | 32 ++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 add-interactive.h
add-interactive.c +119 -0
@@ -1,3 +1,4 @@ +#include "add-interactive.h" #include "cache.h" #include "commit.h" #include "color.h"
@@ -7,6 +8,8 @@#define HEADER_INDENT " " +#define HEADER_MAXLEN 30 +enum collection_phase {WORKTREE ,INDEX
@@ -29,6 +32,11 @@ struct collection_status {struct hashmap file_map; }; +struct command { +char *name; +void (*command_fn)(void ); +}; +struct list_and_choose_options {int column_n;unsigned singleton:1 ;
@@ -84,6 +92,42 @@ static const char *get_color(enum color_add_i ix)return "" ; } +static int parse_color_slot (const char *slot) +{ +if (!strcasecmp (slot,"prompt" )) +return COLOR_PROMPT ; +if (!strcasecmp (slot,"header" )) +return COLOR_HEADER ; +if (!strcasecmp (slot,"help" )) +return COLOR_HELP ; +if (!strcasecmp (slot,"error" )) +return COLOR_ERROR ; + +return -1 ; +} + +int add_i_config (const char *var, +const char *value,void *cbdata) +{ +const char *name; + +if (!strcmp (var,"color.interactive" )) { + use_color =git_config_colorbool (var, value); +return 0 ; + } + +if (skip_prefix (var,"color.interactive." , &name)) { +int slot =parse_color_slot (name); +if (slot <0 ) +return 0 ; +if (!value) +return config_error_nonbool (var); +return color_parse (value, colors[slot]); + } + +return git_default_config (var, value, cbdata); +} +static int hash_cmp (const void *unused_cmp_data,const void *entry,const void *entry_or_key,const void *keydata) {
@@ -313,3 +357,78 @@ static struct choices *list_and_choose(struct choices *data,return NULL ; } } + +static struct choice *make_choice (const char *name ) +{ +struct choice *choice; +FLEXPTR_ALLOC_STR (choice, name, name); +return choice; +} + +static struct choice *add_choice (struct choices *choices,const char type, +struct file_stat *file,struct command *command) +{ +struct choice *choice; +switch (type) { +case 'f' : + choice =make_choice (file->name); + choice->u.file.index.added = file->index.added; + choice->u.file.index.deleted = file->index.deleted; + choice->u.file.worktree.added = file->worktree.added; + choice->u.file.worktree.deleted = file->worktree.deleted; +break ; +case 'c' : + choice =make_choice (command->name); + choice->u.command_fn = command->command_fn; +break ; + } + choice->type = type; + +ALLOC_GROW (choices->choices, choices->nr +1 , choices->alloc); + choices->choices[choices->nr++] = choice; + +return choice; +} + +static void free_choices (struct choices *choices) +{ +int i; + +for (i =0 ; i < choices->nr; i++) +free (choices->choices[i]); +free (choices->choices); + choices->choices =NULL ; + choices->nr = choices->alloc =0 ; +} + +void add_i_status (void ) +{ +struct collection_status *s; +struct list_and_choose_options opts = {0 }; +struct hashmap *map; +struct hashmap_iter iter; +struct choices choices =CHOICES_INIT ; +struct file_stat *entry; +const char *modified_fmt =_ ("%12s %12s %s" ); +const char type ='f' ; + + opts.header =xmalloc (sizeof (char ) * (HEADER_MAXLEN +1 )); +snprintf (opts.header,HEADER_MAXLEN +1 , modified_fmt, +_ ("staged" ),_ ("unstaged" ),_ ("path" )); + + s =list_modified (the_repository,NULL ); +if (s ==NULL ) +return ; + + map = &s->file_map; +hashmap_iter_init (map, &iter); +while ((entry =hashmap_iter_next (&iter))) { +add_choice (&choices, type, entry,NULL ); + } + +list_and_choose (&choices, &opts); + +hashmap_free (&s->file_map,1 ); +free (s); +free_choices (&choices); +}
add-interactive.h new file +8 -0
@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_status (void ); + +#endif
builtin/add--helper.c +32 -0
@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_status (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Signature · 1 line
gitgitgadget
PATCH v5 7/10 add-interactive.c: add support for list_only option
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
If list_only option is not set, (i.e. we want to pick elements from the list, not just display them), highlight unique prefixes of list elements and let user make a choice as shown in prompt_help_cmd and singleton_prompt_help_cmd.
Input that is expected from user is full line. Although that's also the case with Perl script in this particular situation, there is also sub prompt_single_character, which deals with single keystroke.
Ever since f7a4cea25e3ee1c8f27777bc4293dca0210fa573,
we did not use _getch() in our code base's C code, and this would be the first user. There are portability issues with _getch() (or getch()) that we would like to avoid.
That said, from now on, every other input will also be expected to be full line, rather than single keystroke.
--- add-interactive.c | 378 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 373 insertions(+), 5 deletions(-)
add-interactive.c +373 -5
@@ -70,18 +70,27 @@ struct choices { };#define CHOICES_INIT { NULL, 0, 0 } +struct prefix_entry { +struct hashmap_entry e; +const char *name; +size_t prefix_length; +struct choice *item; +}; +static int use_color =-1 ;enum color_add_i {COLOR_PROMPT ,COLOR_HEADER ,COLOR_HELP , -COLOR_ERROR +COLOR_ERROR , +COLOR_RESET };static char colors[][COLOR_MAXLEN ] = {GIT_COLOR_BOLD_BLUE ,/* Prompt */ GIT_COLOR_BOLD ,/* Header */ GIT_COLOR_BOLD_RED ,/* Help */ +GIT_COLOR_BOLD_RED ,/* Error */ GIT_COLOR_RESET /* Reset */ };
@@ -102,6 +111,8 @@ static int parse_color_slot(const char *slot)return COLOR_HELP ;if (!strcasecmp (slot,"error" ))return COLOR_ERROR ; +if (!strcasecmp (slot,"reset" )) +return COLOR_RESET ;return -1 ; }
@@ -288,14 +299,248 @@ static struct collection_status *list_modified(struct repository *r, const charreturn s; } +static int map_cmp (const void *unused_cmp_data, +const void *entry, +const void *entry_or_key, +const void *unused_keydata) +{ +const struct choice *a = entry; +const struct choice *b = entry_or_key; +if ((a->prefix_length == b->prefix_length) && + (strncmp (a->name, b->name, a->prefix_length) ==0 )) +return 0 ; +return 1 ; +} + +static struct prefix_entry *new_prefix_entry (const char *name, +size_t prefix_length, +struct choice *item) +{ +struct prefix_entry *result =xcalloc (1 ,sizeof (*result)); + result->name = name; + result->prefix_length = prefix_length; + result->item = item; +hashmap_entry_init (result,memhash (name, prefix_length)); +return result; +} + +static void find_unique_prefixes (struct choices *data) +{ +int soft_limit =0 ; +int hard_limit =4 ; +struct hashmap map; + +hashmap_init (&map, map_cmp,NULL ,0 ); + +for (int i =0 ; i < data->nr; i++) { +struct prefix_entry *e =xcalloc (1 ,sizeof (*e)); +struct prefix_entry *e2; + e->name = data->choices[i]->name; + e->item = data->choices[i]; + +for (int j = soft_limit +1 ; j <= hard_limit; j++) { +if (!isascii (e->name[j])) +break ; + + e->prefix_length = j; +hashmap_entry_init (e,memhash (e->name, j)); + e2 =hashmap_get (&map, e,NULL ); +if (!e2) { + e->item->prefix_length = j; +hashmap_add (&map, e); + e =NULL ; +break ; + } + +if (!e2->item) { +continue ;/* non-unique prefix */ + } + +if (j != e2->item->prefix_length) +BUG ("Hashmap entry has unexpected prefix length (%" PRIuMAX"/ != %" PRIuMAX"/)" , + (uintmax_t )j, (uintmax_t )e2->item->prefix_length); + +/* skip common prefix */ +for (j++; j <= hard_limit && e->name[j -1 ]; j++) { +if (e->item->name[j -1 ] != e2->item->name[j -1 ]) +break ; +hashmap_add (&map,new_prefix_entry (e->name, j,NULL )); + } +if (j <= hard_limit && e2->name[j -1 ]) { + e2->item->prefix_length = j; +hashmap_add (&map,new_prefix_entry (e2->name, j, e2->item)); + } +else { + e2->item->prefix_length =0 ; + } + e2->item =NULL ; + +if (j <= hard_limit && e->name[j -1 ]) { + e->item->prefix_length = j; +hashmap_add (&map,new_prefix_entry (e->name, + e->item->prefix_length, e->item)); + e =NULL ; + } +else + e->item->prefix_length =0 ; +break ; + } + +free (e); + } +} + +static int find_unique (char *string,struct choices *data) +{ +int found =0 ; +int i =0 ; +int hit =0 ; + +for (i =0 ; i < data->nr; i++) { +struct choice *item = data->choices[i]; + hit =0 ; +if (!strcmp (item->name, string)) + hit =1 ; +if (hit && found) +return 0 ; +if (hit) + found = i +1 ; + } + +return found; +} + +/* filters out prefixes which have special meaning to list_and_choose() */ +static int is_valid_prefix (const char *prefix) +{ +regex_t *regex; +const char *pattern ="(\\s,)|(^-)|(^[0-9]+)" ; +int is_valid =0 ; + + regex =xmalloc (sizeof (*regex)); +if (regcomp (regex, pattern,REG_EXTENDED )) +return 0 ; + + is_valid = prefix && +regexec (regex, prefix,0 ,NULL ,0 ) && +strcmp (prefix,"*" ) && +strcmp (prefix,"?" ); +free (regex); +return is_valid; +} + +/* return a string with the prefix highlighted */ +/* for now use square brackets; later might use ANSI colors (underline, bold) */ +static char *highlight_prefix (struct choice *item) +{ +struct strbuf buf; +struct strbuf prefix; +struct strbuf remainder; +const char *prompt_color =get_color (COLOR_PROMPT ); +const char *reset_color =get_color (COLOR_RESET ); +int remainder_size =strlen (item->name) - item->prefix_length; + +strbuf_init (&buf,0 ); + +strbuf_init (&prefix,0 ); +strbuf_add (&prefix, item->name, item->prefix_length); + +strbuf_init (&remainder,0 ); +strbuf_add (&remainder, item->name + item->prefix_length, + remainder_size +1 ); + +if (!prefix.buf) { +strbuf_release (&buf); +strbuf_release (&prefix); +return remainder.buf; + } + +if (!is_valid_prefix (prefix.buf)) { +strbuf_addstr (&buf, prefix.buf); +strbuf_addstr (&buf, remainder.buf); + } +else if (!use_color) { +strbuf_addstr (&buf,"[" ); +strbuf_addstr (&buf, prefix.buf); +strbuf_addstr (&buf,"]" ); +strbuf_addstr (&buf, remainder.buf); + } +else { +strbuf_addstr (&buf, prompt_color); +strbuf_addstr (&buf, prefix.buf); +strbuf_addstr (&buf, reset_color); +strbuf_addstr (&buf, remainder.buf); + } + +strbuf_release (&prefix); +strbuf_release (&remainder); + +return buf.buf; +} + +static void singleton_prompt_help_cmd (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf_ln (stdout, help_color,"%s" ,_ ("Prompt help:" )); +color_fprintf_ln (stdout, help_color,"1 - %s" , +_ ("select a numbered item" )); +color_fprintf_ln (stdout, help_color,"foo - %s" , +_ ("select item based on unique prefix" )); +color_fprintf_ln (stdout, help_color," - %s" , +_ ("(empty) select nothing" )); +} + +static void prompt_help_cmd (void ) +{ +const char *help_color =get_color (COLOR_HELP ); +color_fprintf_ln (stdout, help_color,"%s" , +_ ("Prompt help:" )); +color_fprintf_ln (stdout, help_color,"1 - %s" , +_ ("select a single item" )); +color_fprintf_ln (stdout, help_color,"3-5 - %s" , +_ ("select a range of items" )); +color_fprintf_ln (stdout, help_color,"2-3,6-9 - %s" , +_ ("select multiple ranges" )); +color_fprintf_ln (stdout, help_color,"foo - %s" , +_ ("select item based on unique prefix" )); +color_fprintf_ln (stdout, help_color,"-... - %s" , +_ ("unselect specified items" )); +color_fprintf_ln (stdout, help_color,"* - %s" , +_ ("choose all items" )); +color_fprintf_ln (stdout, help_color," - %s" , +_ ("(empty) finish selecting" )); +} +static struct choices *list_and_choose (struct choices *data,struct list_and_choose_options *opts) { -if (!data) +char *chosen_choices =xcalloc (data->nr,sizeof (char *)); +struct choices *results =xcalloc (1 ,sizeof (*results)); +int chosen_size =0 ; + +if (!data) { +free (chosen_choices); +free (results);return NULL ; + } + +if (!opts->list_only) +find_unique_prefixes (data); +top:while (1 ) {int last_lf =0 ; +const char *prompt_color =get_color (COLOR_PROMPT ); +const char *error_color =get_color (COLOR_ERROR ); +struct strbuf input =STRBUF_INIT ; +struct strbuf choice; +struct strbuf token; +char *token_tmp; +regex_t *regex_dash_range; +regex_t *regex_number; +const char *pattern_dash_range; +const char *pattern_number; +const char delim[] =" ," ;if (opts->header) {const char *header_color =get_color (COLOR_HEADER );
@@ -306,13 +551,17 @@ static struct choices *list_and_choose(struct choices *data,for (int i =0 ; i < data->nr; i++) {struct choice *c = data->choices[i]; +char chosen = chosen_choices[i]?'*' :' ' ;char *print;const char *modified_fmt =_ ("%12s %12s %s" );char worktree_changes[50 ];char index_changes[50 ];char print_buf[100 ]; - print = (char *)c->name; +if (!opts->list_only) + print =highlight_prefix (data->choices[i]); +else + print = (char *)c->name;if ((data->choices[i]->type =='f' ) && (!opts->list_only_file_names)) {uintmax_t worktree_added = c->u.file.worktree.added;
@@ -338,7 +587,7 @@ static struct choices *list_and_choose(struct choices *data,snprintf (print,100 ,"%s" , print_buf); } -printf (" %2d: %s" , i +1 , print); +printf ("%c%2d: %s" , chosen, i +1 , print);if ((opts->list_flat) && ((i +1 ) % (opts->column_n))) {printf ("\t" );
@@ -354,8 +603,126 @@ static struct choices *list_and_choose(struct choices *data,if (!last_lf)printf ("\n" ); -return NULL ; +if (opts->list_only) +return NULL ; + +color_fprintf (stdout, prompt_color,"%s" , opts->prompt); +if (opts->singleton) +printf ("> " ); +else +printf (">> " ); + +fflush (stdout); +strbuf_getline (&input, stdin); +strbuf_trim (&input); + +if (!input.buf) +break ; + +if (!input.buf[0 ]) { +printf ("\n" ); +if (opts->on_eof_fn) + opts->on_eof_fn (); +break ; + } + +if (!strcmp (input.buf,"?" )) { + opts->singleton?singleton_prompt_help_cmd () :prompt_help_cmd (); + goto top; + } + + token_tmp =strtok (input.buf, delim); +strbuf_init (&token,0 ); +strbuf_add (&token, token_tmp,strlen (token_tmp)); + +while (1 ) { +int choose =1 ; +int bottom =0 , top =0 ; +strbuf_init (&choice,0 ); +strbuf_addbuf (&choice, &token); + +/* Input that begins with '-'; unchoose */ + pattern_dash_range ="^-" ; + regex_dash_range =xmalloc (sizeof (*regex_dash_range)); + +if (regcomp (regex_dash_range, pattern_dash_range,REG_EXTENDED )) +BUG ("regex compilation for pattern %s failed" , + pattern_dash_range); +if (!regexec (regex_dash_range, choice.buf,0 ,NULL ,0 )) { + choose =0 ; +/* remove dash from input */ +strbuf_remove (&choice,0 ,1 ); + } + +/* A range can be specified like 5-7 or 5-. */ + pattern_dash_range ="^([0-9]+)-([0-9]*)$" ; + pattern_number ="^[0-9]+$" ; + regex_number =xmalloc (sizeof (*regex_number)); + +if (regcomp (regex_dash_range, pattern_dash_range,REG_EXTENDED )) +BUG ("regex compilation for pattern %s failed" , + pattern_dash_range); +if (regcomp (regex_number, pattern_number,REG_EXTENDED )) +BUG ("regex compilation for pattern %s failed" , pattern_number); + +if (!regexec (regex_dash_range, choice.buf,0 ,NULL ,0 )) { +const char delim_dash[] ="-" ; +char *num =NULL ; + num =strtok (choice.buf, delim_dash); + bottom =atoi (num); + num =strtok (NULL , delim_dash); + top = num?atoi (num) : (1 + data->nr); + } +else if (!regexec (regex_number, choice.buf,0 ,NULL ,0 )) + bottom = top =atoi (choice.buf); +else if (!strcmp (choice.buf,"*" )) { + bottom =1 ; + top =1 + data->nr; + } +else { + bottom = top =find_unique (choice.buf, data); +if (!bottom) { +color_fprintf_ln (stdout, error_color,_ ("Huh (%s)?" ), choice.buf); + goto top; + } + } + +if (opts->singleton && bottom != top) { +color_fprintf_ln (stdout, error_color,_ ("Huh (%s)?" ), choice.buf); + goto top; + } + +for (int i = bottom -1 ; i <= top -1 ; i++) { +if (data->nr <= i || i <0 ) +continue ; + chosen_choices[i] = choose; +if (choose ==1 ) + chosen_size++; + } + +strbuf_release (&token); +strbuf_release (&choice); + + token_tmp =strtok (NULL , delim); +if (!token_tmp) +break ; +strbuf_init (&token,0 ); +strbuf_add (&token, token_tmp,strlen (token_tmp)); + } + +if ((opts->immediate) || !(strcmp (choice.buf,"*" ))) +break ; } + +for (int i =0 ; i < data->nr; i++) { +if (chosen_choices[i]) { +ALLOC_GROW (results->choices, results->nr +1 , results->alloc); + results->choices[results->nr++] = data->choices[i]; + } + } + +free (chosen_choices); +return results; }static struct choice *make_choice (const char *name )
@@ -412,6 +779,7 @@ void add_i_status(void)const char *modified_fmt =_ ("%12s %12s %s" );const char type ='f' ; + opts.list_only =1 ; opts.header =xmalloc (sizeof (char ) * (HEADER_MAXLEN +1 ));snprintf (opts.header,HEADER_MAXLEN +1 , modified_fmt,_ ("staged" ),_ ("unstaged" ),_ ("path" ));
Signature · 1 line
gitgitgadget
PATCH v5 3/10 add-interactive.c: implement list_modified
In reply to Slavica Đukić via GitGitGadgetFrom: Slavica Djukic <[email protected]>
Implement list_modified from Perl, which will be used by most of the *_cmd functions, including status in the following commit.
It lists a numstat comparing changed files between a) the worktree and the index; b) the index and the HEAD.
To do so, we use run_diff_index() and run_diff_files() to get changed files, use the diffstat API on them to get the numstat and use a combination of a hashmap and qsort() to print the result in O(n) + O(n lg n) complexity.
Add new file: add-interactive.c which will be used for implementing "application logic" of git add -i (alongside with add-interactive.h, added in later commit), whereas add--helper.c will be used mostly for parsing the command line.
--- Makefile | 1 + add-interactive.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+) create mode 100644 add-interactive.c
Makefile +1 -0
@@ -847,6 +847,7 @@ LIB_H = $(shell $(FIND) . \ -name '*.h' -print) LIB_OBJS += abspath.o +LIB_OBJS += add-interactive.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o
add-interactive.c new file +188 -0
188 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v5 01/10] diff: export diffstat interface
In reply to Daniel Ferreira via GitGitGadgetQuoted 4 lines · Daniel Ferreira via GitGitGadget
diff.c +0
-0@@ -6001,12 +5985,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat;
diff.c void diff_flush(struct diff_options *options) · line 5991
Quoted 10 lines
diff.c +1
-6-memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )
In the post-context of this hunk there are series of "if we are showing this kind of diffstat, pass &diffstat to a helper that shows it" calls, and this piece of code itself is guarded by "if we are showing any of these kinds of diffstat, enter this block". So a helper function that computes necessary data in &diffstat upfront does make sense and makes the code readable quite a lot.
But...
diff.c static int is_submodule_ignored(const char *path, struct diff_options *options) · line 6291
Quoted 4 lines
diff.c +4
-0+void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff;
... as a reusable helper, it would make a saner API if you did not to hardcode the dependency to the singleton diff_queued_diff (i.e. instead, pass a pointer to struct diff_queue_struct as a parameter---the caller has it as 'q' at the callsite).
Other than that, makes sense to me; thanks.
Here is a meta question, which is mostly meant to those who use gitgitgadget@. The person who wanted to send this copy of the patch this time (i.e. Slavica, not Daniel) is not shown anywhere on the header of the e-mail, so the response to the message will not go to Slavica at all, even though it probably is visible by monitoring where gitgitgadget@ delivers (i.e. the PR comments).
Is that desirable as a reviewee? I manually added Slavica's address taken from the S-o-b: line to this response just in case, but if it is not desirable, I'll stop doing so.
Thanks.
Re: [PATCH v5 02/10] add--helper: create builtin helper for interactive add
In reply to Daniel Ferreira via GitGitGadgetgit.c static int run_builtin(struct cmd_struct *p, int argc, const char **argv) · line 450
Quoted 12 lines · Daniel Ferreira via GitGitGadget
git.c +1
-0@@ -444,6 +444,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },
When adding a more complex replacement command we often cannot write RUN_SETUP nor NEED_WORK_TREE here and instead do the job ourselves in cmd_ourcommand() the hard way. But because this one is only for helping "add -i", we can let the git.c::run_builtin() aka "git potty" do the hard part, which is very nice.
Makes sense so far.
Thanks.
Re: [PATCH v5 03/10] add-interactive.c: implement list_modified
In reply to Slavica Djukic via GitGitGadgetQuoted 33 lines · Slavica Djukic via GitGitGadget
add-interactive.c new file +27
-0@@ -0,0 +1,188 @@ +#include "cache.h" +#include "commit.h" +#include "color.h" +#include "config.h" +#include "diffcore.h" +#include "revision.h" + +enum collection_phase { +WORKTREE , +INDEX +}; + +struct file_stat { +struct hashmap_entry ent; +struct { +uintmax_t added, deleted; + } index, worktree; +char name[FLEX_ARRAY ]; +}; + +struct collection_status { +enum collection_phase phase; + +const char *reference; +struct pathspec pathspec; + +struct hashmap file_map;
add-interactive.c line 37
Quoted 10 lines
add-interactive.c new file +10
-0+}; + +static int hash_cmp (const void *unused_cmp_data,const void *entry, +const void *entry_or_key,const void *keydata) +{ +const struct file_stat *e1 = entry, *e2 = entry_or_key; +const char *name = keydata ? keydata : e2->name; + +return strcmp (e1->name, name); +}
This one is used with hashmap API, which gives unfortunate name "cmp_fn" to its callback, which does not necessarily need to compare. It only needs to say "are they different (yes/no)?" without having to order them (iow, it would not break anything if the above is changed to return strcmp(name, e1->name)).
Rather than naming the callback after the name of the API that calls it (i.e. "hash"), it would make it more descriptive if it were named after what is being compared for equality, in this case, pathname. Perhaps "pathname_equal()" or something?
add-interactive.c line 45
Quoted 7 lines
add-interactive.c new file +7
-0+static int alphabetical_cmp (const void *a,const void *b) +{ +struct file_stat *f1 = *((struct file_stat **)a); +struct file_stat *f2 = *((struct file_stat **)b); + +return strcmp (f1->name, f2->name); +}
This one is used with qsort(), and there are two things you may want the function name to tell the readers. "what is compared" and "how it is compared". Alphabetical is a bit fuzzy (different people sort "a.txt", "A.txt", and "b.txt" in different order) and it gives a piece of information of secondary importance (i.e. it only starts to make sense after you know you are ordering pathnames), so perhaps "pathname_cmp()" or something may be a better name.
add-interactive.c line 58
Quoted 13 lines
add-interactive.c new file +13
-0+ +static void collect_changes_cb (struct diff_queue_struct *q, +struct diff_options *options, +void *data) +{ +struct collection_status *s = data; +struct diffstat_t stat = {0 }; +int i; + +if (!q->nr) +return ; + +compute_diffstat (options, &stat);
Remember what I said on patch 01/10? This callback function is designed to take arbitrary diff_queue_struct q as its parameter and work on it, but compute_diffstat() is misdesigned and cannot work on anything but the singleton diff_queued_diff, which is an impedance mismatch that can easily be corrected by going back to 01/10 and fixing it.
add-interactive.c line 70
Quoted 11 lines
add-interactive.c new file +11
-0+for (i =0 ; i < stat.nr; i++) { +struct file_stat *entry; +const char *name = stat.files[i]->name; +unsigned int hash =strhash (name); + + entry =hashmap_get_from_hash (&s->file_map, hash, name); +if (!entry) { +FLEX_ALLOC_STR (entry, name, name); +hashmap_entry_init (entry, hash); +hashmap_add (&s->file_map, entry); + }
The path may already be in the collection_status.file_map from the previous run when "diff-index --cached" is run, in which case we avoid adding it twice, which makes sense.
add-interactive.c line 78
Quoted 7 lines
add-interactive.c new file +7
-0+if (s->phase ==WORKTREE ) { + entry->worktree.added = stat.files[i]->added; + entry->worktree.deleted = stat.files[i]->deleted; + }else if (s->phase ==INDEX ) { + entry->index.added = stat.files[i]->added; + entry->index.deleted = stat.files[i]->deleted; + }
As the set of phases will not going to grow, not having the final 'else BUG("phase is neither WORKTREE nor INDEX");' here is OK.
But stepping back a bit, if we know we will not grow the phases, then it may be simpler *and* equally descriptive to rename .phase field to a boolean ".collecting_from_index" that literally means "are we collecting from the index?" and that way we can also get rid of the enum.
add-interactive.c line 91
Quoted 13 lines
add-interactive.c new file +13
-0+ } +} + +static void collect_changes_worktree (struct collection_status *s) +{ +struct rev_info rev; + + s->phase =WORKTREE ; + +init_revisions (&rev,NULL ); +setup_revisions (0 ,NULL , &rev,NULL ); + + rev.max_count =0 ;
The convention to (ab)use the max_count field to specify the unmerged stage, against which the working tree file is compared for an unmerged path, is one of the older quirks of the run_diff_files() API. "git diff-files" itself when running cmd_diff_files() leaves rev.max_count untouched to get a normal output (as opposed to the case when it is told to do --base/--ours/--theirs), so it ends up passing -1 in this field in such a case.
This explicit assignment to 0 must be explained with in-code comment why we want to do so.
add-interactive.c line 93
Quoted 1 line
add-interactive.c new file +1
-0+ rev.diffopt.flags.ignore_dirty_submodules =1 ;
OK, this matches what "sub list_modified" does.
Quoted 18 lines
add-interactive.c new file +18
-0+ rev.diffopt.output_format =DIFF_FORMAT_CALLBACK ; + rev.diffopt.format_callback = collect_changes_cb; + rev.diffopt.format_callback_data = s; +run_diff_files (&rev,0 ); +} + +static void collect_changes_index (struct collection_status *s) +{ +struct rev_info rev; +struct setup_revision_opt opt = {0 }; + + s->phase =INDEX ; + +init_revisions (&rev,NULL ); + opt.def = s->reference; +setup_revisions (0 ,NULL , &rev, &opt); + + rev.diffopt.output_format =DIFF_FORMAT_CALLBACK ;
add-interactive.c line 122
Quoted 10 lines
add-interactive.c new file +10
-0+ rev.diffopt.format_callback = collect_changes_cb; + rev.diffopt.format_callback_data = s; + +run_diff_index (&rev,1 ); +} + +static int is_inital_commit (void ) +{ +struct object_id sha1; +if (get_oid ("HEAD" , &sha1))
Using the newer get_oid() API (as opposed to get_sha1() API) while calling the variable that receives its result "sha1" is an oxymoron. Most users of the API call it "oid", I think.
add-interactive.c line 125
Quoted 3 lines
add-interactive.c new file +3
-0+return 1 ; +return 0 ; +}
inital???
Grep for "unborn" in the codebase. It probably makes sense to call it on_unborn_branch() instead.
static int on_unborn_branch(void)
{
struct object_id oid;
return !!get_oid("HEAD", &oid);
}Eventually, the users of "unborn" in sequencer.c and builtin/reset.c may want to share the implementation but the helper is so small that we probably should not worry about it until the topic is done and leave it for a later clean-up.
add-interactive.c line 129
Quoted 3 lines
add-interactive.c new file +3
-0+static const char *get_diff_reference (void ) +{ +if (is_inital_commit ())
Style. Do not forget SP between "if" and "(".
add-interactive.c line 132
Quoted 3 lines
add-interactive.c new file +3
-0+return empty_tree_oid_hex (); +return "HEAD" ; +}
Although probably
return on_unborn_branch() ? empty_tree_oid_hex() : "HEAD";
is clear enough.
add-interactive.c line 138
Quoted 5 lines
add-interactive.c new file +5
-0+static void filter_files (const char *filter,struct hashmap *file_map, +struct file_stat **files) +{ + +for (int i =0 ; i <hashmap_get_size (file_map); i++) {
The declaration of "int i" there is one of the C99ism we do not use, IIRC ("git grep 'for (int ' \*.c" comes up empty).
add-interactive.c line 148
Quoted 10 lines
add-interactive.c new file +10
-0+struct file_stat *f = files[i]; + +if ((!(f->worktree.added || f->worktree.deleted)) && + (!strcmp (filter,"file-only" ))) +hashmap_remove (file_map, f,NULL ); + +if ((!(f->index.added || f->index.deleted)) && + (!strcmp (filter,"index-only" ))) +hashmap_remove (file_map, f,NULL ); + }
As part of the "interactive" machinery, obvious inefficiency in the code may not matter that much, but having to make exact string comparison for an unmodified parameter twice for each iteration does not look like showing a good taste.
What is the earliest time the code can decide that it is asked to do a "file-only" filtering? What I am trying to get at is that if we know early enough, instead of grabbing outputs from "diff-files" and "diff-index --cached", only to drop such entries that do not have output in "diff-files", we could run only "diff-index --cached" without spending any cycle to run "diff-files" in the first place.
Even if such a reorganizing of the callflow is impossible (I didn't check), at least a micro optimization would be:
int i;
int file_only = !strcmp(filter, "file-only");
int index_only = !strcmp(filter, "index-only");
for (i = 0; i < hashmap_get_size(file_map), i++) {
struct file_stat *f = files[i];
if (file_only && !(f->worktree.added || f->worktree.deleted))
hashmap_remove(file_map, f, NULL);
...
}Quoted 26 lines
add-interactive.c new file +26
-0+} + +static struct collection_status *list_modified (struct repository *r,const char *filter) +{ +int i =0 ; +struct collection_status *s =xcalloc (1 ,sizeof (*s)); +struct hashmap_iter iter; +struct file_stat **files; +struct file_stat *entry; + +if (repo_read_index (r) <0 ) { +printf ("\n" ); +return NULL ; + } + + s->reference =get_diff_reference (); +hashmap_init (&s->file_map, hash_cmp,NULL ,0 ); + +collect_changes_worktree (s); +collect_changes_index (s); + +if (hashmap_get_size (&s->file_map) <1 ) { +printf ("\n" ); +return NULL ; + } +
add-interactive.c line 184
Quoted 10 lines
add-interactive.c new file +10
-0+hashmap_iter_init (&s->file_map, &iter); + + files =xcalloc (hashmap_get_size (&s->file_map),sizeof (struct file_stat *)); +while ((entry =hashmap_iter_next (&iter))) { + files[i++] = entry; + } +QSORT (files,hashmap_get_size (&s->file_map), alphabetical_cmp); + +if (filter) +filter_files (filter, &s->file_map, files);
Ahh, this answers my previous question, no? The "filter" parameter given to this function will never change, so we could inspect it even before we call collect_chagnes_{worktree,index}(). If the caller says "I want to see only the paths that appear in the diff-files output", there is no need to call collect_changes_index() at all, no?
Quoted 4 lines
add-interactive.c new file +4
-0+ +free (files); +return s; +}
Re: [PATCH v5 03/10] add-interactive.c: implement list_modified
Junio C Hamano <[email protected]> writes:
A few things I missed in the previous message.
Quoted 27 lines
add-interactive.c line 70
Quoted 11 lines
add-interactive.c new file +11
-0+for (i =0 ; i < stat.nr; i++) { +struct file_stat *entry; +const char *name = stat.files[i]->name; +unsigned int hash =strhash (name); + + entry =hashmap_get_from_hash (&s->file_map, hash, name); +if (!entry) { +FLEX_ALLOC_STR (entry, name, name); +hashmap_entry_init (entry, hash); +hashmap_add (&s->file_map, entry); + }The path may already be in the collection_status.file_map from the previous run when "diff-index --cached" is run, in which case we avoid adding it twice, which makes sense.
add-interactive.c line 78
Quoted 7 lines
add-interactive.c new file +7
-0+if (s->phase ==WORKTREE ) { + entry->worktree.added = stat.files[i]->added; + entry->worktree.deleted = stat.files[i]->deleted; + }else if (s->phase ==INDEX ) { + entry->index.added = stat.files[i]->added; + entry->index.deleted = stat.files[i]->deleted; + }As the set of phases will not going to grow, not having the final 'else
BUG("phase is neither WORKTREE nor INDEX");' here is OK.
Quoted 5 lines
But stepping back a bit, if we know we will not grow the phases, then it may be simpler *and* equally descriptive to rename .phase field to a boolean ".collecting_from_index" that literally means "are we collecting from the index?" and that way we can also get rid of the enum.
... so that this can become
if (s->collecting_from_index) {
entry->index.added = stat.files[i]->added;
entry->index.deleted = stat.files[i]->deleted;
} else {
...
}without "else if" and without having to worry about "what if phase is neither?".
Quoted 9 lines
Grep for "unborn" in the codebase. It probably makes sense to call it
on_unborn_branch()instead.static int on_unborn_branch(void) { struct object_id oid; return !!get_oid("HEAD", &oid); }
Quoted 4 lines
Eventually, the users of "unborn" in
sequencer.candbuiltin/reset.cmay want to share the implementation but the helper is so small that we probably should not worry about it until the topic is done and leave it for a later clean-up.
And before such a clean-up happens, the implementation of the helper would want to be improved. "Does 'rev-parse --verify HEAD' work?" was an easiest way to see if we are on an unborn branch from a script that works most of the time, but as we are rewriting it in C, we should use the more direct and correct API to see if "HEAD" is a symref, and if it points at a branch that does not yet exist, which is available in the refs API as resolve_ref_unsafe().
One issue with lazy use of get_oid("HEAD") is that the function dwims and tries to find HEAD in common hierarchies like .git/refs/heads/HEAD etc. when .git/HEAD does not work. We do not want such a dwimmery when seeing "are we on an unborn branch?".
Quoted 15 lines
Quoted 15 lines
add-interactive.c new file +15
-0+static struct collection_status *list_modified (struct repository *r,const char *filter) +{ +int i =0 ; +struct collection_status *s =xcalloc (1 ,sizeof (*s)); +struct hashmap_iter iter; +struct file_stat **files; +struct file_stat *entry; + +if (repo_read_index (r) <0 ) { +printf ("\n" ); +return NULL ; + } + + s->reference =get_diff_reference (); +hashmap_init (&s->file_map, hash_cmp,NULL ,0 );
Quoted 8 lines
add-interactive.c line 173
Quoted 8 lines
add-interactive.c new file +8
-0+ +collect_changes_worktree (s); +collect_changes_index (s); + +if (hashmap_get_size (&s->file_map) <1 ) { +printf ("\n" ); +return NULL ; + }
The non-error codepath of this function does not do any output, but we see two "just emit newline" before returning NUULL to signal an error to the caller" in the above. Such a printing from this level in the callchain (although we haven't seen callers of this function yet at this point in the series) is wrong. Presumably, the caller, when it obtains a non-NULL 's', does something useful and maybe as a part of the "useful" thing prints something to the standard output. Then the caller is also responsible for handling a NULL return. I.e. upon seeing such a NULL collection status, if the party that invoked the caller wants to see a single empty line for whatever reason (which in turn is a questionable practice, if you ask me, but at this point in the series we haven't seen what that invoker is doing, so for now lets assume that it is sane to want to see an empty line), the caller should do the putchar('\n').
Also, these two "return NULL" leaks 's'.
Quoted 16 lines · Slavica Đukić via GitGitGadget
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#uChanges since v4:
* rename
print_modifiedtolist_modifed
* the big change was implementinglist_and_choose, which resulted in code refactoring, i.e. separatinglist_modifiedandstatus_cmdand makingstatus_cmduse bothlist_modifiedandlist_and_choose
* implement struct choice instead of structstuff_itemas main data structure forlist_and_choose
* introducelist_onlyoption and implement support for !list_only users
* introduce highlighting of unique prefixes
Quoted 2 lines
Note that authorship handling is slightly changed. In some of the commits, I used Original-patch-by instead of listing Daniel Ferreira as author.
Sounds good to me.
Re: [PATCH v5 01/10] diff: export diffstat interface
In reply to Junio C HamanoHello Junio,
Thank you for suggestions and for taking time to look at this patch series.
Quoted 44 lines · Junio C Hamano
Quoted 4 lines · Daniel Ferreira via GitGitGadget
diff.c +0
-0@@ -6001,12 +5985,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) {struct diffstat_t diffstat;diff.c void diff_flush(struct diff_options *options) · line 5991
Quoted 10 lines
diff.c +1
-6-memset (&diffstat,0 ,sizeof (struct diffstat_t )); -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (check_pair_status (p)) -diff_flush_stat (p, options, &diffstat); - } +compute_diffstat (options, &diffstat);if (output_format &DIFF_FORMAT_NUMSTAT )show_numstat (&diffstat, options);if (output_format &DIFF_FORMAT_DIFFSTAT )In the post-context of this hunk there are series of "if we are showing this kind of diffstat, pass &diffstat to a helper that shows it" calls, and this piece of code itself is guarded by "if we are showing any of these kinds of diffstat, enter this block". So a helper function that computes necessary data in &diffstat upfront does make sense and makes the code readable quite a lot.
But...
diff.c static int is_submodule_ignored(const char *path, struct diff_options *options) · line 6291
Quoted 4 lines
diff.c +4
-0+void compute_diffstat (struct diff_options *options,struct diffstat_t *diffstat) +{ +int i; +struct diff_queue_struct *q = &diff_queued_diff;... as a reusable helper, it would make a saner API if you did not to hardcode the dependency to the singleton
diff_queued_diff(i.e. instead, pass a pointer to structdiff_queue_structas a parameter---the caller has it as 'q' at the callsite).Other than that, makes sense to me; thanks.
Here is a meta question, which is mostly meant to those who use gitgitgadget@. The person who wanted to send this copy of the patch this time (i.e. Slavica, not Daniel) is not shown anywhere on the header of the e-mail, so the response to the message will not go to Slavica at all, even though it probably is visible by monitoring where gitgitgadget@ delivers (i.e. the PR comments).
Quoted 3 lines
Is that desirable as a reviewee? I manually added Slavica's address taken from the S-o-b: line to this response just in case, but if it is not desirable, I'll stop doing so.
Since gitgitgadget puts reviewer's e-mails in PR comments and send e-mails (to me) as well, you don't need to add manually my address.
But thanks for thinking about this and doing it just in case I don't get e-mails.
Quoted 3 lines
Thanks.
Re: [PATCH v5 03/10] add-interactive.c: implement list_modified
In reply to Junio C HamanoQuoted 294 lines · Junio C Hamano
Quoted 33 lines · Slavica Djukic via GitGitGadget
add-interactive.c new file +27
-0@@ -0,0 +1,188 @@ +#include "cache.h" +#include "commit.h" +#include "color.h" +#include "config.h" +#include "diffcore.h" +#include "revision.h" + +enum collection_phase { +WORKTREE , +INDEX +}; + +struct file_stat { +struct hashmap_entry ent; +struct { +uintmax_t added, deleted; + } index, worktree; +char name[FLEX_ARRAY ]; +}; + +struct collection_status { +enum collection_phase phase; + +const char *reference; +struct pathspec pathspec; + +struct hashmap file_map;add-interactive.c line 37
Quoted 10 lines
add-interactive.c new file +10
-0+}; + +static int hash_cmp (const void *unused_cmp_data,const void *entry, +const void *entry_or_key,const void *keydata) +{ +const struct file_stat *e1 = entry, *e2 = entry_or_key; +const char *name = keydata ? keydata : e2->name; + +return strcmp (e1->name, name); +}This one is used with hashmap API, which gives unfortunate name "
cmp_fn" to its callback, which does not necessarily need to compare. It only needs to say "are they different (yes/no)?" without having to order them (iow, it would not break anything if the above is changed to returnstrcmp(name, e1->name)).Rather than naming the callback after the name of the API that calls it (i.e. "hash"), it would make it more descriptive if it were named after what is being compared for equality, in this case, pathname. Perhaps "
pathname_equal()" or something?add-interactive.c line 45
Quoted 7 lines
add-interactive.c new file +7
-0+static int alphabetical_cmp (const void *a,const void *b) +{ +struct file_stat *f1 = *((struct file_stat **)a); +struct file_stat *f2 = *((struct file_stat **)b); + +return strcmp (f1->name, f2->name); +}This one is used with
qsort(), and there are two things you may want the function name to tell the readers. "what is compared" and "how it is compared". Alphabetical is a bit fuzzy (different people sort "a.txt", "A.txt", and "b.txt" in different order) and it gives a piece of information of secondary importance (i.e. it only starts to make sense after you know you are ordering pathnames), so perhaps "pathname_cmp()" or something may be a better name.add-interactive.c line 58
Quoted 13 lines
add-interactive.c new file +13
-0+ +static void collect_changes_cb (struct diff_queue_struct *q, +struct diff_options *options, +void *data) +{ +struct collection_status *s = data; +struct diffstat_t stat = {0 }; +int i; + +if (!q->nr) +return ; + +compute_diffstat (options, &stat);Remember what I said on patch 01/10? This callback function is designed to take arbitrary
diff_queue_structq as its parameter and work on it, butcompute_diffstat()is misdesigned and cannot work on anything but the singletondiff_queued_diff, which is an impedance mismatch that can easily be corrected by going back to 01/10 and fixing it.add-interactive.c line 70
Quoted 11 lines
add-interactive.c new file +11
-0+for (i =0 ; i < stat.nr; i++) { +struct file_stat *entry; +const char *name = stat.files[i]->name; +unsigned int hash =strhash (name); + + entry =hashmap_get_from_hash (&s->file_map, hash, name); +if (!entry) { +FLEX_ALLOC_STR (entry, name, name); +hashmap_entry_init (entry, hash); +hashmap_add (&s->file_map, entry); + }The path may already be in the collection_status.file_map from the previous run when "diff-index --cached" is run, in which case we avoid adding it twice, which makes sense.
add-interactive.c line 78
Quoted 7 lines
add-interactive.c new file +7
-0+if (s->phase ==WORKTREE ) { + entry->worktree.added = stat.files[i]->added; + entry->worktree.deleted = stat.files[i]->deleted; + }else if (s->phase ==INDEX ) { + entry->index.added = stat.files[i]->added; + entry->index.deleted = stat.files[i]->deleted; + }As the set of phases will not going to grow, not having the final 'else
BUG("phase is neither WORKTREE nor INDEX");' here is OK.But stepping back a bit, if we know we will not grow the phases, then it may be simpler *and* equally descriptive to rename .phase field to a boolean ".collecting_from_index" that literally means "are we collecting from the index?" and that way we can also get rid of the enum.
add-interactive.c line 91
Quoted 13 lines
add-interactive.c new file +13
-0+ } +} + +static void collect_changes_worktree (struct collection_status *s) +{ +struct rev_info rev; + + s->phase =WORKTREE ; + +init_revisions (&rev,NULL ); +setup_revisions (0 ,NULL , &rev,NULL ); + + rev.max_count =0 ;The convention to (ab)use the
max_countfield to specify the unmerged stage, against which the working tree file is compared for an unmerged path, is one of the older quirks of therun_diff_files()API. "git diff-files" itself when runningcmd_diff_files()leaves rev.max_count untouched to get a normal output (as opposed to the case when it is told to do --base/--ours/--theirs), so it ends up passing -1 in this field in such a case.This explicit assignment to 0 must be explained with in-code comment why we want to do so.
add-interactive.c line 93
Quoted 1 line
add-interactive.c new file +1
-0+ rev.diffopt.flags.ignore_dirty_submodules =1 ;OK, this matches what "sub
list_modified" does.Quoted 18 lines
add-interactive.c new file +18
-0+ rev.diffopt.output_format =DIFF_FORMAT_CALLBACK ; + rev.diffopt.format_callback = collect_changes_cb; + rev.diffopt.format_callback_data = s; +run_diff_files (&rev,0 ); +} + +static void collect_changes_index (struct collection_status *s) +{ +struct rev_info rev; +struct setup_revision_opt opt = {0 }; + + s->phase =INDEX ; + +init_revisions (&rev,NULL ); + opt.def = s->reference; +setup_revisions (0 ,NULL , &rev, &opt); + + rev.diffopt.output_format =DIFF_FORMAT_CALLBACK ;add-interactive.c line 122
Quoted 10 lines
add-interactive.c new file +10
-0+ rev.diffopt.format_callback = collect_changes_cb; + rev.diffopt.format_callback_data = s; + +run_diff_index (&rev,1 ); +} + +static int is_inital_commit (void ) +{ +struct object_id sha1; +if (get_oid ("HEAD" , &sha1))Using the newer
get_oid()API (as opposed toget_sha1()API) while calling the variable that receives its result "sha1" is an oxymoron. Most users of the API call it "oid", I think.add-interactive.c line 125
Quoted 3 lines
add-interactive.c new file +3
-0+return 1 ; +return 0 ; +}inital???
Grep for "unborn" in the codebase. It probably makes sense to call it
on_unborn_branch()instead.static int on_unborn_branch(void) { struct object_id oid; return !!get_oid("HEAD", &oid); }Eventually, the users of "unborn" in
sequencer.candbuiltin/reset.cmay want to share the implementation but the helper is so small that we probably should not worry about it until the topic is done and leave it for a later clean-up.add-interactive.c line 129
Quoted 3 lines
add-interactive.c new file +3
-0+static const char *get_diff_reference (void ) +{ +if (is_inital_commit ())Style. Do not forget SP between "if" and "(".
add-interactive.c line 132
Quoted 3 lines
add-interactive.c new file +3
-0+return empty_tree_oid_hex (); +return "HEAD" ; +}Although probably
return on_unborn_branch() ? empty_tree_oid_hex() : "HEAD";is clear enough.
add-interactive.c line 138
Quoted 5 lines
add-interactive.c new file +5
-0+static void filter_files (const char *filter,struct hashmap *file_map, +struct file_stat **files) +{ + +for (int i =0 ; i <hashmap_get_size (file_map); i++) {The declaration of "int i" there is one of the C99ism we do not use, IIRC ("git grep 'for (int ' \*.c" comes up empty).
add-interactive.c line 148
Quoted 10 lines
add-interactive.c new file +10
-0+struct file_stat *f = files[i]; + +if ((!(f->worktree.added || f->worktree.deleted)) && + (!strcmp (filter,"file-only" ))) +hashmap_remove (file_map, f,NULL ); + +if ((!(f->index.added || f->index.deleted)) && + (!strcmp (filter,"index-only" ))) +hashmap_remove (file_map, f,NULL ); + }As part of the "interactive" machinery, obvious inefficiency in the code may not matter that much, but having to make exact string comparison for an unmodified parameter twice for each iteration does not look like showing a good taste.
What is the earliest time the code can decide that it is asked to do a "file-only" filtering? What I am trying to get at is that if we know early enough, instead of grabbing outputs from "diff-files" and "diff-index --cached", only to drop such entries that do not have output in "diff-files", we could run only "diff-index --cached" without spending any cycle to run "diff-files" in the first place.
Even if such a reorganizing of the callflow is impossible (I didn't check), at least a micro optimization would be:
int i; int file_only = !strcmp(filter, "file-only"); int index_only = !strcmp(filter, "index-only"); for (i = 0; i < hashmap_get_size(file_map), i++) { struct file_stat *f = files[i]; if (file_only && !(f->worktree.added || f->worktree.deleted)) hashmap_remove(file_map, f, NULL); ... }Quoted 34 lines
add-interactive.c new file +34
-0+} + +static struct collection_status *list_modified (struct repository *r,const char *filter) +{ +int i =0 ; +struct collection_status *s =xcalloc (1 ,sizeof (*s)); +struct hashmap_iter iter; +struct file_stat **files; +struct file_stat *entry; + +if (repo_read_index (r) <0 ) { +printf ("\n" ); +return NULL ; + } + + s->reference =get_diff_reference (); +hashmap_init (&s->file_map, hash_cmp,NULL ,0 ); + +collect_changes_worktree (s); +collect_changes_index (s); + +if (hashmap_get_size (&s->file_map) <1 ) { +printf ("\n" ); +return NULL ; + } + +hashmap_iter_init (&s->file_map, &iter); + + files =xcalloc (hashmap_get_size (&s->file_map),sizeof (struct file_stat *)); +while ((entry =hashmap_iter_next (&iter))) { + files[i++] = entry; + } +QSORT (files,hashmap_get_size (&s->file_map), alphabetical_cmp); +
Quoted 8 lines
add-interactive.c line 184
Quoted 2 lines
add-interactive.c new file +2
-0+if (filter) +filter_files (filter, &s->file_map, files);Ahh, this answers my previous question, no? The "filter" parameter given to this function will never change, so we could inspect it even before we call collect_chagnes_{worktree,index}(). If the caller says "I want to see only the paths that appear in the diff-files output", there is no need to call
collect_changes_index()at all, no?
Yes, this answers your previous question. The "filter" parameter doesn't change and we can collect specific changes right away instead of collecting all of them and applying filter after.
Thanks for this one and all the other comments.
Quoted 5 lines
Quoted 5 lines
add-interactive.c new file +4
-0+ +free (files); +return s; +}
Re: [PATCH v5 03/10] add-interactive.c: implement list_modified
In reply to Junio C HamanoQuoted 101 lines · Junio C Hamano
Junio C Hamano <[email protected]> writes:
A few things I missed in the previous message.
Quoted 25 lines
add-interactive.c line 70
Quoted 11 lines
add-interactive.c new file +11
-0+for (i =0 ; i < stat.nr; i++) { +struct file_stat *entry; +const char *name = stat.files[i]->name; +unsigned int hash =strhash (name); + + entry =hashmap_get_from_hash (&s->file_map, hash, name); +if (!entry) { +FLEX_ALLOC_STR (entry, name, name); +hashmap_entry_init (entry, hash); +hashmap_add (&s->file_map, entry); + }The path may already be in the collection_status.file_map from the previous run when "diff-index --cached" is run, in which case we avoid adding it twice, which makes sense.
add-interactive.c line 78
Quoted 7 lines
add-interactive.c new file +7
-0+if (s->phase ==WORKTREE ) { + entry->worktree.added = stat.files[i]->added; + entry->worktree.deleted = stat.files[i]->deleted; + }else if (s->phase ==INDEX ) { + entry->index.added = stat.files[i]->added; + entry->index.deleted = stat.files[i]->deleted; + }As the set of phases will not going to grow, not having the final 'else
BUG("phase is neither WORKTREE nor INDEX");' here is OK.Quoted 5 lines
But stepping back a bit, if we know we will not grow the phases, then it may be simpler *and* equally descriptive to rename .phase field to a boolean ".collecting_from_index" that literally means "are we collecting from the index?" and that way we can also get rid of the enum.
... so that this can become
if (s->collecting_from_index) { entry->index.added = stat.files[i]->added; entry->index.deleted = stat.files[i]->deleted; } else { ... }without "else if" and without having to worry about "what if phase is neither?".
Quoted 9 lines
Grep for "unborn" in the codebase. It probably makes sense to call it
on_unborn_branch()instead.static int on_unborn_branch(void) { struct object_id oid; return !!get_oid("HEAD", &oid); }Quoted 4 lines
Eventually, the users of "unborn" in
sequencer.candbuiltin/reset.cmay want to share the implementation but the helper is so small that we probably should not worry about it until the topic is done and leave it for a later clean-up.And before such a clean-up happens, the implementation of the helper would want to be improved. "Does 'rev-parse --verify HEAD' work?" was an easiest way to see if we are on an unborn branch from a script that works most of the time, but as we are rewriting it in C, we should use the more direct and correct API to see if "HEAD" is a symref, and if it points at a branch that does not yet exist, which is available in the refs API as
resolve_ref_unsafe().One issue with lazy use of
get_oid("HEAD")is that the function dwims and tries to find HEAD in common hierarchies like .git/refs/heads/HEAD etc. when .git/HEAD does not work. We do not want such a dwimmery when seeing "are we on an unborn branch?".Quoted 15 lines
Quoted 15 lines
add-interactive.c new file +15
-0+static struct collection_status *list_modified (struct repository *r,const char *filter) +{ +int i =0 ; +struct collection_status *s =xcalloc (1 ,sizeof (*s)); +struct hashmap_iter iter; +struct file_stat **files; +struct file_stat *entry; + +if (repo_read_index (r) <0 ) { +printf ("\n" ); +return NULL ; + } + + s->reference =get_diff_reference (); +hashmap_init (&s->file_map, hash_cmp,NULL ,0 );Quoted 8 lines
add-interactive.c line 173
Quoted 8 lines
add-interactive.c new file +8
-0+ +collect_changes_worktree (s); +collect_changes_index (s); + +if (hashmap_get_size (&s->file_map) <1 ) { +printf ("\n" ); +return NULL ; + }The non-error codepath of this function does not do any output, but we see two "just emit newline" before returning NUULL to signal an error to the caller" in the above. Such a printing from this level in the callchain (although we haven't seen callers of this function yet at this point in the series) is wrong. Presumably, the caller, when it obtains a non-NULL 's', does something useful and maybe as a
Quoted 8 lines
part of the "useful" thing prints something to the standard output. Then the caller is also responsible for handling a NULL return. I.e. upon seeing such a NULL collection status, if the party that invoked the caller wants to see a single empty line for whatever reason (which in turn is a questionable practice, if you ask me, but at this point in the series we haven't seen what that invoker is doing, so for now lets assume that it is sane to want to see an empty line), the caller should do the putchar('\n').
Caller of list_modified is add_i_status in the later commit, and yes it should definitely take care of this newline.
It should print it after calling list_modified *and* list_and_choose.
It is done in the same way in the Perl script:
sub status_cmd {
list_and_choose({ LIST_ONLY => 1, HEADER => $status_head }, list_modified());
print "\n";
}
I overlooked this because print_modified (now list_modified) in the previous iteration was equivalent of status command, and everything was handled there.
After refactoring, I missed to pick up those printfs.
Quoted 1 line
Also, these two "return NULL" leaks 's'.
I guess I missed some more things. Thank you.
Quoted 1 line
Re: [PATCH v5 04/10] add-interactive.c: implement list_and_choose
In reply to Slavica Djukic via GitGitGadgetQuoted 8 lines · Slavica Djukic via GitGitGadget
add-interactive.c +2
-0+#define HEADER_INDENT " " +enum collection_phase {WORKTREE ,INDEX @@ -27,6 +29,61 @@ struct collection_status {struct hashmap file_map; };
add-interactive.c struct collection_status { · line 40
Quoted 10 lines
add-interactive.c +9
-0+struct list_and_choose_options { +int column_n; +unsigned singleton:1 ; +unsigned list_flat:1 ; +unsigned list_only:1 ; +unsigned list_only_file_names:1 ; +unsigned immediate:1 ; +char *header; +const char *prompt;
Makes a reader wonder if "header" can also be const (not to be taken as a suggestion to bend backwards to make it so).
add-interactive.c struct collection_status { · line 46
Quoted 6 lines
add-interactive.c +6
-0+void (*on_eof_fn)(void ); +}; + +struct choice { +struct hashmap_entry e; +char type;
If this is for choosing among the member of union, possible value(s) for the type member and which value corresponds to which union member must be documented somewhere, perhaps as a comment around here.
Quoted 4 lines
add-interactive.c +4
-0+ union { + void (*command_fn)(void); + struct { + struct {
add-interactive.c struct collection_status { · line 60
Quoted 10 lines
add-interactive.c +10
-0+uintmax_t added, deleted; + } index, worktree; + } file; + } u; +size_t prefix_length; +const char *name; +}; + +struct choices { +struct choice **choices;
In general, do not name an array in plural. An exception is when the code mostly refers to the array as a whole.
When most accesses are to individual elements, then it would be a big win to be able to see choice[2] and pronounce it "the second choice" (you do not say "the second choices").
add-interactive.c struct collection_status { · line 73
Quoted 13 lines
add-interactive.c +13
-0+size_t alloc, nr; +}; +#define CHOICES_INIT { NULL, 0, 0 } + +static int use_color =-1 ; +enum color_add_i { +COLOR_PROMPT , +COLOR_HEADER , +COLOR_HELP , +COLOR_ERROR +}; + +static char colors[][COLOR_MAXLEN ] = {
Do not be overly selfish to assume that this will stay to be the only color pallette in this file. If this is the color palette for list_and_choose, then have it in its name, e.g. list_and_choose_color[] or something like that.
add-interactive.c struct collection_status { · line 78
Quoted 5 lines
add-interactive.c +5
-0+GIT_COLOR_BOLD_BLUE ,/* Prompt */ +GIT_COLOR_BOLD ,/* Header */ +GIT_COLOR_BOLD_RED ,/* Help */ + GIT_COLOR_RESET /* Reset */ +};
Is the above list of values and comments correct?
Doesn't each member of enum correspond to each element in list_and_choose_color[][COLOR_MAXLEN] array? It does not exactly match my intuition to have help text in red and error messages in plain color.
Quoted 8 lines
add-interactive.c +4
-0@@ -186,3 +243,73 @@ static struct collection_status *list_modified(struct repository *r, const charfree (files);return s; } + +static struct choices *list_and_choose (struct choices *data, +struct list_and_choose_options *opts) +{
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 259
Quoted 10 lines
add-interactive.c +10
-0+if (!data) +return NULL ; + +while (1 ) { +int last_lf =0 ; + +if (opts->header) { +const char *header_color =get_color (COLOR_HEADER ); +if (!opts->list_flat) +printf (HEADER_INDENT );
I won't complain if this is sufficient for the application, but the above would not allow different level of indentation depending on what header is being shown. It may make sense to get rid of list_flat boolean and instead allow a new "const char *header_indent" member in the opts structure supplied by the caller.
Don't use printf() when you _know_ you want to show a simple string without any % interpolation. fputs(HEADER_INDENT, stdout) would suffice.
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 263
Quoted 4 lines
add-interactive.c +4
-0+color_fprintf_ln (stdout, header_color,"%s" , opts->header); + } + +for (int i =0 ; i < data->nr; i++) {
We do not say "for (int i" (see a previous review).
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 269
Quoted 6 lines
add-interactive.c +6
-0+struct choice *c = data->choices[i]; +char *print; +const char *modified_fmt =_ ("%12s %12s %s" ); +char worktree_changes[50 ]; +char index_changes[50 ]; +char print_buf[100 ];
It appears that many of these variables are only needed inside "we are showing 'f' and not just names" block. Can their scope be narrowed?
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 271
Quoted 1 line
add-interactive.c +1
-0+ print = (char *)c->name;
Yuck. Stuff c->name into print_buf[] instead and get rid of "print" pointer.
Quoted 9 lines
add-interactive.c +9
-0+if ((data->choices[i]->type =='f' ) && (!opts->list_only_file_names)) { +uintmax_t worktree_added = c->u.file.worktree.added; +uintmax_t worktree_deleted = c->u.file.worktree.deleted; +uintmax_t index_added = c->u.file.index.added; +uintmax_t index_deleted = c->u.file.index.deleted; + +if (worktree_added || worktree_deleted) +snprintf (worktree_changes,50 ,"+%" PRIuMAX"/-%" PRIuMAX, + worktree_added, worktree_deleted);
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 292
Quoted 10 lines
add-interactive.c +10
-0+else +snprintf (worktree_changes,50 ,"%s" ,_ ("nothing" )); +if (index_added || index_deleted) +snprintf (index_changes,50 ,"+%" PRIuMAX"/-%" PRIuMAX, + index_added, index_deleted); +else +snprintf (index_changes,50 ,"%s" ,_ ("unchanged" )); + +snprintf (print_buf,100 , modified_fmt, index_changes, + worktree_changes, print);
All of the above look overly repetitious; a helper function that takes a pointer to "struct { uintmax_t a, d; }" and populates changes[] buffer would cut them down by half, but other than that I do not see a room for drastic improvement here X-<.
Oh, it would greatly help to use two strbuf for wt/ix_changes that are defined outside the loop that is strbuf_reset() after each iteration and use things like strbuf_addf().
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 294
Quoted 2 lines
add-interactive.c +2
-0+ print =xmalloc (strlen (print_buf) +1 ); +snprintf (print,100 ,"%s" , print_buf);
Likewise.
Quoted 10 lines
add-interactive.c +10
-0+ } + +printf (" %2d: %s" , i +1 , print); +if ((opts->list_flat) && ((i +1 ) % (opts->column_n))) { +printf ("\t" ); + last_lf =0 ; + } +else { +printf ("\n" ); + last_lf =1 ;
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 315
Quoted 10 lines
add-interactive.c +10
-0+ } + + } + +if (!last_lf) +printf ("\n" ); + +return NULL ; + } +}
This obviously only lists but does not let you choose at this step in the series, but that is OK.
But I see a deeper problem with the design of this helper. The things this helper can list is quite limited.
The original was designed so that the shown strings are prepared by the caller and this helper is solely responsible for showing the choices, giving prompt, and accepting choice (in various abbreviated forms), all _WITHOUT_ having to know the meaning of what is in the list. It gave us a much better separation of labor and responsibility between the caller and the callee, I would think.
Re: [PATCH v5 05/10] add-interactive.c: implement status command
In reply to Slavica Djukic via GitGitGadgetadd-interactive.c static const char *get_color(enum color_add_i ix) · line 104
Quoted 10 lines · Slavica Djukic via GitGitGadget
add-interactive.c +10
-0+static int parse_color_slot (const char *slot) +{ +if (!strcasecmp (slot,"prompt" )) +return COLOR_PROMPT ; +if (!strcasecmp (slot,"header" )) +return COLOR_HEADER ; +if (!strcasecmp (slot,"help" )) +return COLOR_HELP ; +if (!strcasecmp (slot,"error" )) +return COLOR_ERROR ;
As these are "color.interactive.<name>", matching case-insensitively is the right thing to do. Good.
If we would end up increasing the number of slots, we may need to switch to LOOKUP_CONFIG(), but for just four of them, this will do.
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 361
Quoted 6 lines
add-interactive.c +2
-0@@ -313,3 +357,78 @@ static struct choices *list_and_choose(struct choices *data,return NULL ; } } + +static struct choice *make_choice (const char *name )
Excess SP before ')'?
Review static struct choices *list_and_choose(struct choices *data, · line 363
Quoted 2 lines
(quoted diff) +2
-0+{ + struct choice *choice;
Style: have a blank line here to delineate decls (at the beginning of a function) and stmts (at all the decls).
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 371
Quoted 8 lines
add-interactive.c +8
-0+FLEXPTR_ALLOC_STR (choice, name, name); +return choice; +} + +static struct choice *add_choice (struct choices *choices,const char type, +struct file_stat *file,struct command *command) +{ +struct choice *choice;
Ditto here.
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 373
Quoted 2 lines
add-interactive.c +2
-0+switch (type) { +case 'f' :
Style: in our codebase, switch and case are indented to the same level, with the body of each case arm indented one more level.
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 378
Quoted 5 lines
add-interactive.c +5
-0+ choice =make_choice (file->name); + choice->u.file.index.added = file->index.added; + choice->u.file.index.deleted = file->index.deleted; + choice->u.file.worktree.added = file->worktree.added; + choice->u.file.worktree.deleted = file->worktree.deleted;
Would it make sense to make sure that all of file->index, u.file.index, file->wt, u.file.wt are exactly the same type of struct by introducing
struct adddel { uintmax_t add, del; };in a very early part of the series, and embed that structure as a member in "struct choice" and "struct file_stat"? That way, these assignments would become two structure assignments, that would be much easier to read.
Quoted 4 lines
add-interactive.c +4
-0+void add_i_status (void ) +{ +struct collection_status *s; +struct list_and_choose_options opts = {0 };
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 417
Quoted 10 lines
add-interactive.c +10
-0+struct hashmap *map; +struct hashmap_iter iter; +struct choices choices =CHOICES_INIT ; +struct file_stat *entry; +const char *modified_fmt =_ ("%12s %12s %s" ); +const char type ='f' ; + + opts.header =xmalloc (sizeof (char ) * (HEADER_MAXLEN +1 )); +snprintf (opts.header,HEADER_MAXLEN +1 , modified_fmt, +_ ("staged" ),_ ("unstaged" ),_ ("path" ));
Is there aversion to use of strbuf among your mentors?
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 429
Quoted 11 lines
add-interactive.c +11
-0+ s =list_modified (the_repository,NULL ); +if (s ==NULL ) +return ; + + map = &s->file_map; +hashmap_iter_init (map, &iter); +while ((entry =hashmap_iter_next (&iter))) { +add_choice (&choices, type, entry,NULL ); + } + +list_and_choose (&choices, &opts);
In what order are these filenames given? Whatever random order the hashmap happens to store them in?
I vaguely recall in an earlier step the code used hashmap to collect but at the end produced a sorted list out of the final result. Shouldn't we be iterating over that sorted list instead? Do we even need the hashmap at this point?
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 433
Quoted 3 lines
add-interactive.c +3
-0+hashmap_free (&s->file_map,1 ); +free (s); +free_choices (&choices);
Did we just leak opt.header?
Quoted 58 lines
(quoted diff) +1
-0+}add-interactive.h new file +8
-0@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_status (void ); + +#endif builtin/add--helper.c +32
-0@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_status (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Re: [PATCH v5 05/10] add-interactive.c: implement status command
Quoted 55 lines · Junio C Hamano
add-interactive.c static const char *get_color(enum color_add_i ix) · line 104
Quoted 10 lines · Slavica Djukic via GitGitGadget
add-interactive.c +10
-0+static int parse_color_slot (const char *slot) +{ +if (!strcasecmp (slot,"prompt" )) +return COLOR_PROMPT ; +if (!strcasecmp (slot,"header" )) +return COLOR_HEADER ; +if (!strcasecmp (slot,"help" )) +return COLOR_HELP ; +if (!strcasecmp (slot,"error" )) +return COLOR_ERROR ;As these are "color.interactive.<name>", matching case-insensitively is the right thing to do. Good.
If we would end up increasing the number of slots, we may need to switch to
LOOKUP_CONFIG(), but for just four of them, this will do.add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 361
Quoted 6 lines
add-interactive.c +2
-0@@ -313,3 +357,78 @@ static struct choices *list_and_choose(struct choices *data,return NULL ; } } + +static struct choice *make_choice (const char *name )Excess SP before ')'?
Review static struct choices *list_and_choose(struct choices *data, · line 363
Quoted 2 lines
(quoted diff) +2
-0+{ + struct choice *choice;Style: have a blank line here to delineate decls (at the beginning of a function) and stmts (at all the decls).
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 371
Quoted 8 lines
add-interactive.c +8
-0+FLEXPTR_ALLOC_STR (choice, name, name); +return choice; +} + +static struct choice *add_choice (struct choices *choices,const char type, +struct file_stat *file,struct command *command) +{ +struct choice *choice;Ditto here.
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 373
Quoted 2 lines
add-interactive.c +2
-0+switch (type) { +case 'f' :Style: in our codebase, switch and case are indented to the same level, with the body of each case arm indented one more level.
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 378
Quoted 5 lines
add-interactive.c +5
-0+ choice =make_choice (file->name); + choice->u.file.index.added = file->index.added; + choice->u.file.index.deleted = file->index.deleted; + choice->u.file.worktree.added = file->worktree.added; + choice->u.file.worktree.deleted = file->worktree.deleted;Would it make sense to make sure that all of
file->index, u.file.index,file->wt, u.file.wt are exactly the same type of struct by introducing
Quoted 6 lines
struct adddel { uintmax_t add, del; };in a very early part of the series, and embed that structure as a member in "struct choice" and "struct
file_stat"? That way, these assignments would become two structure assignments, that would be much easier to read.
Yes, definitely. I worked on this and it will be included in the next iteration.
Quoted 32 lines
Quoted 5 lines
add-interactive.c +4
-0+void add_i_status (void ) +{ +struct collection_status *s; +struct list_and_choose_options opts = {0 };add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 417
Quoted 10 lines
add-interactive.c +10
-0+struct hashmap *map; +struct hashmap_iter iter; +struct choices choices =CHOICES_INIT ; +struct file_stat *entry; +const char *modified_fmt =_ ("%12s %12s %s" ); +const char type ='f' ; + + opts.header =xmalloc (sizeof (char ) * (HEADER_MAXLEN +1 )); +snprintf (opts.header,HEADER_MAXLEN +1 , modified_fmt, +_ ("staged" ),_ ("unstaged" ),_ ("path" ));Is there aversion to use of strbuf among your mentors?
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 429
Quoted 11 lines
add-interactive.c +11
-0+ s =list_modified (the_repository,NULL ); +if (s ==NULL ) +return ; + + map = &s->file_map; +hashmap_iter_init (map, &iter); +while ((entry =hashmap_iter_next (&iter))) { +add_choice (&choices, type, entry,NULL ); + } + +list_and_choose (&choices, &opts);In what order are these filenames given? Whatever random order the hashmap happens to store them in?
Quoted 4 lines
I vaguely recall in an earlier step the code used hashmap to collect but at the end produced a sorted list out of the final result. Shouldn't we be iterating over that sorted list instead? Do we even need the hashmap at this point?
We actually don't need hashmap at this point, I've changed this so that list_modified returns produced sorted list.
I've also applied all other suggestions in this message. Thank you.
Quoted 63 lines
add-interactive.c static struct choices *list_and_choose(struct choices *data, · line 433
Quoted 3 lines
add-interactive.c +3
-0+hashmap_free (&s->file_map,1 ); +free (s); +free_choices (&choices);Did we just leak opt.header?
Quoted 58 lines
(quoted diff) +1
-0+}add-interactive.h new file +8
-0@@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +int add_i_config (const char *var,const char *value,void *cbdata); + +void add_i_status (void ); + +#endif builtin/add--helper.c +32
-0@@ -1,6 +1,38 @@ +#include "add-interactive.h" #include "builtin.h" +#include "config.h" +#include "revision.h" + +static const char *const builtin_add_helper_usage[] = { +N_ ("git add-interactive--helper <command>" ), +NULL +}; + +enum cmd_mode { +DEFAULT =0 , +STATUS +};int cmd_add__helper (int argc,const char **argv,const char *prefix) { +enum cmd_mode mode =DEFAULT ; + +struct option options[] = { +OPT_CMDMODE (0 ,"status" , &mode, +N_ ("print status information with diffstat" ),STATUS ), +OPT_END () + }; + +git_config (add_i_config,NULL ); + argc =parse_options (argc, argv,NULL , options, + builtin_add_helper_usage, +PARSE_OPT_KEEP_ARGV0 ); + +if (mode ==STATUS ) +add_i_status (); +else +usage_with_options (builtin_add_helper_usage, + options); +return 0 ; }
Re: [PATCH v5 04/10] add-interactive.c: implement list_and_choose
In reply to Junio C HamanoQuoted 80 lines · Junio C Hamano
Quoted 8 lines · Slavica Djukic via GitGitGadget
add-interactive.c +2
-0+#define HEADER_INDENT " " +enum collection_phase {WORKTREE ,INDEX @@ -27,6 +29,61 @@ struct collection_status {struct hashmap file_map; };add-interactive.c struct collection_status { · line 40
Quoted 10 lines
add-interactive.c +9
-0+struct list_and_choose_options { +int column_n; +unsigned singleton:1 ; +unsigned list_flat:1 ; +unsigned list_only:1 ; +unsigned list_only_file_names:1 ; +unsigned immediate:1 ; +char *header; +const char *prompt;Makes a reader wonder if "header" can also be const (not to be taken as a suggestion to bend backwards to make it so).
add-interactive.c struct collection_status { · line 46
Quoted 6 lines
add-interactive.c +6
-0+void (*on_eof_fn)(void ); +}; + +struct choice { +struct hashmap_entry e; +char type;If this is for choosing among the member of union, possible value(s) for the type member and which value corresponds to which union member must be documented somewhere, perhaps as a comment around here.
Quoted 4 lines
add-interactive.c +4
-0+ union { + void (*command_fn)(void); + struct { + struct {add-interactive.c struct collection_status { · line 60
Quoted 10 lines
add-interactive.c +10
-0+uintmax_t added, deleted; + } index, worktree; + } file; + } u; +size_t prefix_length; +const char *name; +}; + +struct choices { +struct choice **choices;In general, do not name an array in plural. An exception is when the code mostly refers to the array as a whole.
When most accesses are to individual elements, then it would be a big win to be able to see choice[2] and pronounce it "the second choice" (you do not say "the second choices").
add-interactive.c struct collection_status { · line 73
Quoted 13 lines
add-interactive.c +13
-0+size_t alloc, nr; +}; +#define CHOICES_INIT { NULL, 0, 0 } + +static int use_color =-1 ; +enum color_add_i { +COLOR_PROMPT , +COLOR_HEADER , +COLOR_HELP , +COLOR_ERROR +}; + +static char colors[][COLOR_MAXLEN ] = {Do not be overly selfish to assume that this will stay to be the only color pallette in this file. If this is the color palette for
list_and_choose, then have it in its name, e.g. list_and_choose_color[] or something like that.add-interactive.c struct collection_status { · line 78
Quoted 5 lines
add-interactive.c +5
-0+GIT_COLOR_BOLD_BLUE ,/* Prompt */ +GIT_COLOR_BOLD ,/* Header */ +GIT_COLOR_BOLD_RED ,/* Help */ + GIT_COLOR_RESET /* Reset */ +};Is the above list of values and comments correct?
Quoted 4 lines
Doesn't each member of enum correspond to each element in list_and_choose_color[][COLOR_MAXLEN] array? It does not exactly match my intuition to have help text in red and error messages in plain color.
I noticed I didn't add colors in corresponding commits, but list is correct -- later on in patch series there is
GIT_COLOR_BOLD_RED, /* Error*/
added so that error messages are shown in red.
Help text is also in red following up what is happening in git-add--interactive.perl.
Quoted 33 lines
Quoted 8 lines
add-interactive.c +4
-0@@ -186,3 +243,73 @@ static struct collection_status *list_modified(struct repository *r, const charfree (files);return s; } + +static struct choices *list_and_choose (struct choices *data, +struct list_and_choose_options *opts) +{add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 259
Quoted 10 lines
add-interactive.c +10
-0+if (!data) +return NULL ; + +while (1 ) { +int last_lf =0 ; + +if (opts->header) { +const char *header_color =get_color (COLOR_HEADER ); +if (!opts->list_flat) +printf (HEADER_INDENT );I won't complain if this is sufficient for the application, but the above would not allow different level of indentation depending on what header is being shown. It may make sense to get rid of
list_flatboolean and instead allow a new "const char*header_indent" member in the opts structure supplied by the caller.Don't use
printf()when you _know_ you want to show a simple string without any % interpolation.fputs(HEADER_INDENT, stdout)would suffice.add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 263
Quoted 4 lines
add-interactive.c +4
-0+color_fprintf_ln (stdout, header_color,"%s" , opts->header); + } + +for (int i =0 ; i < data->nr; i++) {We do not say "for (int i" (see a previous review).
Quoted 9 lines
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 269
Quoted 6 lines
add-interactive.c +6
-0+struct choice *c = data->choices[i]; +char *print; +const char *modified_fmt =_ ("%12s %12s %s" ); +char worktree_changes[50 ]; +char index_changes[50 ]; +char print_buf[100 ];It appears that many of these variables are only needed inside "we are showing 'f' and not just names" block. Can their scope be narrowed?
Yes, I will change this.
Quoted 63 lines
add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 271
Quoted 2 lines
add-interactive.c +1
-0+ print = (char *)c->name;Yuck. Stuff
c->nameinto print_buf[] instead and get rid of "print" pointer.Quoted 9 lines
add-interactive.c +9
-0+if ((data->choices[i]->type =='f' ) && (!opts->list_only_file_names)) { +uintmax_t worktree_added = c->u.file.worktree.added; +uintmax_t worktree_deleted = c->u.file.worktree.deleted; +uintmax_t index_added = c->u.file.index.added; +uintmax_t index_deleted = c->u.file.index.deleted; + +if (worktree_added || worktree_deleted) +snprintf (worktree_changes,50 ,"+%" PRIuMAX"/-%" PRIuMAX, + worktree_added, worktree_deleted);add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 292
Quoted 10 lines
add-interactive.c +10
-0+else +snprintf (worktree_changes,50 ,"%s" ,_ ("nothing" )); +if (index_added || index_deleted) +snprintf (index_changes,50 ,"+%" PRIuMAX"/-%" PRIuMAX, + index_added, index_deleted); +else +snprintf (index_changes,50 ,"%s" ,_ ("unchanged" )); + +snprintf (print_buf,100 , modified_fmt, index_changes, + worktree_changes, print);All of the above look overly repetitious; a helper function that takes a pointer to "struct {
uintmax_ta, d; }" and populates changes[] buffer would cut them down by half, but other than that I do not see a room for drastic improvement here X-<.Oh, it would greatly help to use two strbuf for wt/ix_changes that are defined outside the loop that is
strbuf_reset()after each iteration and use things likestrbuf_addf().add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 294
Quoted 2 lines
add-interactive.c +2
-0+ print =xmalloc (strlen (print_buf) +1 ); +snprintf (print,100 ,"%s" , print_buf);Likewise.
Quoted 10 lines
add-interactive.c +10
-0+ } + +printf (" %2d: %s" , i +1 , print); +if ((opts->list_flat) && ((i +1 ) % (opts->column_n))) { +printf ("\t" ); + last_lf =0 ; + } +else { +printf ("\n" ); + last_lf =1 ;add-interactive.c static struct collection_status *list_modified(struct repository *r, const char · line 315
Quoted 10 lines
add-interactive.c +10
-0+ } + + } + +if (!last_lf) +printf ("\n" ); + +return NULL ; + } +}This obviously only lists but does not let you choose at this step in the series, but that is OK.
But I see a deeper problem with the design of this helper. The things this helper can list is quite limited.
Quoted 7 lines
The original was designed so that the shown strings are prepared by the caller and this helper is solely responsible for showing the choices, giving prompt, and accepting choice (in various abbreviated forms), all _WITHOUT_ having to know the meaning of what is in the list. It gave us a much better separation of labor and responsibility between the caller and the callee, I would think.
Thanks for pointing this out. I talked to my mentor and I'm now working on making list_and_choose more "type-independent".
I didn't reply to all suggestions in this message, but I did apply them in the code.
Quoted 1 line
End of Outreachy internship
In reply to Slavica Đukić via GitGitGadgetHi everyone,
Today is officially the last day of my Outreachy internship. I wanted to say a few things about my experience and future of the project I worked on.
Project was "Turn git add -i into built-in" with Johannes Schindelin as my mentor.
I truly had amazing time, I learned so much about
coding and Git itself, went to Git Merge and FOSDEM and had a chance to meet great people.
I've never contributed before to open-source, and now that I did, I find it invaluable experience.
Although my obligations at this point don't allow me to work on the project, I will try my best to contribute in the future to Git or/and other great open-source projects.
That said, my mentor Johannes Schindelin will continue to work on the project from now on.
What is implemented in this project so far:
commands: status and help, helper functions like highlight_prefix(), find_unique_prefixes(), is_valid_prefix(), etc., but most importantly:list_and_choose() and list_modified().
Most of the add -i commands will use those two functions to collect, show the data and let the user make a choice.
What is left to be done:
In the current patch series, I got a lot of suggestion from Junio, and I applied all of them -- the only thing left is correcting list_and_choose more to make it type-independent.
In the future, all the other commands: update, revert, add untracked, patch, diff and quit.
I would like to thank all reviewers for useful suggestions and comprehension.
And thousand thanks to Johannes Schindelin, who turned out to be the most amazing mentor.
-Slavica Djukic
Quoted 515 lines · Slavica Đukić via GitGitGadget
This is the first version of a patch series to start porting git-add--interactive from Perl to C. Daniel Ferreira's patch series used as a head start:
https://public-inbox.org/git/[email protected]/t/#uChanges since v4:
* rename
print_modifiedtolist_modifed
* the big change was implementinglist_and_choose, which resulted in code refactoring, i.e. separatinglist_modifiedandstatus_cmdand makingstatus_cmduse bothlist_modifiedandlist_and_choose
* implement struct choice instead of structstuff_itemas main data structure forlist_and_choose
* introducelist_onlyoption and implement support for !list_only users
* introduce highlighting of unique prefixesNote that authorship handling is slightly changed. In some of the commits, I used Original-patch-by instead of listing Daniel Ferreira as author.
Also, I would like to point out that my Outreachy internship is going to finish on March 4 and I would really appreciate reviews before it does.
Daniel Ferreira (3):
diff: export diffstat interface
add--helper: create builtin helper for interactive add add--interactive.perl: use add--helper --status forstatus_cmdSlavica Djukic (7):
add-interactive.c: implementlist_modifiedadd-interactive.c: implementlist_and_chooseadd-interactive.c: implement status commandadd-interactive.c: add support forlist_onlyoptionadd-interactive.c: implement show-help command
t3701-add-interactive: testadd_i_show_help()add--interactive.perl: use add--helper --show-help forhelp_cmd.gitignore | 1 + Makefile | 2 + add-interactive.c | 819 +++++++++++++++++++++++++++++++++++++ add-interactive.h | 10 + builtin.h | 1 + builtin/add--helper.c | 43 ++ diff.c | 36 +- diff.h | 18 + git-add--interactive.perl | 17 +- git.c | 1 + t/t3701-add-interactive.sh | 24 ++ 11 files changed, 937 insertions(+), 35 deletions(-)create mode 100644
add-interactive.c
create mode 100644add-interactive.h
create mode 100644builtin/add--helper.cbase-commit:
ca1b4116483b397e78483376296bcd23916ab553
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/103Range-diff vs v4:
1: 737767b6f4 ! 1: d839f0c082 diff: export diffstat interface @@ -11,6 +11,7 @@ how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status. + Mentored-by: Johannes Schindelin <[email protected]> Signed-off-by: Daniel Ferreira <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> 2: 91b1963125 ! 2: 304c3863b1 add--helper: create builtin helper for interactive add @@ -2,8 +2,8 @@ add--helper: create builtin helper for interactive add - Create a builtin helper for git-add--interactive, which right now is not - able to do anything. + Create a builtin helper for git-add--interactive, which at this point + is not doing anything. This is the first step in an effort to convert git-add--interactive.perl to a C builtin, in search for better portability, expressibility and @@ -13,6 +13,7 @@ remove the last "big" Git script to have Perl as a dependency, allowing most Git users to have a NOPERL build running without big losses. + Mentored-by: Johannes Schindelin <[email protected]> Signed-off-by: Daniel Ferreira <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> 3: d247ef69fe ! 3: 8790ffaa39 add-interactive.c: implement status command @@ -1,17 +1,12 @@ -Author: Daniel Ferreira <[email protected]> +Author: Slavica Djukic <[email protected]> - add-interactive.c: implement status command + add-interactive.c: implement list_modified - Add new files: add-interactive.c and add-interactive.h, which - will be used for implementing "application logic" of git add -i, - whereas add--helper.c will be used mostly for parsing the command line. - We're a bit lax with the command-line parsing, as the command is - intended to be called only by one internal user: the add--interactive script. + Implement list_modified from Perl, which will be used + by most of the *_cmd functions, including status in the + following commit. - Implement add --interactive's status command in add-interactive.c and - use it in builtin add--helper.c. - - It prints a numstat comparing changed files between a) the worktree and + It lists a numstat comparing changed files between a) the worktree and the index; b) the index and the HEAD. To do so, we use run_diff_index() and run_diff_files() to get changed @@ -19,11 +14,13 @@ combination of a hashmap and qsort() to print the result in O(n) + O(n lg n) complexity. - This is the first interactive add command implemented in C of those - anticipated by the previous commit, which introduced - the add--helper built-in. + Add new file: add-interactive.c which will be used for implementing + "application logic" of git add -i (alongside with add-interactive.h, + added in later commit), whereas add--helper.c will be used mostly + for parsing the command line. - Signed-off-by: Daniel Ferreira <[email protected]> + Mentored-by: Johannes Schindelin <[email protected]> + Original-patch-by: Daniel Ferreira <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> diff --git a/Makefile b/Makefile @@ -43,7 +40,6 @@ --- /dev/null +++ b/add-interactive.c @@ -+#include "add-interactive.h" +#include "cache.h" +#include "commit.h" +#include "color.h" @@ -51,8 +47,6 @@ +#include "diffcore.h" +#include "revision.h" + -+#define HEADER_INDENT " " -+ +enum collection_phase { + WORKTREE, + INDEX @@ -75,66 +69,8 @@ + struct hashmap file_map; +}; + -+static int use_color = -1; -+enum color_add_i { -+ COLOR_PROMPT, -+ COLOR_HEADER, -+ COLOR_HELP, -+ COLOR_ERROR -+}; -+ -+static char colors[][COLOR_MAXLEN] = { -+ GIT_COLOR_BOLD_BLUE, /* Prompt */ -+ GIT_COLOR_BOLD, /* Header */ -+ GIT_COLOR_BOLD_RED, /* Help */ -+ GIT_COLOR_BOLD_RED /* Error */ -+}; -+ -+static const char *get_color(enum color_add_i ix) -+{ -+ if (want_color(use_color)) -+ return colors[ix]; -+ return ""; -+} -+ -+static int parse_color_slot(const char *slot) -+{ -+ if (!strcasecmp(slot, "prompt")) -+ return COLOR_PROMPT; -+ if (!strcasecmp(slot, "header")) -+ return COLOR_HEADER; -+ if (!strcasecmp(slot, "help")) -+ return COLOR_HELP; -+ if (!strcasecmp(slot, "error")) -+ return COLOR_ERROR; -+ -+ return -1; -+} -+ -+int add_i_config(const char *var, -+ const char *value, void *cbdata) -+{ -+ const char *name; -+ -+ if (!strcmp(var, "color.interactive")) { -+ use_color = git_config_colorbool(var, value); -+ return 0; -+ } -+ -+ if (skip_prefix(var, "color.interactive.", &name)) { -+ int slot = parse_color_slot(name); -+ if (slot < 0) -+ return 0; -+ if (!value) -+ return config_error_nonbool(var); -+ return color_parse(value, colors[slot]); -+ } -+ -+ return git_default_config(var, value, cbdata); -+} -+ +static int hash_cmp(const void *unused_cmp_data, const void *entry, -+ const void *entry_or_key, const void *keydata) ++ const void *entry_or_key, const void *keydata) +{ + const struct file_stat *e1 = entry, *e2 = entry_or_key; + const char *name = keydata ? keydata : e2->name; @@ -151,8 +87,8 @@ +} + +static void collect_changes_cb(struct diff_queue_struct *q, -+ struct diff_options *options, -+ void *data) ++ struct diff_options *options, ++ void *data) +{ + struct collection_status *s = data; + struct diffstat_t stat = { 0 }; @@ -222,128 +158,73 @@ + run_diff_index(&rev, 1); +} + -+void add_i_print_modified(void) ++static int is_inital_commit(void) +{ -+ int i = 0; -+ struct collection_status s; -+ /* TRANSLATORS: you can adjust this to align "git add -i" status menu */ -+ const char *modified_fmt = _("%12s %12s %s"); -+ const char *header_color = get_color(COLOR_HEADER); + struct object_id sha1; ++ if (get_oid("HEAD", &sha1)) ++ return 1; ++ return 0; ++} ++ ++static const char *get_diff_reference(void) ++{ ++ if(is_inital_commit()) ++ return empty_tree_oid_hex(); ++ return "HEAD"; ++} ++ ++static void filter_files(const char *filter, struct hashmap *file_map, ++ struct file_stat **files) ++{ ++ ++ for (int i = 0; i < hashmap_get_size(file_map); i++) { ++ struct file_stat *f = files[i]; ++ ++ if ((!(f->worktree.added || f->worktree.deleted)) && ++ (!strcmp(filter, "file-only"))) ++ hashmap_remove(file_map, f, NULL); + ++ if ((!(f->index.added || f->index.deleted)) && ++ (!strcmp(filter, "index-only"))) ++ hashmap_remove(file_map, f, NULL); ++ } ++} ++ ++static struct collection_status *list_modified(struct repository *r, const char *filter) ++{ ++ int i = 0; ++ struct collection_status *s = xcalloc(1, sizeof(*s)); + struct hashmap_iter iter; + struct file_stat **files; + struct file_stat *entry; + -+ if (read_cache() < 0) -+ return; ++ if (repo_read_index(r) < 0) { ++ printf("\n"); ++ return NULL; ++ } + -+ s.reference = !get_oid("HEAD", &sha1) ? "HEAD": empty_tree_oid_hex(); -+ hashmap_init(&s.file_map, hash_cmp, NULL, 0); ++ s->reference = get_diff_reference(); ++ hashmap_init(&s->file_map, hash_cmp, NULL, 0); + -+ collect_changes_worktree(&s); -+ collect_changes_index(&s); ++ collect_changes_worktree(s); ++ collect_changes_index(s); + -+ if (hashmap_get_size(&s.file_map) < 1) { ++ if (hashmap_get_size(&s->file_map) < 1) { + printf("\n"); -+ return; ++ return NULL; + } + -+ printf(HEADER_INDENT); -+ color_fprintf(stdout, header_color, modified_fmt, _("staged"), -+ _("unstaged"), _("path")); -+ printf("\n"); -+ -+ hashmap_iter_init(&s.file_map, &iter); ++ hashmap_iter_init(&s->file_map, &iter); + -+ files = xcalloc(hashmap_get_size(&s.file_map), sizeof(struct file_stat *)); ++ files = xcalloc(hashmap_get_size(&s->file_map), sizeof(struct file_stat *)); + while ((entry = hashmap_iter_next(&iter))) { + files[i++] = entry; + } -+ QSORT(files, hashmap_get_size(&s.file_map), alphabetical_cmp); ++ QSORT(files, hashmap_get_size(&s->file_map), alphabetical_cmp); + -+ for (i = 0; i < hashmap_get_size(&s.file_map); i++) { -+ struct file_stat *f = files[i]; -+ -+ char worktree_changes[50]; -+ char index_changes[50]; -+ -+ if (f->worktree.added || f->worktree.deleted) -+ snprintf(worktree_changes, 50, "+%"PRIuMAX"/-%"PRIuMAX, f->worktree.added, -+ f->worktree.deleted); -+ else -+ snprintf(worktree_changes, 50, "%s", _("nothing")); -+ -+ if (f->index.added || f->index.deleted) -+ snprintf(index_changes, 50, "+%"PRIuMAX"/-%"PRIuMAX, f->index.added, -+ f->index.deleted); -+ else -+ snprintf(index_changes, 50, "%s", _("unchanged")); -+ -+ printf(" %2d: ", i + 1); -+ printf(modified_fmt, index_changes, worktree_changes, f->name); -+ printf("\n"); -+ } -+ printf("\n"); ++ if (filter) ++ filter_files(filter, &s->file_map, files); + + free(files); -+ hashmap_free(&s.file_map, 1); ++ return s; +} - - diff --git a/add-interactive.h b/add-interactive.h - new file mode 100644 - --- /dev/null - +++ b/add-interactive.h -@@ -+#ifndef ADD_INTERACTIVE_H -+#define ADD_INTERACTIVE_H -+ -+int add_i_config(const char *var, const char *value, void *cbdata); -+ -+void add_i_print_modified(void); -+ -+#endif - \ No newline at end of file - - diff --git a/builtin/add--helper.c b/builtin/add--helper.c - --- a/builtin/add--helper.c - +++ b/builtin/add--helper.c -@@ -+#include "add-interactive.h" - #include "builtin.h" -+#include "config.h" -+#include "revision.h" -+ -+static const char * const builtin_add_helper_usage[] = { -+ N_("git add-interactive--helper <command>"), -+ NULL -+}; -+ -+enum cmd_mode { -+ DEFAULT = 0, -+ STATUS -+}; - - int cmd_add__helper(int argc, const char **argv, const char *prefix) - { -+ enum cmd_mode mode = DEFAULT; -+ -+ struct option options[] = { -+ OPT_CMDMODE(0, "status", &mode, -+ N_("print status information with diffstat"), STATUS), -+ OPT_END() -+ }; -+ -+ git_config(add_i_config, NULL); -+ argc = parse_options(argc, argv, NULL, options, -+ builtin_add_helper_usage, -+ PARSE_OPT_KEEP_ARGV0); -+ -+ if (mode == STATUS) -+ add_i_print_modified(); -+ else -+ usage_with_options(builtin_add_helper_usage, -+ options); -+ - return 0; - } -: ---------- > 4: a97b29d274 add-interactive.c: implement list_and_choose -: ---------- > 5: 9a72aabe6c add-interactive.c: implement status command 4: fb3f9378ac ! 6: 883963ee6e add--interactive.perl: use add--helper --status for status_cmd @@ -14,6 +14,7 @@ the fact that it would be hard to test, we'll pass on adding a regression test for this. + Mentored-by: Johannes Schindelin <[email protected]> Signed-off-by: Daniel Ferreira <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> -: ---------- > 7: 7912f37517 add-interactive.c: add support for list_only option 5: ab16afd1d5 ! 8: 441321fc3d add-interactive.c: implement show-help command @@ -10,31 +10,32 @@ handle_builtin and re-routed to the help command, without ever calling cmd_add__helper(). + Mentored-by: Johannes Schindelin <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> diff --git a/add-interactive.c b/add-interactive.c --- a/add-interactive.c +++ b/add-interactive.c @@ - free(files); - hashmap_free(&s.file_map, 1); + free(s); + free_choices(&choices); } + +void add_i_show_help(void) +{ + const char *help_color = get_color(COLOR_HELP); + color_fprintf_ln(stdout, help_color, "status - %s", -+ _("show paths with changes")); ++ _("show paths with changes")); + color_fprintf_ln(stdout, help_color, "update - %s", -+ _("add working tree state to the staged set of changes")); ++ _("add working tree state to the staged set of changes")); + color_fprintf_ln(stdout, help_color, "revert - %s", -+ _("revert staged set of changes back to the HEAD version")); ++ _("revert staged set of changes back to the HEAD version")); + color_fprintf_ln(stdout, help_color, "patch - %s", -+ _("pick hunks and update selectively")); ++ _("pick hunks and update selectively")); + color_fprintf_ln(stdout, help_color, "diff - %s", -+ _("view diff between HEAD and index")); ++ _("view diff between HEAD and index")); + color_fprintf_ln(stdout, help_color, "add untracked - %s", -+ _("add contents of untracked files to the staged set of changes")); ++ _("add contents of untracked files to the staged set of changes")); +} diff --git a/add-interactive.h b/add-interactive.h @@ -42,13 +43,11 @@ +++ b/add-interactive.h @@ - void add_i_print_modified(void); + void add_i_status(void); --#endif - \ No newline at end of file +void add_i_show_help(void); + -+#endif + #endif diff --git a/builtin/add--helper.c b/builtin/add--helper.c --- a/builtin/add--helper.c @@ -66,16 +65,16 @@ @@ struct option options[] = { OPT_CMDMODE(0, "status", &mode, - N_("print status information with diffstat"), STATUS), + N_("print status information with diffstat"), STATUS), + OPT_CMDMODE(0, "show-help", &mode, -+ N_("show help"), HELP), ++ N_("show help"), HELP), OPT_END() }; @@ if (mode == STATUS) - add_i_print_modified(); + add_i_status(); + else if (mode == HELP) + add_i_show_help(); else 6: 0a27304a84 ! 9: 315ae8b211 t3701-add-interactive: test add_i_show_help() @@ -11,6 +11,7 @@ Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100 to force colored output on Windows. + Mentored-by: Johannes Schindelin <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh 7: ca2a7c4375 ! 10: 2b4bdce730 add--interactive.perl: use add--helper --show-help for help_cmd @@ -12,6 +12,7 @@ to print the numstat, also here we forgo adding a regression test: the Perl script is on its way out (and this patch is part of that journey). + Mentored-by: Johannes Schindelin <[email protected]> Signed-off-by: Slavica Djukic <[email protected]> Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
Re: [PATCH v5 02/10] add--helper: create builtin helper for interactive add
In reply to Junio C HamanoHi Junio,
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Daniel Ferreira via GitGitGadget
git.c +0
-0@@ -444,6 +444,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
Quoted 12 lines
git.c static int run_builtin(struct cmd_struct *p, int argc, const char **argv) · line 450
Quoted 6 lines
git.c +1
-0static struct cmd_struct commands[] = { {"add" , cmd_add,RUN_SETUP |NEED_WORK_TREE }, + {"add--helper" , cmd_add__helper,RUN_SETUP |NEED_WORK_TREE }, {"am" , cmd_am,RUN_SETUP |NEED_WORK_TREE }, {"annotate" , cmd_annotate,RUN_SETUP |NO_PARSEOPT }, {"apply" , cmd_apply,RUN_SETUP_GENTLY },When adding a more complex replacement command we often cannot write
RUN_SETUPnorNEED_WORK_TREEhere and instead do the job ourselves incmd_ourcommand()the hard way. But because this one is only for helping "add -i", we can let thegit.c::run_builtin()aka "git potty" do the hard part, which is very nice.
Sadly, I ran into a quite curious (and seemingly equally insurmountable) problem with this approach: when I use system() to execute a Git built-in from the Perl script, and when that built-in then consumes even a single byte of stdin, that file descriptor gets closed in the Perl process, and no amount of file descriptor duplication and redirecting that I could come up helped. After calling the built-in helper to run, say, the update command, the main interactive add process always exited on me due to EOF on stdin, and I could not make it work.
To be sure, this happens only with MSYS2's Perl. If I try the same on Linux (WSL, really), everything works Just Fine.
So my current approach is actually the other way round: I started implementing the main loop in C, and implemented all commands except for patch so far, and I think I will just hand off to the Perl script for that command for now.
Unfortunately that means that my current state looks already quite a bit different than what you reviewed. My hope is, however, that it will make for a nice "story" once I have a version to show that I think could be applied as-is.
Ciao,
Dscho