Re: [PATCH v2 1/5] revision: add --merges={show|only|hide} option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 7, 2015, 05:16 UTC
Message-ID: <CAPig+cQ434roQFA0E5wvGAgp5UdGu5oKCq-uNvohMhsdG3S1og@mail.gmail.com>
In-Reply-To: <1428110521-31028-1-git-send-email-koosha@posteo.de>

On Fri, Apr 3, 2015 at 9:21 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:

Show 9 quoted lines

> From: Junio C Hamano <gitster@pobox.com>
>
> revision: add a new option 'merges=' with
> possible values of 'only', 'show' and 'hide'.
> The option is used when showing the list of commits.
> The value 'only' lists only merges. The value 'show'
> is the default behavior which shows the commits as well
> as merges and the value 'hide' makes it just list commit
> items.

A couple very minor comments if you to re-roll this series, otherwise keep them in mind for future submissions:

Since "revision:" already prefixes the first line of the commit message (in the Subject:), its reiteration at the start of this paragraph is unnecessary. It's sufficient to start the paragraph with "Add new --merges= option...".

The excessively jagged right edge of the paragraph makes it slightly difficult to read. Try to wrap paragraphs to about 70-72 characters.

Show 61 quoted lines

> [kk: chose names for options; wrote commit message]
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
>  revision.c | 20 ++++++++++++++++++++
>  revision.h |  1 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 6399a04..c3c3dcc 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
>         add_grep(revs, pattern, GREP_PATTERN_BODY);
>  }
>
> +int parse_merges_opt(struct rev_info *revs, const char *param)
> +{
> +       if (!strcmp(param, "show")) {
> +               revs->min_parents = 0;
> +               revs->max_parents = -1;
> +       } else if (!strcmp(param, "only")) {
> +               revs->min_parents = 2;
> +               revs->max_parents = -1;
> +       } else if (!strcmp(param, "hide")) {
> +               revs->min_parents = 0;
> +               revs->max_parents = 1;
> +       } else {
> +               return -1;
> +       }
> +       return 0;
> +}
> +
>  static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
>                                int *unkc, const char **unkv)
>  {
> @@ -1800,6 +1817,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>                 revs->show_all = 1;
>         } else if (!strcmp(arg, "--remove-empty")) {
>                 revs->remove_empty_trees = 1;
> +       } else if (starts_with(arg, "--merges=")) {
> +               if (parse_merges_opt(revs, arg + 9))
> +                       die("unknown option: %s", arg);
>         } else if (!strcmp(arg, "--merges")) {
>                 revs->min_parents = 2;
>         } else if (!strcmp(arg, "--no-merges")) {
> diff --git a/revision.h b/revision.h
> index 0ea8b4e..f9df58c 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
>  extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
>                                const struct option *options,
>                                const char * const usagestr[]);
> +extern int parse_merges_opt(struct rev_info *, const char *);
>  #define REVARG_CANNOT_BE_FILENAME 01
>  #define REVARG_COMMITTISH 02
>  extern int handle_revision_arg(const char *arg, struct rev_info *revs,
> --
> 2.3.3.263.g095251d.dirty