git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 1/7] diff: export diffstat interface
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
@ 2018-12-20 12:09 ` Daniel Ferreira via GitGitGadget
  2018-12-20 12:09 ` [PATCH 2/7] add--helper: create builtin helper for interactive add Daniel Ferreira via GitGitGadget
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

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.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 diff.c | 36 ++++++++++++++----------------------
 diff.h | 18 ++++++++++++++++++
 2 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/diff.c b/diff.c
index dc9965e836..46a7d8cf29 100644
--- a/diff.c
+++ b/diff.c
@@ -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 --git a/diff.h b/diff.h
index ce5e8a8183..7809db3039 100644
--- a/diff.h
+++ b/diff.h
@@ -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
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 2/7] add--helper: create builtin helper for interactive add
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
  2018-12-20 12:09 ` [PATCH 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
@ 2018-12-20 12:09 ` Daniel Ferreira via GitGitGadget
  2018-12-20 12:09 ` [PATCH 3/7] add-interactive.c: implement status command Daniel Ferreira via GitGitGadget
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

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.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 .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

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..2ee71ed217 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,6 +15,7 @@
 /git
 /git-add
 /git-add--interactive
+/git-add--helper
 /git-am
 /git-annotate
 /git-apply
diff --git a/Makefile b/Makefile
index 1a44c811aa..9c84b80739 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/builtin.h b/builtin.h
index 6538932e99..dd811ef7d5 100644
--- a/builtin.h
+++ b/builtin.h
@@ -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);
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
new file mode 100644
index 0000000000..6a97f0e191
--- /dev/null
+++ b/builtin/add--helper.c
@@ -0,0 +1,6 @@
+#include "builtin.h"
+
+int cmd_add__helper(int argc, const char **argv, const char *prefix)
+{
+	return 0;
+}
diff --git a/git.c b/git.c
index 2f604a41ea..5507591f2e 100644
--- a/git.c
+++ b/git.c
@@ -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 },
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 3/7] add-interactive.c: implement status command
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
  2018-12-20 12:09 ` [PATCH 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
  2018-12-20 12:09 ` [PATCH 2/7] add--helper: create builtin helper for interactive add Daniel Ferreira via GitGitGadget
@ 2018-12-20 12:09 ` Daniel Ferreira via GitGitGadget
  2018-12-20 12:09 ` [PATCH 4/7] add--interactive.perl: use add--helper --status for status_cmd Daniel Ferreira via GitGitGadget
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

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.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 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

diff --git a/Makefile b/Makefile
index 9c84b80739..2a4a5cc37b 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/add-interactive.c b/add-interactive.c
new file mode 100644
index 0000000000..c55d934186
--- /dev/null
+++ b/add-interactive.c
@@ -0,0 +1,246 @@
+#include "add-interactive.h"
+#include "cache.h"
+#include "commit.h"
+#include "color.h"
+#include "config.h"
+#include "diffcore.h"
+#include "revision.h"
+
+#define HEADER_INDENT "      "
+
+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;
+};
+
+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 struct file_stat *e1 = entry, *e2 = entry_or_key;
+	const char *name = keydata ? keydata : e2->name;
+
+	return strcmp(e1->name, name);
+}
+
+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);
+}
+
+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);
+
+	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);
+		}
+
+		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;
+		}
+	}
+}
+
+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;
+
+	rev.diffopt.flags.ignore_dirty_submodules = 1;
+	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;
+	rev.diffopt.format_callback = collect_changes_cb;
+	rev.diffopt.format_callback_data = s;
+
+	run_diff_index(&rev, 1);
+}
+
+void add_i_print_modified(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;
+
+	struct hashmap_iter iter;
+	struct file_stat **files;
+	struct file_stat *entry;
+
+	if (read_cache() < 0)
+		return;
+
+	s.reference = !get_oid("HEAD", &sha1) ? "HEAD": empty_tree_oid_hex();
+	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;
+	}
+
+	printf(HEADER_INDENT);
+	color_fprintf(stdout, header_color, modified_fmt, _("staged"),
+			_("unstaged"), _("path"));
+	printf("\n");
+
+	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);
+
+	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");
+
+	free(files);
+	hashmap_free(&s.file_map, 1);
+}
diff --git a/add-interactive.h b/add-interactive.h
new file mode 100644
index 0000000000..1f4747553c
--- /dev/null
+++ b/add-interactive.h
@@ -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
\ No newline at end of file
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
index 6a97f0e191..43545d9af5 100644
--- a/builtin/add--helper.c
+++ b/builtin/add--helper.c
@@ -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;
 }
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 4/7] add--interactive.perl: use add--helper --status for status_cmd
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (2 preceding siblings ...)
  2018-12-20 12:09 ` [PATCH 3/7] add-interactive.c: implement status command Daniel Ferreira via GitGitGadget
@ 2018-12-20 12:09 ` Daniel Ferreira via GitGitGadget
  2018-12-20 12:09 ` [PATCH 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

Call the newly introduced add--helper builtin on
status_cmd() instead of relying on add--interactive's Perl
functions to build print the numstat.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 git-add--interactive.perl | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 20eb81cc92..a6536f9cf3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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 {
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 5/7] add-interactive.c: implement show-help command
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (3 preceding siblings ...)
  2018-12-20 12:09 ` [PATCH 4/7] add--interactive.perl: use add--helper --status for status_cmd Daniel Ferreira via GitGitGadget
@ 2018-12-20 12:09 ` Slavica Djukic via GitGitGadget
  2019-01-14 11:12   ` Phillip Wood
  2018-12-20 12:09 ` [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY Slavica Djukic via GitGitGadget
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 29+ messages in thread
From: Slavica Djukic via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Slavica Djukic

From: Slavica Djukic <slawica92@hotmail.com>

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().

Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 add-interactive.c     | 19 +++++++++++++++++++
 add-interactive.h     |  2 ++
 builtin/add--helper.c |  7 ++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index c55d934186..ff5bfbac49 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -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");
+}
diff --git a/add-interactive.h b/add-interactive.h
index 1f4747553c..a74c65b7e1 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -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
\ No newline at end of file
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
index 43545d9af5..e288412d56 100644
--- a/builtin/add--helper.c
+++ b/builtin/add--helper.c
@@ -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);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (4 preceding siblings ...)
  2018-12-20 12:09 ` [PATCH 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
@ 2018-12-20 12:09 ` Slavica Djukic via GitGitGadget
  2019-01-14 11:13   ` Phillip Wood
  2018-12-20 12:09 ` [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 29+ messages in thread
From: Slavica Djukic via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Slavica Djukic

From: Slavica Djukic <slawica92@hotmail.com>

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.

Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 color.c     | 4 ++++
 perl/Git.pm | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index ebb222ec33..4aa6cc3442 100644
--- a/color.c
+++ b/color.c
@@ -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)) {
diff --git a/perl/Git.pm b/perl/Git.pm
index d856930b2e..51a1ce0617 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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');
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (5 preceding siblings ...)
  2018-12-20 12:09 ` [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY Slavica Djukic via GitGitGadget
@ 2018-12-20 12:09 ` Slavica Djukic via GitGitGadget
  2019-01-14 11:17   ` Phillip Wood
  2018-12-20 12:37 ` [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 29+ messages in thread
From: Slavica Djukic via GitGitGadget @ 2018-12-20 12:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Slavica Djukic

From: Slavica Djukic <slawica92@hotmail.com>

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 <slawica92@hotmail.com>
---
 git-add--interactive.perl  | 11 +----------
 t/t3701-add-interactive.sh | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a6536f9cf3..32ee729a58 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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 {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 65dfbc033a..9c9d5bd935 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 0/7] Turn git add-i into built-in
@ 2018-12-20 12:34 Johannes Schindelin
  2018-12-20 12:09 ` [PATCH 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
                   ` (9 more replies)
  0 siblings, 10 replies; 29+ messages in thread
From: Johannes Schindelin @ 2018-12-20 12:34 UTC (permalink / raw)
  To: git; +Cc: gitster, Slavica Đukić via GitGitGadget

From: "Slavica Đukić via GitGitGadget" <gitgitgadget@gmail.com>

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/1494907234-28903-1-git-send-email-bnmvco@gmail.com/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
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/7] Turn git add-i into built-in
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (6 preceding siblings ...)
  2018-12-20 12:09 ` [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget
@ 2018-12-20 12:37 ` Johannes Schindelin
  2019-01-11 14:09 ` Slavica Djukic
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
  9 siblings, 0 replies; 29+ messages in thread
From: Johannes Schindelin @ 2018-12-20 12:37 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, gitster

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

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

On Thu, 20 Dec 2018, Johannes Schindelin wrote:

> From: "Slavica Đukić via GitGitGadget" <gitgitgadget@gmail.com>
> 
> 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/1494907234-28903-1-git-send-email-bnmvco@gmail.com/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
> -- 
> gitgitgadget
> 
^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/7] Turn git add-i into built-in
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (7 preceding siblings ...)
  2018-12-20 12:37 ` [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
@ 2019-01-11 14:09 ` Slavica Djukic
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
  9 siblings, 0 replies; 29+ messages in thread
From: Slavica Djukic @ 2019-01-11 14:09 UTC (permalink / raw)
  To: Johannes Schindelin, git; +Cc: gitster


On 20-Dec-18 1:34 PM, Johannes Schindelin wrote:
> From: "Slavica Đukić via GitGitGadget" <gitgitgadget@gmail.com>
>
> 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/1494907234-28903-1-git-send-email-bnmvco@gmail.com/t/#u


Ping? :)


>
> 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 5/7] add-interactive.c: implement show-help command
  2018-12-20 12:09 ` [PATCH 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
@ 2019-01-14 11:12   ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2019-01-14 11:12 UTC (permalink / raw)
  To: Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano, Slavica Djukic

Hi Slavica

I think the basics of this patch are fine, I've got a few comments below

On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
> From: Slavica Djukic <slawica92@hotmail.com>
> 
> 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().
> 
> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> ---
>  add-interactive.c     | 19 +++++++++++++++++++
>  add-interactive.h     |  2 ++
>  builtin/add--helper.c |  7 ++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/add-interactive.c b/add-interactive.c
> index c55d934186..ff5bfbac49 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -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");

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.

> +
>  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?)

> +	printf("\n");
> +	color_fprintf(stdout, help_color, modified_fmt, _(help_info));
> +	printf("\n");
> +}
> diff --git a/add-interactive.h b/add-interactive.h
> index 1f4747553c..a74c65b7e1 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -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()?

> +
>  #endif
> \ No newline at end of file

C files should end with a new line (it is actually undefined behavior if
they don't!)

Best Wishes

Phillip

> diff --git a/builtin/add--helper.c b/builtin/add--helper.c
> index 43545d9af5..e288412d56 100644
> --- a/builtin/add--helper.c
> +++ b/builtin/add--helper.c
> @@ -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);
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
  2018-12-20 12:09 ` [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY Slavica Djukic via GitGitGadget
@ 2019-01-14 11:13   ` Phillip Wood
  2019-01-15 12:45     ` Slavica Djukic
  2019-01-15 13:50     ` Johannes Schindelin
  0 siblings, 2 replies; 29+ messages in thread
From: Phillip Wood @ 2019-01-14 11:13 UTC (permalink / raw)
  To: Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano, Slavica Djukic

Hi Salvica/Johannes

On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
> From: Slavica Djukic <slawica92@hotmail.com>
> 
> 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

> 
> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> ---
>  color.c     | 4 ++++
>  perl/Git.pm | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/color.c b/color.c
> index ebb222ec33..4aa6cc3442 100644
> --- a/color.c
> +++ b/color.c
> @@ -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)) {
> diff --git a/perl/Git.pm b/perl/Git.pm
> index d856930b2e..51a1ce0617 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -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');
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
  2018-12-20 12:09 ` [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget
@ 2019-01-14 11:17   ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2019-01-14 11:17 UTC (permalink / raw)
  To: Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano, Slavica Djukic

Hi Slavica

On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
> From: Slavica Djukic <slawica92@hotmail.com>
> 
> 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.
> 

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

> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> ---
>  git-add--interactive.perl  | 11 +----------
>  t/t3701-add-interactive.sh | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index a6536f9cf3..32ee729a58 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -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 {
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 65dfbc033a..9c9d5bd935 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
  2019-01-14 11:13   ` Phillip Wood
@ 2019-01-15 12:45     ` Slavica Djukic
  2019-01-15 13:50     ` Johannes Schindelin
  1 sibling, 0 replies; 29+ messages in thread
From: Slavica Djukic @ 2019-01-15 12:45 UTC (permalink / raw)
  To: phillip.wood, Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano

Hi Phillip,

First of all, thank you for taking your time to review this patch series.

On 14-Jan-19 12:13 PM, Phillip Wood wrote:
> Hi Salvica/Johannes
>
> On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
>> From: Slavica Djukic <slawica92@hotmail.com>
>>
>> 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


>
> Best Wishes
>
> Phillip
>
>> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
>> ---
>>   color.c     | 4 ++++
>>   perl/Git.pm | 2 +-
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/color.c b/color.c
>> index ebb222ec33..4aa6cc3442 100644
>> --- a/color.c
>> +++ b/color.c
>> @@ -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)) {
>> diff --git a/perl/Git.pm b/perl/Git.pm
>> index d856930b2e..51a1ce0617 100644
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -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');
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
  2019-01-14 11:13   ` Phillip Wood
  2019-01-15 12:45     ` Slavica Djukic
@ 2019-01-15 13:50     ` Johannes Schindelin
  2019-01-15 16:09       ` Phillip Wood
  1 sibling, 1 reply; 29+ messages in thread
From: Johannes Schindelin @ 2019-01-15 13:50 UTC (permalink / raw)
  To: phillip.wood
  Cc: Slavica Djukic via GitGitGadget, git, Junio C Hamano, Slavica Djukic

Hi Phillip,

On Mon, 14 Jan 2019, Phillip Wood wrote:

> Hi Salvica/Johannes

Close ;-)

> On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
> > From: Slavica Djukic <slawica92@hotmail.com>
> > 
> > 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY
  2019-01-15 13:50     ` Johannes Schindelin
@ 2019-01-15 16:09       ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2019-01-15 16:09 UTC (permalink / raw)
  To: Johannes Schindelin, phillip.wood
  Cc: Slavica Djukic via GitGitGadget, git, Junio C Hamano, Slavica Djukic

On 15/01/2019 13:50, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Mon, 14 Jan 2019, Phillip Wood wrote:
> 
>> Hi Salvica/Johannes
> 
> Close ;-)

Sorry Slavica

> 
>> On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
>>> From: Slavica Djukic <slawica92@hotmail.com>
>>>
>>> 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...

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

> 
> Ciao,
> Dscho
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 0/7] Turn git add-i into built-in
  2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
                   ` (8 preceding siblings ...)
  2019-01-11 14:09 ` Slavica Djukic
@ 2019-01-18  7:47 ` " Slavica Đukić via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
                     ` (6 more replies)
  9 siblings, 7 replies; 29+ messages in thread
From: Slavica Đukić via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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/1494907234-28903-1-git-send-email-bnmvco@gmail.com/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 <slawica92@hotmail.com>
      
       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_done

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 1/7] diff: export diffstat interface
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
@ 2019-01-18  7:47   ` Daniel Ferreira via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 2/7] add--helper: create builtin helper for interactive add Daniel Ferreira via GitGitGadget
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

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.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 diff.c | 36 ++++++++++++++----------------------
 diff.h | 18 ++++++++++++++++++
 2 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/diff.c b/diff.c
index dc9965e836..46a7d8cf29 100644
--- a/diff.c
+++ b/diff.c
@@ -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 --git a/diff.h b/diff.h
index ce5e8a8183..7809db3039 100644
--- a/diff.h
+++ b/diff.h
@@ -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
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 2/7] add--helper: create builtin helper for interactive add
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
@ 2019-01-18  7:47   ` Daniel Ferreira via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 3/7] add-interactive.c: implement status command Daniel Ferreira via GitGitGadget
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

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.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 .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

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..2ee71ed217 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,6 +15,7 @@
 /git
 /git-add
 /git-add--interactive
+/git-add--helper
 /git-am
 /git-annotate
 /git-apply
diff --git a/Makefile b/Makefile
index 1a44c811aa..9c84b80739 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/builtin.h b/builtin.h
index 6538932e99..dd811ef7d5 100644
--- a/builtin.h
+++ b/builtin.h
@@ -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);
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
new file mode 100644
index 0000000000..6a97f0e191
--- /dev/null
+++ b/builtin/add--helper.c
@@ -0,0 +1,6 @@
+#include "builtin.h"
+
+int cmd_add__helper(int argc, const char **argv, const char *prefix)
+{
+	return 0;
+}
diff --git a/git.c b/git.c
index 2f604a41ea..5507591f2e 100644
--- a/git.c
+++ b/git.c
@@ -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 },
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 3/7] add-interactive.c: implement status command
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 2/7] add--helper: create builtin helper for interactive add Daniel Ferreira via GitGitGadget
@ 2019-01-18  7:47   ` Daniel Ferreira via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 4/7] add--interactive.perl: use add--helper --status for status_cmd Daniel Ferreira via GitGitGadget
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

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.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 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

diff --git a/Makefile b/Makefile
index 9c84b80739..2a4a5cc37b 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/add-interactive.c b/add-interactive.c
new file mode 100644
index 0000000000..c55d934186
--- /dev/null
+++ b/add-interactive.c
@@ -0,0 +1,246 @@
+#include "add-interactive.h"
+#include "cache.h"
+#include "commit.h"
+#include "color.h"
+#include "config.h"
+#include "diffcore.h"
+#include "revision.h"
+
+#define HEADER_INDENT "      "
+
+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;
+};
+
+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 struct file_stat *e1 = entry, *e2 = entry_or_key;
+	const char *name = keydata ? keydata : e2->name;
+
+	return strcmp(e1->name, name);
+}
+
+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);
+}
+
+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);
+
+	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);
+		}
+
+		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;
+		}
+	}
+}
+
+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;
+
+	rev.diffopt.flags.ignore_dirty_submodules = 1;
+	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;
+	rev.diffopt.format_callback = collect_changes_cb;
+	rev.diffopt.format_callback_data = s;
+
+	run_diff_index(&rev, 1);
+}
+
+void add_i_print_modified(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;
+
+	struct hashmap_iter iter;
+	struct file_stat **files;
+	struct file_stat *entry;
+
+	if (read_cache() < 0)
+		return;
+
+	s.reference = !get_oid("HEAD", &sha1) ? "HEAD": empty_tree_oid_hex();
+	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;
+	}
+
+	printf(HEADER_INDENT);
+	color_fprintf(stdout, header_color, modified_fmt, _("staged"),
+			_("unstaged"), _("path"));
+	printf("\n");
+
+	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);
+
+	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");
+
+	free(files);
+	hashmap_free(&s.file_map, 1);
+}
diff --git a/add-interactive.h b/add-interactive.h
new file mode 100644
index 0000000000..1f4747553c
--- /dev/null
+++ b/add-interactive.h
@@ -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
\ No newline at end of file
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
index 6a97f0e191..43545d9af5 100644
--- a/builtin/add--helper.c
+++ b/builtin/add--helper.c
@@ -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;
 }
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 4/7] add--interactive.perl: use add--helper --status for status_cmd
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
                     ` (2 preceding siblings ...)
  2019-01-18  7:47   ` [PATCH v2 3/7] add-interactive.c: implement status command Daniel Ferreira via GitGitGadget
@ 2019-01-18  7:47   ` Daniel Ferreira via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help() Slavica Djukic via GitGitGadget
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 29+ messages in thread
From: Daniel Ferreira via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Daniel Ferreira

From: Daniel Ferreira <bnmvco@gmail.com>

Call the newly introduced add--helper builtin on
status_cmd() instead of relying on add--interactive's Perl
functions to build print the numstat.

Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 git-add--interactive.perl | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 20eb81cc92..a6536f9cf3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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 {
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 5/7] add-interactive.c: implement show-help command
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
                     ` (4 preceding siblings ...)
  2019-01-18  7:47   ` [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help() Slavica Djukic via GitGitGadget
@ 2019-01-18  7:47   ` Slavica Djukic via GitGitGadget
  2019-01-18 11:20     ` Phillip Wood
  2019-01-18  7:47   ` [PATCH v2 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget
  6 siblings, 1 reply; 29+ messages in thread
From: Slavica Djukic via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Slavica Djukic

From: Slavica Djukic <slawica92@hotmail.com>

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().

Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 add-interactive.c     | 23 +++++++++++++++++++++++
 add-interactive.h     |  4 +++-
 builtin/add--helper.c |  7 ++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index c55d934186..76c3f4c3eb 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -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");
+}
diff --git a/add-interactive.h b/add-interactive.h
index 1f4747553c..46e17c5c71 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -5,4 +5,6 @@ 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
+void add_i_show_help(void);
+
+#endif
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
index 43545d9af5..a3b3a68b68 100644
--- a/builtin/add--helper.c
+++ b/builtin/add--helper.c
@@ -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);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help()
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
                     ` (3 preceding siblings ...)
  2019-01-18  7:47   ` [PATCH v2 4/7] add--interactive.perl: use add--helper --status for status_cmd Daniel Ferreira via GitGitGadget
@ 2019-01-18  7:47   ` Slavica Djukic via GitGitGadget
  2019-01-18 11:23     ` Phillip Wood
  2019-01-18  7:47   ` [PATCH v2 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
  2019-01-18  7:47   ` [PATCH v2 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget
  6 siblings, 1 reply; 29+ messages in thread
From: Slavica Djukic via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Slavica Djukic

From: Slavica Djukic <slawica92@hotmail.com>

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.

Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 65dfbc033a..14e3286995 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
  2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
                     ` (5 preceding siblings ...)
  2019-01-18  7:47   ` [PATCH v2 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
@ 2019-01-18  7:47   ` Slavica Djukic via GitGitGadget
  6 siblings, 0 replies; 29+ messages in thread
From: Slavica Djukic via GitGitGadget @ 2019-01-18  7:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Slavica Djukic

From: Slavica Djukic <slawica92@hotmail.com>

Change help_cmd sub in git-add--interactive.perl to use
show-help command from builtin add--helper.

Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
 git-add--interactive.perl | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a6536f9cf3..32ee729a58 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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 {
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
  2019-01-18  7:47   ` [PATCH v2 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
@ 2019-01-18 11:20     ` Phillip Wood
  2019-01-18 12:19       ` Slavica Djukic
       [not found]       ` <VI1PR05MB577331CCE110D2EAE325927CA69C0@VI1PR05MB5773.eurprd05.prod.outlook.com>
  0 siblings, 2 replies; 29+ messages in thread
From: Phillip Wood @ 2019-01-18 11:20 UTC (permalink / raw)
  To: Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano, Slavica Djukic

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).

On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote:
> From: Slavica Djukic <slawica92@hotmail.com>
> 
> 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().
> 
> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> ---
>  add-interactive.c     | 23 +++++++++++++++++++++++
>  add-interactive.h     |  4 +++-
>  builtin/add--helper.c |  7 ++++++-
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/add-interactive.c b/add-interactive.c
> index c55d934186..76c3f4c3eb 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -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

> +	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");
> +}
> diff --git a/add-interactive.h b/add-interactive.h
> index 1f4747553c..46e17c5c71 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -5,4 +5,6 @@ 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
> +void add_i_show_help(void);
> +
> +#endif
> diff --git a/builtin/add--helper.c b/builtin/add--helper.c
> index 43545d9af5..a3b3a68b68 100644
> --- a/builtin/add--helper.c
> +++ b/builtin/add--helper.c
> @@ -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);
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help()
  2019-01-18  7:47   ` [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help() Slavica Djukic via GitGitGadget
@ 2019-01-18 11:23     ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2019-01-18 11:23 UTC (permalink / raw)
  To: Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano, Slavica Djukic

Hi Slavica

Thanks for moving this up, it's really good to add the test before the
conversion to C.

Best Wishes

Phillip

On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote:
> From: Slavica Djukic <slawica92@hotmail.com>
> 
> 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.
> 
> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> ---
>  t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 65dfbc033a..14e3286995 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
  2019-01-18 11:20     ` Phillip Wood
@ 2019-01-18 12:19       ` Slavica Djukic
       [not found]       ` <VI1PR05MB577331CCE110D2EAE325927CA69C0@VI1PR05MB5773.eurprd05.prod.outlook.com>
  1 sibling, 0 replies; 29+ messages in thread
From: Slavica Djukic @ 2019-01-18 12:19 UTC (permalink / raw)
  To: phillip.wood, Slavica Djukic via GitGitGadget, git; +Cc: Junio C Hamano

Hi Phillip,

On 18-Jan-19 12:20 PM, Phillip Wood wrote:
> 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.


>
> On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote:
>> From: Slavica Djukic <slawica92@hotmail.com>
>>
>> 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().
>>
>> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
>> ---
>>   add-interactive.c     | 23 +++++++++++++++++++++++
>>   add-interactive.h     |  4 +++-
>>   builtin/add--helper.c |  7 ++++++-
>>   3 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/add-interactive.c b/add-interactive.c
>> index c55d934186..76c3f4c3eb 100644
>> --- a/add-interactive.c
>> +++ b/add-interactive.c
>> @@ -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");


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.


>
>
> Best Wishes
>
> Phillip
>
>> +	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");
>> +}
>> diff --git a/add-interactive.h b/add-interactive.h
>> index 1f4747553c..46e17c5c71 100644
>> --- a/add-interactive.h
>> +++ b/add-interactive.h
>> @@ -5,4 +5,6 @@ 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
>> +void add_i_show_help(void);
>> +
>> +#endif
>> diff --git a/builtin/add--helper.c b/builtin/add--helper.c
>> index 43545d9af5..a3b3a68b68 100644
>> --- a/builtin/add--helper.c
>> +++ b/builtin/add--helper.c
>> @@ -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);
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
       [not found]       ` <VI1PR05MB577331CCE110D2EAE325927CA69C0@VI1PR05MB5773.eurprd05.prod.outlook.com>
@ 2019-01-18 14:25         ` Phillip Wood
  2019-01-18 20:40           ` Johannes Schindelin
  0 siblings, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2019-01-18 14:25 UTC (permalink / raw)
  To: Slavica Đukić,
	phillip.wood, Slavica Djukic via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin

Hi Slavica

On 18/01/2019 12:19, Slavica Đukić wrote:
> Hi Phillip,
> 
> On 18-Jan-19 12:20 PM, Phillip Wood wrote:
>> 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.

I'm not sure what GitGitGadget does about CC'ing people but Johannes 
will know

>> On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote:
>>> From: Slavica Djukic <slawica92@hotmail.com>
>>>
>>> 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().
>>>
>>> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
>>> ---
>>>    add-interactive.c     | 23 +++++++++++++++++++++++
>>>    add-interactive.h     |  4 +++-
>>>    builtin/add--helper.c |  7 ++++++-
>>>    3 files changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/add-interactive.c b/add-interactive.c
>>> index c55d934186..76c3f4c3eb 100644
>>> --- a/add-interactive.c
>>> +++ b/add-interactive.c
>>> @@ -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");
> 
> 
> 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.

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

>> Best Wishes
>>
>> Phillip
>>
>>> +	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");
>>> +}
>>> diff --git a/add-interactive.h b/add-interactive.h
>>> index 1f4747553c..46e17c5c71 100644
>>> --- a/add-interactive.h
>>> +++ b/add-interactive.h
>>> @@ -5,4 +5,6 @@ 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
>>> +void add_i_show_help(void);
>>> +
>>> +#endif
>>> diff --git a/builtin/add--helper.c b/builtin/add--helper.c
>>> index 43545d9af5..a3b3a68b68 100644
>>> --- a/builtin/add--helper.c
>>> +++ b/builtin/add--helper.c
>>> @@ -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);
>>>


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 5/7] add-interactive.c: implement show-help command
  2019-01-18 14:25         ` Phillip Wood
@ 2019-01-18 20:40           ` Johannes Schindelin
  0 siblings, 0 replies; 29+ messages in thread
From: Johannes Schindelin @ 2019-01-18 20:40 UTC (permalink / raw)
  To: phillip.wood
  Cc: Slavica Đukić,
	Slavica Djukic via GitGitGadget, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 6631 bytes --]

Hi Phillip,

On Fri, 18 Jan 2019, Phillip Wood wrote:

> On 18/01/2019 12:19, Slavica Đukić wrote:
> > 
> > On 18-Jan-19 12:20 PM, Phillip Wood wrote:
> > >
> > > 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.
> 
> 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:

	Cc: Name <email@example.org>, Other <other@example.org>

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

> 
> > > On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote:
> > > > From: Slavica Djukic <slawica92@hotmail.com>
> > > >
> > > > 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().
> > > >
> > > > Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> > > > ---
> > > >    add-interactive.c     | 23 +++++++++++++++++++++++
> > > >    add-interactive.h     |  4 +++-
> > > >    builtin/add--helper.c |  7 ++++++-
> > > >    3 files changed, 32 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/add-interactive.c b/add-interactive.c
> > > > index c55d934186..76c3f4c3eb 100644
> > > > --- a/add-interactive.c
> > > > +++ b/add-interactive.c
> > > > @@ -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");
> > 
> > 
> > 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.
> 
> 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
> 
> > > Best Wishes
> > >
> > > Phillip
> > >
> > > > +	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");
> > > > +}
> > > > diff --git a/add-interactive.h b/add-interactive.h
> > > > index 1f4747553c..46e17c5c71 100644
> > > > --- a/add-interactive.h
> > > > +++ b/add-interactive.h
> > > > @@ -5,4 +5,6 @@ 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
> > > > +void add_i_show_help(void);
> > > > +
> > > > +#endif
> > > > diff --git a/builtin/add--helper.c b/builtin/add--helper.c
> > > > index 43545d9af5..a3b3a68b68 100644
> > > > --- a/builtin/add--helper.c
> > > > +++ b/builtin/add--helper.c
> > > > @@ -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);
> > > >
> 
> 
> 
^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-20 12:34 [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
2018-12-20 12:09 ` [PATCH 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2018-12-20 12:09 ` [PATCH 2/7] add--helper: create builtin helper for interactive add Daniel Ferreira via GitGitGadget
2018-12-20 12:09 ` [PATCH 3/7] add-interactive.c: implement status command Daniel Ferreira via GitGitGadget
2018-12-20 12:09 ` [PATCH 4/7] add--interactive.perl: use add--helper --status for status_cmd Daniel Ferreira via GitGitGadget
2018-12-20 12:09 ` [PATCH 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
2019-01-14 11:12   ` Phillip Wood
2018-12-20 12:09 ` [PATCH 6/7] Git.pm: introduce environment variable GIT_TEST_PRETEND_TTY Slavica Djukic via GitGitGadget
2019-01-14 11:13   ` Phillip Wood
2019-01-15 12:45     ` Slavica Djukic
2019-01-15 13:50     ` Johannes Schindelin
2019-01-15 16:09       ` Phillip Wood
2018-12-20 12:09 ` [PATCH 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget
2019-01-14 11:17   ` Phillip Wood
2018-12-20 12:37 ` [PATCH 0/7] Turn git add-i into built-in Johannes Schindelin
2019-01-11 14:09 ` Slavica Djukic
2019-01-18  7:47 ` [PATCH v2 " Slavica Đukić via GitGitGadget
2019-01-18  7:47   ` [PATCH v2 1/7] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-01-18  7:47   ` [PATCH v2 2/7] add--helper: create builtin helper for interactive add Daniel Ferreira via GitGitGadget
2019-01-18  7:47   ` [PATCH v2 3/7] add-interactive.c: implement status command Daniel Ferreira via GitGitGadget
2019-01-18  7:47   ` [PATCH v2 4/7] add--interactive.perl: use add--helper --status for status_cmd Daniel Ferreira via GitGitGadget
2019-01-18  7:47   ` [PATCH v2 6/7] t3701-add-interactive: test add_i_show_help() Slavica Djukic via GitGitGadget
2019-01-18 11:23     ` Phillip Wood
2019-01-18  7:47   ` [PATCH v2 5/7] add-interactive.c: implement show-help command Slavica Djukic via GitGitGadget
2019-01-18 11:20     ` Phillip Wood
2019-01-18 12:19       ` Slavica Djukic
     [not found]       ` <VI1PR05MB577331CCE110D2EAE325927CA69C0@VI1PR05MB5773.eurprd05.prod.outlook.com>
2019-01-18 14:25         ` Phillip Wood
2019-01-18 20:40           ` Johannes Schindelin
2019-01-18  7:47   ` [PATCH v2 7/7] add--interactive.perl: use add--helper --show-help for help_cmd Slavica Djukic via GitGitGadget

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox