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

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

[Nathan W. Panike]

Currently, the command

git format-patch -1 e83c5163316f89bfbde

in the git repository creates an empty file.  Instead, one is
forced to do

git format-patch -1 --root e83c5163316f89bfbde

This seems arbitrary.  This patch fixes this case, so that

git format-patch -1 e83c5163316f89bfbde

will produce an actual patch.
---
 builtin-log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 4a02ee9..5e7b61f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -977,6 +977,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		list[nr - 1] = commit;
 	}
 	total = nr;
+	if (total == 1 && !list[0]->parents)
+		rev.show_root_diff=1;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
 	if (numbered)
-- 
1.6.1.76.gc123b.dirty

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

[Alexander Potashev]

Hello!

I experienced this problem today while preparing a simple patch for
reply in "[PATCH 2/2] Use is_pseudo_dir_name everywhere" thread.
I used a workaround: add a file, commit, remove it, commit, add it once
again, commit and after all format-patch.

On 13:35 Fri 09 Jan     , Nathan W. Panike wrote:
> Currently, the command
> 
> git format-patch -1 e83c5163316f89bfbde
> 
> in the git repository creates an empty file.  Instead, one is
> forced to do
> 
> git format-patch -1 --root e83c5163316f89bfbde
> 
> This seems arbitrary.  This patch fixes this case, so that
> 
> git format-patch -1 e83c5163316f89bfbde

Your patch doesn't solve the problem if there are more than one commit
(say, 2 commits) and you run 'git format-patch -2'. Even with your patch
format-patch writes an empty patch file corresponding to the root commit
(actually, it creates 2 patches, but the first is empty).

Please, correct me if I'm wrong.

> 
> will produce an actual patch.
> ---
>  builtin-log.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-log.c b/builtin-log.c
> index 4a02ee9..5e7b61f 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -977,6 +977,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		list[nr - 1] = commit;
>  	}
>  	total = nr;
> +	if (total == 1 && !list[0]->parents)
> +		rev.show_root_diff=1;
>  	if (!keep_subject && auto_number && total > 1)
>  		numbered = 1;
>  	if (numbered)
> -- 
> 1.6.1.76.gc123b.dirty

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

[Nathan W. Panike]

Rework this patch to try to handle the case where one does

git format-patch -n ...

and n is a number larger than 1.  Currently, the command

git format-patch -1 e83c5163316f89bfbde

in the git repository creates an empty file.  Instead, one is
forced to do

git format-patch -1 --root e83c5163316f89bfbde

This seems arbitrary.  This patch fixes this case, so that

git format-patch -1 e83c5163316f89bfbde

will produce an actual patch.

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;
+		}
 	}
 	total = nr;
 	if (!keep_subject && auto_number && total > 1)
-- 
1.6.1.76.gc123b.dirty

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

[Junio C Hamano]

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

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

> 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;

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

[Nathan W. Panike]

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.

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

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

> 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

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

[Alexander Potashev]

Hello, Junio!

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

I think the problem was not only format-patch misbehaviour. If you use
"log.showroot = no", you still get an empty patch file, which is not
very good, because format-patch doesn't work very well, it creates
_corrupt_ patches! It's much better to not create the patch file at all
in this case.

However, it has nothing in common with your patch, but there's room for
another commit.

> 
>  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;

(unknown)

[nathan.panike]

>From 65c4fed27fe9752ffd0e3b7cb6807561a4dd4601 Mon Sep 17 00: 00:00 2001
From: Nathan W. Panike <nathan.panike@gmail.com>
Date: Fri, 9 Jan 2009 11:53:43 -0600
Subject: [PATCH] Get format-patch to show first commit after root commit

Currently, the command

git format-patch -1 e83c5163316f89bfbde

in the git repository creates an empty file.  Instead, one is
forced to do

git format-patch -1 --root e83c5163316f89bfbde

This seems arbitrary.  This patch fixes this case, so that

git format-patch -1 e83c5163316f89bfbde

will produce an actual patch.
---
 builtin-log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 4a02ee9..5e7b61f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -977,6 +977,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		list[nr - 1] = commit;
 	}
 	total = nr;
+	if (total == 1 && !list[0]->parents)
+		rev.show_root_diff=1;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
 	if (numbered)
-- 
1.6.1.76.gc123b.dirty

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

[Johannes Schindelin]

Hi,

your mailing process has a problem; neither the recipients nor the subject 
made it into the mail header.

On Fri, 9 Jan 2009, nathan.panike@gmail.com wrote:

> >From 65c4fed27fe9752ffd0e3b7cb6807561a4dd4601 Mon Sep 17 00: 00:00 2001
> From: Nathan W. Panike <nathan.panike@gmail.com>
> Date: Fri, 9 Jan 2009 11:53:43 -0600
> Subject: [PATCH] Get format-patch to show first commit after root commit
> 
> Currently, the command
> 
> git format-patch -1 e83c5163316f89bfbde

You do not need -1, and using 19 digits seems a bit arbitrary; the 
convention seems to be 7 digits (that is what --abbrev-commit does).

> 
> in the git repository creates an empty file.  Instead, one is
> forced to do
> 
> git format-patch -1 --root e83c5163316f89bfbde
> 
> This seems arbitrary.  This patch fixes this case, so that
> 
> git format-patch -1 e83c5163316f89bfbde
> 
> will produce an actual patch.

IMHO mentioning --root is misleading, as the real bug is the empty diff.

Apart from that, I like the patch, though.

Thanks,
Dscho

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

[Johannes Schindelin]

Hi,

On Sat, 10 Jan 2009, Johannes Schindelin wrote:

> On Fri, 9 Jan 2009, nathan.panike@gmail.com wrote:
> 
> > >From 65c4fed27fe9752ffd0e3b7cb6807561a4dd4601 Mon Sep 17 00: 00:00 2001
> > From: Nathan W. Panike <nathan.panike@gmail.com>
> > Date: Fri, 9 Jan 2009 11:53:43 -0600
> > Subject: [PATCH] Get format-patch to show first commit after root commit
> > 
> > Currently, the command
> > 
> > git format-patch -1 e83c5163316f89bfbde
> 
> You do not need -1, and using 19 digits seems a bit arbitrary; the 
> convention seems to be 7 digits (that is what --abbrev-commit does).

Sorry, the -1 is needed.

Ciao,
Dscho