Skip to contentSkip to conversation

Teach git annotate to use parse option instead of its own argv

PATCH In review Abimbola Olaitan via GitGitGadget 3 participants 9 messages Nov 3, 2019 – Nov 4, 2019 Kbuild

lore
Share & tools
lore
#0 lore raw

cover Teach git annotate to use parse option instead of its own argv

The file builtIn/annotate.c was removed, letting git annotate use the cmd_blame directly.
===================

The following files were edited
===============================

--- Makefile --- git.c --- builtIn/annotate.c was removed

Abimbola (5):
remove-annotate: change cmd_annotate to cmd_blame edit the makefile
remove-annotate: comment out annotate BUILDIN_OBJS
remove-annotate: delete builtin annotate file fix unproper comment in Makefile

 Makefile           |   1 -
 annotate.txt       | Bin 0 -> 163170 bytes
 blame.txt          | Bin 0 -> 177882 bytes
 builtin/annotate.c |  22 -------
 git.c              | 143 ++++++++++++++++++++++++++-------------------
 5 files changed, 82 insertions(+), 84 deletions(-)
 create mode 100644 annotate.txt
 create mode 100644 blame.txt
 delete mode 100644 builtin/annotate.c

base-commit: efd54442381a2792186abc994060b8f7dd8b834b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-442%2Fcraftword%2Fremove-annotate-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-442/craftword/remove-annotate-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/442

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/5 remove-annotate: change cmd_annotate to cmd_blame

From: Abimbola <[email protected]>

Changing this command is to remove the annotate.c file which does almost the same thing as blame.c. git annotate will invoc blame directly

---
 git.c | 143 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 82 insertions(+), 61 deletions(-)
git.c +82 -61
index ce6ab0ece2..84042846b5 100644
@@ -5,17 +5,17 @@
 #include "run-command.h"
 #include "alias.h"
 
-#define RUN_SETUP		(1<<0)
-#define RUN_SETUP_GENTLY	(1<<1)
-#define USE_PAGER		(1<<2)
+#define RUN_SETUP (1 << 0)
+#define RUN_SETUP_GENTLY (1 << 1)
+#define USE_PAGER (1 << 2)
 /*
  * require working tree to be present -- anything uses this needs
  * RUN_SETUP for reading from the configuration file.
  */
-#define NEED_WORK_TREE		(1<<3)
-#define SUPPORT_SUPER_PREFIX	(1<<4)
-#define DELAY_PAGER_CONFIG	(1<<5)
-#define NO_PARSEOPT		(1<<6) /* parse-options is not used */
+#define NEED_WORK_TREE (1 << 3)
+#define SUPPORT_SUPER_PREFIX (1 << 4)
+#define DELAY_PAGER_CONFIG (1 << 5)
+#define NO_PARSEOPT (1 << 6) /* parse-options is not used */
 
 struct cmd_struct {
 	const char *cmd;
@@ -23,22 +23,23 @@ struct cmd_struct {
 	unsigned int option;
 };
 
-const char git_usage_string[] =
-	N_("git [--version] [--help] [-C <path>] [-c <name>=<value>]\n"
-	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
-	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
-	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
-	   "           <command> [<args>]");
+const char git_usage_string[] = N_(
+	"git [--version] [--help] [-C <path>] [-c <name>=<value>]\n"
+	"           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
+	"           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
+	"           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
+	"           <command> [<args>]");
 
-const char git_more_info_string[] =
-	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
-	   "concept guides. See 'git help <command>' or 'git help <concept>'\n"
-	   "to read about a specific subcommand or concept.\n"
-	   "See 'git help git' for an overview of the system.");
+const char git_more_info_string[] = N_(
+	"'git help -a' and 'git help -g' list available subcommands and some\n"
+	"concept guides. See 'git help <command>' or 'git help <concept>'\n"
+	"to read about a specific subcommand or concept.\n"
+	"See 'git help git' for an overview of the system.");
 
 static int use_pager = -1;
 
-static void list_builtins(struct string_list *list, unsigned int exclude_option);
+static void list_builtins(struct string_list *list,
+			  unsigned int exclude_option);
 
 static void exclude_helpers_from_list(struct string_list *list)
 {
@@ -66,9 +67,9 @@ static int list_cmds(const char *spec)
 	int nongit;
 
 	/*
-	* Set up the repository so we can pick up any repo-level config (like
-	* completion.commands).
-	*/
+	 * Set up the repository so we can pick up any repo-level config (like
+	 * completion.commands).
+	 */
 	setup_git_directory_gently(&nongit);
 
 	while (*spec) {
@@ -93,8 +94,7 @@ static int list_cmds(const char *spec)
 			strbuf_add(&sb, spec + 5, len - 5);
 			list_cmds_by_category(&list, sb.buf);
 			strbuf_release(&sb);
-		}
-		else
+		} else
 			die(_("unsupported command listing type '%s'"), spec);
 		spec += len;
 		if (*spec == ',')
@@ -183,7 +183,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--git-dir")) {
 			if (*argc < 2) {
-				fprintf(stderr, _("no directory given for --git-dir\n" ));
+				fprintf(stderr,
+					_("no directory given for --git-dir\n"));
 				usage(git_usage_string);
 			}
 			setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
@@ -197,7 +198,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--namespace")) {
 			if (*argc < 2) {
-				fprintf(stderr, _("no namespace given for --namespace\n" ));
+				fprintf(stderr,
+					_("no namespace given for --namespace\n"));
 				usage(git_usage_string);
 			}
 			setenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);
@@ -211,7 +213,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
 			if (*argc < 2) {
-				fprintf(stderr, _("no directory given for --work-tree\n" ));
+				fprintf(stderr,
+					_("no directory given for --work-tree\n"));
 				usage(git_usage_string);
 			}
 			setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
@@ -225,7 +228,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--super-prefix")) {
 			if (*argc < 2) {
-				fprintf(stderr, _("no prefix given for --super-prefix\n" ));
+				fprintf(stderr,
+					_("no prefix given for --super-prefix\n"));
 				usage(git_usage_string);
 			}
 			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
@@ -247,7 +251,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "-c")) {
 			if (*argc < 2) {
-				fprintf(stderr, _("-c expects a configuration string\n" ));
+				fprintf(stderr,
+					_("-c expects a configuration string\n"));
 				usage(git_usage_string);
 			}
 			git_config_push_parameter((*argv)[1]);
@@ -280,17 +285,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 		} else if (!strcmp(cmd, "--shallow-file")) {
 			(*argv)++;
 			(*argc)--;
-			set_alternate_shallow_file(the_repository, (*argv)[0], 1);
+			set_alternate_shallow_file(the_repository, (*argv)[0],
+						   1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "-C")) {
 			if (*argc < 2) {
-				fprintf(stderr, _("no directory given for -C\n" ));
+				fprintf(stderr,
+					_("no directory given for -C\n"));
 				usage(git_usage_string);
 			}
 			if ((*argv)[1][0]) {
 				if (chdir((*argv)[1]))
-					die_errno("cannot change to '%s'", (*argv)[1]);
+					die_errno("cannot change to '%s'",
+						  (*argv)[1]);
 				if (envchanged)
 					*envchanged = 1;
 			}
@@ -354,7 +362,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			trace2_cmd_name("_run_shell_alias_");
 
 			ret = run_command(&child);
-			if (ret >= 0)   /* normal exit */
+			if (ret >= 0) /* normal exit */
 				exit(ret);
 
 			die_errno(_("while expanding alias '%s': '%s'"),
@@ -378,8 +386,7 @@ static int handle_alias(int *argcp, const char ***argv)
 		if (!strcmp(alias_command, new_argv[0]))
 			die(_("recursive alias: %s"), alias_command);
 
-		trace_argv_printf(new_argv,
-				  "trace: alias expansion: %s =>",
+		trace_argv_printf(new_argv, "trace: alias expansion: %s =>",
 				  alias_command);
 
 		REALLOC_ARRAY(new_argv, count + *argcp);
@@ -416,14 +423,16 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 
-		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+		if (use_pager == -1 &&
+		    p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
 		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
-		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
+		    startup_info->have_repository) /* get_git_dir() may set up
+						      repo, avoid that */
 			trace_repo_setup(prefix);
 	}
 	commit_pager_choice();
@@ -467,7 +476,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 },
 	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
-	{ "annotate", cmd_annotate, RUN_SETUP | NO_PARSEOPT },
+	{ "annotate", cmd_blame, RUN_SETUP },
 	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
 	{ "archive", cmd_archive, RUN_SETUP_GENTLY },
 	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
@@ -478,10 +487,9 @@ static struct cmd_struct commands[] = {
 	{ "check-attr", cmd_check_attr, RUN_SETUP },
 	{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 	{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
-	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT  },
+	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT },
 	{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
-	{ "checkout-index", cmd_checkout_index,
-		RUN_SETUP | NEED_WORK_TREE},
+	{ "checkout-index", cmd_checkout_index, RUN_SETUP | NEED_WORK_TREE },
 	{ "cherry", cmd_cherry, RUN_SETUP },
 	{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
 	{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
@@ -495,7 +503,8 @@ static struct cmd_struct commands[] = {
 	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "describe", cmd_describe, RUN_SETUP },
 	{ "diff", cmd_diff, NO_PARSEOPT },
-	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "diff-files", cmd_diff_files,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
@@ -528,10 +537,14 @@ static struct cmd_struct commands[] = {
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
-	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-ours", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-theirs", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-subtree", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
 	{ "mktree", cmd_mktree, RUN_SETUP },
@@ -549,9 +562,10 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "range-diff", cmd_range_diff, RUN_SETUP | USE_PAGER },
-	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "rebase", cmd_rebase, RUN_SETUP | NEED_WORK_TREE },
-	{ "rebase--interactive", cmd_rebase__interactive, RUN_SETUP | NEED_WORK_TREE },
+	{ "rebase--interactive", cmd_rebase__interactive,
+	  RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
@@ -581,7 +595,8 @@ static struct cmd_struct commands[] = {
 	{ "stash", cmd_stash },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
-	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
+	{ "submodule--helper", cmd_submodule__helper,
+	  RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
 	{ "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE },
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
@@ -623,8 +638,7 @@ static void list_builtins(struct string_list *out, unsigned int exclude_option)
 {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
-		if (exclude_option &&
-		    (commands[i].option & exclude_option))
+		if (exclude_option && (commands[i].option & exclude_option))
 			continue;
 		string_list_append(out, commands[i].cmd);
 	}
@@ -752,7 +766,8 @@ static int run_argv(int *argcp, const char ***argv)
 			trace2_cmd_name("_run_git_alias_");
 
 			if (get_super_prefix())
-				die("%s doesn't support --super-prefix", **argv);
+				die("%s doesn't support --super-prefix",
+				    **argv);
 
 			commit_pager_choice();
 
@@ -764,10 +779,13 @@ static int run_argv(int *argcp, const char ***argv)
 
 			/*
 			 * if we fail because the command is not found, it is
-			 * OK to return. Otherwise, we just pass along the status code.
+			 * OK to return. Otherwise, we just pass along the
+			 * status code.
 			 */
-			i = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |
-						  RUN_CLEAN_ON_EXIT, "git_alias");
+			i = run_command_v_opt_tr2(args.argv,
+						  RUN_SILENT_EXEC_FAILURE |
+							  RUN_CLEAN_ON_EXIT,
+						  "git_alias");
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
@@ -781,7 +799,8 @@ static int run_argv(int *argcp, const char ***argv)
 			int i;
 			struct strbuf sb = STRBUF_INIT;
 			for (i = 0; i < cmd_list.nr; i++) {
-				struct string_list_item *item = &cmd_list.items[i];
+				struct string_list_item *item =
+					&cmd_list.items[i];
 
 				strbuf_addf(&sb, "\n  %s", item->string);
 				if (item == seen)
@@ -790,7 +809,8 @@ static int run_argv(int *argcp, const char ***argv)
 					strbuf_addstr(&sb, " ==>");
 			}
 			die(_("alias loop detected: expansion of '%s' does"
-			      " not terminate:%s"), cmd_list.items[0].string, sb.buf);
+			      " not terminate:%s"),
+			    cmd_list.items[0].string, sb.buf);
 		}
 
 		string_list_append(&cmd_list, *argv[0]);
@@ -872,8 +892,9 @@ int cmd_main(int argc, const char **argv)
 		if (errno != ENOENT)
 			break;
 		if (was_alias) {
-			fprintf(stderr, _("expansion of alias '%s' failed; "
-					  "'%s' is not a git command\n"),
+			fprintf(stderr,
+				_("expansion of alias '%s' failed; "
+				  "'%s' is not a git command\n"),
 				cmd, argv[0]);
 			exit(1);
 		}
@@ -884,8 +905,8 @@ int cmd_main(int argc, const char **argv)
 			break;
 	}
 
-	fprintf(stderr, _("failed to run command '%s': %s\n"),
-		cmd, strerror(errno));
+	fprintf(stderr, _("failed to run command '%s': %s\n"), cmd,
+		strerror(errno));
 
 	return 1;
 }
Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/5] remove-annotate: change cmd_annotate to cmd_blame

Quoted 5 lines · Abimbola via GitGitGadget

Subject: Re: [PATCH 1/5] remove-annotate: change cmd_annotate to cmd_blame
From: Abimbola <[email protected]>

Changing this command is to remove the annotate.c file which does almost the same thing as blame.c. git annotate will invoke blame directly

Both the summary and the detailed description of the changes is not entirely clean. What is this 'remove-annotate' subsystem? Why we would want to change cmd_annotate to cmd_blame -- and wouldn't we loose difference (in defaults, and thus in output format) between git-blame and git-annotate?

Quoted 2 lines

Why not

Quoted 3 lines
---
 git.c | 143 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 82 insertions(+), 61 deletions(-)

Looks like a big change, strangely. And annotate.c is not actually deleted...

Quoted 6 lines
git.c +0 -0
index ce6ab0ece2..84042846b5 100644

git.c line 10

Quoted 9 lines
git.c +3 -3
@@ -5,17 +5,17 @@
 #include "run-command.h"
 #include "alias.h"
 
-#define RUN_SETUP		(1<<0)
-#define RUN_SETUP_GENTLY	(1<<1)
-#define USE_PAGER		(1<<2)
+#define RUN_SETUP (1 << 0)
+#define RUN_SETUP_GENTLY (1 << 1)
+#define USE_PAGER (1 << 2)

Please, please do not include such unnecessary (and possibly accidental) changes.

It makes review harded (where are the actual changes), and it makes merging it harder (as it can much more easily conflict with other changes in flight).

Also, those changes that remove aligning of values make code less readable, and would probably be rejected.

[skip whitespace-only changes (I hope)]

git.c struct cmd_struct { · line 42

Quoted 3 lines
git.c +2 -1
-static void list_builtins(struct string_list *list, unsigned int exclude_option);
+static void list_builtins(struct string_list *list,
+			  unsigned int exclude_option);

This may be a good cleanup, but is unrelated to the change in question. Better leave it to a separate patch.

[skip horizontal and vertical whitespace changes (I hope)]

git.c static int list_cmds(const char *spec) · line 100

Quoted 10 lines
git.c +1 -2
@@ -93,8 +94,7 @@ static int list_cmds(const char *spec)
 			strbuf_add(&sb, spec + 5, len - 5);
 			list_cmds_by_category(&list, sb.buf);
 			strbuf_release(&sb);
-		}
-		else
+		} else
 			die(_("unsupported command listing type '%s'"), spec);
 		spec += len;
 		if (*spec == ',')

This may be a good cleanup, but is unrelated to the change in question, Better leave it to another separate patch.

git.c static int run_builtin(struct cmd_struct *p, int argc, const char **argv) · line 482

Quoted 9 lines
git.c +1 -1
@@ -467,7 +476,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 },
 	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
-	{ "annotate", cmd_annotate, RUN_SETUP | NO_PARSEOPT },
+	{ "annotate", cmd_blame, RUN_SETUP },
 	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
 	{ "archive", cmd_archive, RUN_SETUP_GENTLY },
 	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },

That looks like the major portion of the change. How the look and feel of git-annotate command is preserved, though?

git.c static struct cmd_struct commands[] = { · line 490

Quoted 6 lines
git.c +1 -1
@@ -478,10 +487,9 @@ static struct cmd_struct commands[] = {
 	{ "check-attr", cmd_check_attr, RUN_SETUP },
 	{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 	{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
-	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT  },
+	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT },

This may be a good cleanup, but is unrelated to the change in question. Better leave it to a patch.

Regards,

Signature · 1 line
Jakub Narębski
#3 lore raw

Re: [PATCH 1/5] remove-annotate: change cmd_annotate to cmd_blame

Quoted 19 lines · Jakub Narebski
Quoted 5 lines · Abimbola via GitGitGadget

Subject: Re: [PATCH 1/5] remove-annotate: change cmd_annotate to cmd_blame
From: Abimbola <[email protected]>

Changing this command is to remove the annotate.c file which does almost the same thing as blame.c. git annotate will invoke blame directly

Both the summary and the detailed description of the changes is not entirely clean. What is this 'remove-annotate' subsystem? Why we would want to change cmd_annotate to cmd_blame -- and wouldn't we loose difference (in defaults, and thus in output format) between git-blame and git-annotate?

Quoted 2 lines

Why not

Quoted 8 lines
Quoted 3 lines
---
 git.c | 143 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 82 insertions(+), 61 deletions(-)

Looks like a big change, strangely. And annotate.c is not actually deleted...

Thanks for all the good comments. The first priority of any work in this area is not to lose the different defaults between "annotate" and "blame", as the former has ("had", at least when we invented "blame") known uses in widely used script(s), and the solution we took was to use a small annotate.c shim to tell the blame machinery to emit historical "annotate" output format. If we can do so without having the shim, that would be OK. If removal of shim loses the distinction, then it is not.

I suspect that cmd_blame() can be taught to look at argv[0] and pretend "-c" was given on the command line, and then the command dispatch table can call cmd_blame() upon seeing "annotate" as the subcommand name, but I haven't actually tried it so there may be hidden dragons.

Thanks.

#4 lore raw

PATCH 2/5 edit the makefile

In reply to Abimbola Olaitan via GitGitGadget

From: Abimbola <[email protected]>

---
 Makefile | 1 -
 1 file changed, 1 deletion(-)
Makefile +0 -1
index 58b92af54b..41f0fcb78f 100644
@@ -1033,7 +1033,6 @@ LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/am.o
-BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
 BUILTIN_OBJS += builtin/archive.o
 BUILTIN_OBJS += builtin/bisect--helper.o
Signature · 1 line
gitgitgadget
#5 lore raw

PATCH 3/5 remove-annotate: comment out annotate BUILDIN_OBJS

In reply to Abimbola Olaitan via GitGitGadget

From: Abimbola <[email protected]>

---
 Makefile | 1 +
 1 file changed, 1 insertion(+)
Makefile +1 -0
index 41f0fcb78f..a632505ad9 100644
@@ -1033,6 +1033,7 @@ LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/am.o
+// BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
 BUILTIN_OBJS += builtin/archive.o
 BUILTIN_OBJS += builtin/bisect--helper.o
Signature · 1 line
gitgitgadget
#6 lore raw

PATCH 4/5 remove-annotate: delete builtin annotate file

In reply to Abimbola Olaitan via GitGitGadget

From: Abimbola <[email protected]>

---
 annotate.txt       | Bin 0 -> 163170 bytes
 blame.txt          | Bin 0 -> 177882 bytes
 builtin/annotate.c |  22 ----------------------
 3 files changed, 22 deletions(-)
 create mode 100644 annotate.txt
 create mode 100644 blame.txt
 delete mode 100644 builtin/annotate.c
annotate.txt new file binary +0 -0
new file mode 100644
index 0000000000000000000000000000000000000000..c39e85a8217476b050dd190c58e6988cb3566f96
GIT binary patch
blame.txt new file binary +0 -0
new file mode 100644
index 0000000000000000000000000000000000000000..f0da3b70966596be154fd2158427f5941967317b
GIT binary patch
builtin/annotate.c deleted +0 -22
deleted file mode 100644
index da413ae0d1..0000000000
@@ -1,22 +0,0 @@
-/*
- * "git annotate" builtin alias
- *
- * Copyright (C) 2006 Ryan Anderson
- */
-#include "git-compat-util.h"
-#include "builtin.h"
-#include "argv-array.h"
-
-int cmd_annotate(int argc, const char **argv, const char *prefix)
-{
-	struct argv_array args = ARGV_ARRAY_INIT;
-	int i;
-
-	argv_array_pushl(&args, "annotate", "-c", NULL);
-
-	for (i = 1; i < argc; i++) {
-		argv_array_push(&args, argv[i]);
-	}
-
-	return cmd_blame(args.argc, args.argv, prefix);
-}
Signature · 1 line
gitgitgadget
#7 lore raw

PATCH 5/5 fix unproper comment in Makefile

In reply to Abimbola Olaitan via GitGitGadget

From: Abimbola <[email protected]>

---
 Makefile | 1 -
 1 file changed, 1 deletion(-)
Makefile +0 -1
index a632505ad9..41f0fcb78f 100644
@@ -1033,7 +1033,6 @@ LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/am.o
-// BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
 BUILTIN_OBJS += builtin/archive.o
 BUILTIN_OBJS += builtin/bisect--helper.o
Signature · 1 line
gitgitgadget
#8 lore raw

Re: [PATCH 5/5] fix unproper comment in Makefile

Quoted 12 lines · Abimbola via GitGitGadget

From: Abimbola <[email protected]>

---
 Makefile | 1 -
 1 file changed, 1 deletion(-)
Makefile +0 -0
index a632505ad9..41f0fcb78f 100644

Makefile LIB_OBJS += zlib.o · line 1038

Quoted 7 lines
Makefile +0 -1
@@ -1033,7 +1033,6 @@ LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/am.o
-// BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
 BUILTIN_OBJS += builtin/archive.o
 BUILTIN_OBJS += builtin/bisect--helper.o

If you are ever going to reroll this topic, please clean up your series before sending it out. 3/5 introduced a bogus line this step removes; it should have used the right comment char "#" instead, or better yet, just drop both.

Find where you are doing "oops, the change done in the previous step was wrong, and here is a commit to bring it into the right state" and learn to use "rebase -i" to eliminate such a "oops, another band aid on top"; it is a useful skill.

Having said that, if the topic is only to remove builtin/annotate.c, I would expect that it would be a single patch topic (i.e. Makefile, reference in git.c and builtin/annotate.c all must be modified or removed at the same time).

Thanks.