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

"Nathan W. Panike" [off-list ref] 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.

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;

Hi:

On Fri, Jan 9, 2009 at 6:49 PM, Junio C Hamano [off-list ref] 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.

Thanks,

Nathan Panike

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.

If 'log.showroot' is not set, format-patch shouldn't even try to create
a patch for the root commit.

Signed-off-by: Alexander Potashev <redacted>
---
 builtin-log.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 4a02ee9..62134d4 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -972,6 +972,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				has_commit_patch_id(commit, &ids))
 			continue;
 
+		if (!commit->parents && !rev.show_root_diff)
+			break;
+
 		nr++;
 		list = xrealloc(list, nr * sizeof(list[0]));
 		list[nr - 1] = commit;

-- 
1.6.1.77.g569c.dirty

Hi:

On Sat, Jan 10, 2009 at 5:39 AM, Alexander Potashev
[off-list ref] wrote:
...

+               if (!commit->parents && !rev.show_root_diff)
+                       break;

Do you really want to stop getting commits?  It seems like the break
statement here should be a continue.

Nathan Panike

On 10:01 Sat 10 Jan     , Nathan W. Panike wrote:

Hi:

On Sat, Jan 10, 2009 at 5:39 AM, Alexander Potashev
[off-list ref] wrote:
...

quoted

+               if (!commit->parents && !rev.show_root_diff)
+                       break;

Do you really want to stop getting commits?  It seems like the break
statement here should be a continue.

AFAIU get_revision stops revision iteration when it appears to stay at
the root commit. So, if we will replace 'break' with 'continue', the
'while' loop will finish right after that 'continue'.

However, I might be wrong... please, correct me then.

Nathan Panike

1. format-patch'ing root commit shouldn't create empty patches
2. With --root it should create a patch for the root commit
3. Similar testcases with two commits in the tree

Signed-off-by: Alexander Potashev <redacted>
---

git format-patch lacks a '--no-root' option, so I used
'git config log.showroot false' to emulate it.



 t/t4033-format-patch-root-commit.sh |   52 +++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)
 create mode 100755 t/t4033-format-patch-root-commit.sh

diff --git a/t/t4033-format-patch-root-commit.sh b/t/t4033-format-patch-root-commit.sh
new file mode 100755
index 0000000..846c11c
--- /dev/null
+++ b/t/t4033-format-patch-root-commit.sh

@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='Format-patch root commit skipping/allowing'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git config log.showroot false
+	git config format.numbered false
+	echo A > file &&
+	git add file &&
+	git commit -m First
+'
+
+test_patch_count() {
+	cnt=$(grep "^Subject: \[PATCH\]" $1 | wc -l) &&
+	test $cnt = $2
+}
+
+test_patch_is_single() {
+	cnt=$(grep "^Subject: \[PATCH\] $2" $1 | wc -l) &&
+	test $cnt = 1
+}
+
+test_expect_success 'format-patch root commit with showroot = false' '
+	git format-patch -1 &&
+	test_must_fail cat 0001-First.patch
+'
+
+test_expect_success 'format-patch root commit' '
+	git format-patch --root --stdout -5 >root-only.patch &&
+	test_patch_count root-only.patch 1 &&
+	test_patch_is_single root-only.patch First
+'
+
+test_expect_success 'format-patch 2 commits without root' '
+	echo B > file &&
+	git commit -a -m Second &&
+
+	git format-patch --stdout -2 >two-except-root.patch &&
+	test_patch_count two-except-root.patch 1 &&
+	test_patch_is_single two-except-root.patch Second
+'
+
+test_expect_success 'format-patch 2 commits including root' '
+	git format-patch --root --stdout -2 >two-with-root.patch &&
+	test_patch_count two-with-root.patch 2 &&
+	test_patch_is_single two-with-root.patch First &&
+	test_patch_is_single two-with-root.patch Second
+'
+
+test_done

-- 
1.6.1.81.g61cf1

Hi:

On Sat, Jan 10, 2009 at 10:17 AM, Alexander Potashev
[off-list ref] wrote:

...

AFAIU get_revision stops revision iteration when it appears to stay at
the root commit. So, if we will replace 'break' with 'continue', the
'while' loop will finish right after that 'continue'.

However, I might be wrong... please, correct me then.

I was thinking of the case where there is more than one root.  Maybe
the code does the right thing then, but I confess I have not looked at
it deeply enough to know.

Nathan Panike

On 19:39 Sat 10 Jan     , Alexander Potashev wrote:

1. format-patch'ing root commit shouldn't create empty patches
2. With --root it should create a patch for the root commit
3. Similar testcases with two commits in the tree

Signed-off-by: Alexander Potashev <redacted>
---

git format-patch lacks a '--no-root' option, so I used
'git config log.showroot false' to emulate it.

Sorry, --root option has nothing in common with log.showroot, but the
testcases are still valid.

"Nathan W. Panike" [off-list ref] writes:

On Sat, Jan 10, 2009 at 5:39 AM, Alexander Potashev
[off-list ref] wrote:
...

quoted

+               if (!commit->parents && !rev.show_root_diff)
+                       break;

Do you really want to stop getting commits?  It seems like the break
statement here should be a continue.

You can give a commit range that has two independent roots.  The above
"break" is wrong.

The variable is called show_root_DIFF, not show_root_COMMIT; even if you
have "log.showroot = false", "git log -p" output would still give you the
initial commit, but without the patch text, no?

But that is not Alexander's fault; it is mine.

I think "log -p" and "format-patch" can and should behave differently in
this case.  "log -p" is for people who already _have_ the history and
would want to inspect how it evolved, and it is reasonable if some people
want to say "the very initial huge import is not interesting to me while
reviewing the history", and turning it off makes sense for them (in fact,
the default was initially that way).

On the other hand, "format-patch" is about exporting a part of your
history so that you can mechanincally replay it elsewhere, and I do not
think of a reasonable justification not to export a root commit fully if
the range user asked for happens to contain one.

I agree with Alexander that we should not output just the message without
the patch text, but I think the right solution is to show both. not to
skip root.

-- >8 --
format-patch: show patch text for the root commit

Even without --root specified, if the range given on the command line
happens to include a root commit, we should include its patch text in the
output.

This fix deliberately ignores log.showroot configuration variable because
"format-patch" and "log -p" can and should behave differently in this
case, as the former is about exporting a part of your history in a form
that is replayable elsewhere and just giving the commit log message
without the patch text does not make any sense for that purpose.

Noticed and fix originally attempted by Nathan W. Panike; credit goes to
Alexander Potashev for injecting sanity to my initial (broken) fix that
used the value from log.showroot configuration, which was misguided.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-log.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git c/builtin-log.c w/builtin-log.c
index 4a02ee9..91e5412 100644
--- c/builtin-log.c
+++ w/builtin-log.c

@@ -935,6 +935,13 @@ 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.
+	 */
+	rev.show_root_diff = 1;
+
 	if (cover_letter) {
 		/* remember the range */
 		int i;