Re: [PATCH 1/5] Add a new option 'merges' to revision.c

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Add a new option 'merges' to revision.c

For the subject, mention the area you're touching, followed by a
colon, followed by a short but meaningful summary of the change. Drop
capitalization.

    revision: add --merges={show|only|hide} option

> revision.c: add a new option 'merges' with

No need to mention "revision.c" here since the patch itself shows this clearly.

Considering that there is already a --merges option, it is somewhat
misleading and not terribly clear to say only that you're adding a new
"option 'merges'". Better would be to spell out --merges= explicitly.

> 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.
>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>

Considering that Junio actually wrote this patch[1], it would be more
correct and considerate to attribute it to him. You can do so by
ensuring that there is a "From:" header at the very top of the commit
message before mailing out the patch:

    From: Junio C Hamano <gitster@pobox.com>

The customary way to indicate that you wrote the commit message and
made a few small adjustments to Junio's patch would be with a
bracketed comment (starting with your initials) just before your
sign-off. Something like this:

    [kk: wrote commit message; added a couple missing
    {min|max}_parents assignments]

[1]: http://article.gmane.org/gmane.comp.version-control.git/265597

> ---
> diff --git a/revision.c b/revision.c
> index 66520c6..edb7bed 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,9 +1817,14 @@ 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->max_parents = -1;
>                 revs->min_parents = 2;
>         } else if (!strcmp(arg, "--no-merges")) {
> +               revs->min_parents = 0;
>                 revs->max_parents = 1;
>         } else if (starts_with(arg, "--min-parents=")) {
>                 revs->min_parents = atoi(arg+14);
> 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