diff mbox series

[v3,11/11] built-in add -i: implement the `help` command

Message ID db70c6475d85dd77385d773274fa390fa7ed08c0.1563289115.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series git add -i: add a rudimentary version in C (supporting only status and help so far) | expand

Commit Message

Johannes Schindelin via GitGitGadget July 16, 2019, 2:58 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

This imitates the code to show the help text from the Perl script
`git-add--interactive.perl` in the built-in version.

To make sure that it renders exactly like the Perl version of `git add
-i`, we also add a test case for that to `t3701-add-interactive.sh`.

Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c          | 27 +++++++++++++++++++++++++--
 t/t3701-add-interactive.sh | 25 +++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Aug. 2, 2019, 9:04 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +static int run_help(struct add_i_state *s, const struct pathspec *ps,
> +		    struct file_list *files, struct list_options *opts)
> +{
> +	const char *help_color = s->help_color;
> +
> +	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"));

As we do not allow the command names to get translated, this makes
sense.

Have we adopted the convention to name callback parameters that have
to stay unused (because the callback function must have a function
signature that accepts the union of what everybody needs to take)
differently from the parameters that actually get used?  It may make
sense to use it in a function like this, to prevent readers from
wasting time by wondering how pathspec is used in this function, for
example.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 69991a3168..cf67756b85 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -647,4 +647,29 @@ test_expect_success 'checkout -p works with pathological context lines' '
>  	test_write_lines a b a b a a b a b a >expect &&
>  	test_cmp expect a
>  '
> +
> +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
> +'

Nicely done.
Jeff King Aug. 2, 2019, 10:26 p.m. UTC | #2 
On Fri, Aug 02, 2019 at 02:04:09PM -0700, Junio C Hamano wrote:

> > +static int run_help(struct add_i_state *s, const struct pathspec *ps,
> > +		    struct file_list *files, struct list_options *opts)
> [...]
> 
> As we do not allow the command names to get translated, this makes
> sense.
> 
> Have we adopted the convention to name callback parameters that have
> to stay unused (because the callback function must have a function
> signature that accepts the union of what everybody needs to take)
> differently from the parameters that actually get used?  It may make
> sense to use it in a function like this, to prevent readers from
> wasting time by wondering how pathspec is used in this function, for
> example.

I haven't yet[1] polished up the remainder of my patches to make us
-Wunused-parameter clean, but the pattern there would look like:

  void some_function(const char *foo, void *UNUSED(bar))
  {
     ... use foo but not bar ...
  }

which both tells the compiler that "bar" may be unused, and renames it
behind the scenes to unused_bar so that it cannot be accidentally used
(or more importantly, so that we can drop the annotation when it does
get used).

All of which is to say that I'm fine if you call it "unused_bar"
manually for now, but I'd switch it to the above in my series. So it may
not matter all that much in the meantime.

-Peff

[1] The sticking point is a few more cases I found where it's unclear to
    me whether they should be marked, or if it's a latent bug.
diff mbox series

Patch

diff --git a/add-interactive.c b/add-interactive.c
index 538658dfa7..c431c72e3f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -428,6 +428,27 @@  static int run_status(struct add_i_state *s, const struct pathspec *ps,
 	return 0;
 }
 
+static int run_help(struct add_i_state *s, const struct pathspec *ps,
+		    struct file_list *files, struct list_options *opts)
+{
+	const char *help_color = s->help_color;
+
+	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"));
+
+	return 0;
+}
+
 struct print_command_item_data {
 	const char *color, *reset;
 };
@@ -473,9 +494,11 @@  int run_add_i(struct repository *r, const struct pathspec *ps)
 		N_("What now"), command_prompt_help
 	};
 	struct command_item
-		status = { { "status" }, run_status };
+		status = { { "status" }, run_status },
+		help = { { "help" }, run_help };
 	struct command_item *commands[] = {
-		&status
+		&status,
+		&help
 	};
 
 	struct print_file_item_data print_file_item_data = {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 69991a3168..cf67756b85 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -647,4 +647,29 @@  test_expect_success 'checkout -p works with pathological context lines' '
 	test_write_lines a b a b a a b a b a >expect &&
 	test_cmp expect a
 '
+
+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