Re: [PATCH 1/5] remove-annotate: change cmd_annotate to cmd_blame
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Nov 3, 2019, 19:59 UTC
- Message-ID
- <86tv7khgtf.fsf@gmail.com>
- In-Reply-To
- <6e164528059529fe07a53f0cfceb22a388a6758b.1572753114.git.gitgitgadget@gmail.com>
"Abimbola via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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...
Show 15 quoted lines
> > 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)]
Show 10 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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?
Show 6 quoted lines
> @@ -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