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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 10, 2009, 00:49 UTC
Message-ID: <7vmye0yohu.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1231536787-20685-1-git-send-email-nathan.panike@gmail.com>

"Nathan W. Panike" <nathan.panike@gmail.com> writes:

Show 5 quoted lines

> Rework this patch to try to handle the case where one does
>
> git format-patch -n ...
>
> and n is a number larger than 1.

It is unclear what "this patch" is in the context of this proposed commit message.

> git format-patch -1 e83c5163316f89bfbde
> ...

I do not think the current backward compatibile behaviour to avoid surprising the end user by creating a huge initial import diff is particularly a good idea.

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.

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.

Perhaps there are some other downsides I may not remember why --root is not the default, though.

Show 16 quoted lines

> Signed-off-by: Nathan W. Panike <nathan.panike@gmail.com>
> ---
>  builtin-log.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-log.c b/builtin-log.c
> index 4a02ee9..0eca15f 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -975,6 +975,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		nr++;
>  		list = xrealloc(list, nr * sizeof(list[0]));
>  		list[nr - 1] = commit;
> +		if(!commit->parents){
> +			rev.show_root_diff=1;
> +		}

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

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.

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;