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

[Jakub Narebski <jnareb@gmail.com>]

"Abimbola via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH 1/5] remove-annotate: change cmd_annotate to cmd_blame
> From: Abimbola <craftwordltd@gmail.com>
>
> 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?

>
> Signed-off-by: Abimbola <craftwordltd@gmail.com>

Why not

  Signed-off-by: Abimbola Olaitan <craftwordltd@gmail.com>

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

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

>
> diff --git a/git.c b/git.c
> index ce6ab0ece2..84042846b5 100644
> --- a/git.c
> +++ b/git.c
> @@ -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)]

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

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

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

> @@ -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,
-- 
Jakub Narębski