Re: [PATCH] Get format-patch to show first commit after root commit

From: Nathan W. Panike <nathan.panike@gmail.com>
Date: Jan 10, 2009, 01:37 UTC
Message-ID: <d77df1110901091737k6c4fb826tb2287072db2e36a@mail.gmail.com>
In-Reply-To: <7vmye0yohu.fsf@gitster.siamese.dyndns.org>

Hi:

On Fri, Jan 9, 2009 at 6:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

> I do not see anything special you do for "one commit" case in your patch,
> yet the proposed commit message keeps stressing "-1", which puzzles me.

I was trying to address Alexander's concerns he brought up previously in the thread.

Show 9 quoted lines

> Wouldn't it suffice to simply say something like:
>
>    You need to explicitly ask for --root to obtain a patch for the root
>    commit.  This may have been a good way to make sure that the user
>    realizes that a patch from the root commit won't be applicable to a
>    history with existing data, but we should assume the user knows what
>    he is doing when the user explicitly specifies a range of commits that
>    includes the root commit.
>

Indeed it would. I was giving a specific case that shows what problem this patch addresses.

Show 12 quoted lines

> Three issues.
>
>  - The "if(){" violates style by not having one SP before "(" and after ")",
>   and surrounds a single statement with needless { } pair.  You need one SP
>   on each side of the = (assignment) as well.
>
>  - Because rev.show_root_diff is a no-op for non-root commit anyway, I do not
>   think you even want a conditional there.
>
>  - It is a bad style to muck with rev.* while it is actively used for
>   iteration (note that the above part is in a while loop that iterates over
>   &rev).

Thanks for the advice.  I shall adhere to it next time I submit a patch.

> I think the attached would be a better patch.  We already have a
> configuration to control if we show the patch for a root commit by
> default, and we can use reuse it here.  The configuration defaults to true
> these days.

I did not realize this configuration was available. The patch below is much more elegant.

Show 28 quoted lines

> Because the code before the hunk must check if the user said "--root
> commit" or just "commit" from the command line and behave quite
> differently by looking at rev.show_root_diff, we cannot do this assignment
> before the command line parsing like other commands in the log family.
>
>  builtin-log.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git c/builtin-log.c w/builtin-log.c
> index 4a02ee9..2d2c111 100644
> --- c/builtin-log.c
> +++ w/builtin-log.c
> @@ -935,6 +935,14 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>                 * get_revision() to do the usual traversal.
>                 */
>        }
> +
> +       /*
> +        * We cannot move this anywhere earlier because we do want to
> +        * know if --root was given explicitly from the comand line.
> +        */
> +       if (default_show_root)
> +               rev.show_root_diff = 1;
> +
>        if (cover_letter) {
>                /* remember the range */
>                int i;
>

Thanks,

Nathan Panike