format-patch: no 'prerequisite-patch-id' info when specifying commit range

format-patch: no 'prerequisite-patch-id' info when specifying commit range

[Eduardo Habkost]

Hi,

I'm trying to use git-format-patch --base to generate the list of
prerequisite patches for a series, but the behavior of git
doesn't seem to match the documentation:

When using a commit count (e.g.: "-2"), git-format-patch generates the
prerequisite-patch-id lines as expected.  But when using a commit range like
"Z..C", the prerequisite-patch-id lines are missing.

Is this intentional, or it is a bug?

Example using git.git commits:

  $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 -2 b7b1fca17 | egrep 'base-commit|prereq'
  base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef
  prerequisite-patch-id: 080ac2faf21a6a7f9b23cb68286866d026a92930
  prerequisite-patch-id: e3ee77500c9aa70248e7ee814662d01f79d0dcdb
  prerequisite-patch-id: 6d831e23e33075681e6b74553151a32b73092013
  (ehabkost@localhost:~/rh/proj/git (ok) 1j)
  $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 b7b1fca17~2..b7b1fca17 | egrep 'base-commit|prereq'
  base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef
  $ git --version
  git version 2.17.1
  $ git log --graph --pretty=oneline -6 b7b1fca17
  * b7b1fca175f1ed7933f361028c631b9ac86d868d fsck: complain when .gitmodules is a symlink
  * 73c3f0f704a91b6792e0199a3f3ab6e3a1971675 index-pack: check .gitmodules files with --strict
  * 6e328d6caef218db320978e3e251009135d87d0e unpack-objects: call fsck_finish() after fscking objects
  * 1995b5e03e1cc97116be58cdc0502d4a23547856 fsck: call fsck_finish() after fscking objects
  * ed8b10f631c9a71df3351d46187bf7f3fa4f9b7e fsck: check .gitmodules content
  * 2738744426c161a98c2ec494d41241a4c5eef9ef fsck: handle promisor objects in .gitmodules check
  $ 

If I understand the documentation correctly, both "-3 C" or "Z..C" were
supposed to be equivalent:

> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
> `--cover-letter` or using `Z..C` instead of `-3 C` to specify the
> range), the base tree information block is shown at the end of the
> first message the command outputs (either the first patch, or the
> cover letter), like this:
> 
> ------------
> base-commit: P
> prerequisite-patch-id: X
> prerequisite-patch-id: Y
> prerequisite-patch-id: Z
> ------------

-- 
Eduardo

Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range

[Ye Xiaolong]

Hi, Eduardo

On 05/29, Eduardo Habkost wrote:
>Hi,
>
>I'm trying to use git-format-patch --base to generate the list of
>prerequisite patches for a series, but the behavior of git
>doesn't seem to match the documentation:
>
>When using a commit count (e.g.: "-2"), git-format-patch generates the
>prerequisite-patch-id lines as expected.  But when using a commit range like
>"Z..C", the prerequisite-patch-id lines are missing.
>
>Is this intentional, or it is a bug?

Thanks for reporting, it seems an unexpected behavior, I'll look into it.

Thanks,
Xiaolong

>
>Example using git.git commits:
>
>  $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 -2 b7b1fca17 | egrep 'base-commit|prereq'
>  base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef
>  prerequisite-patch-id: 080ac2faf21a6a7f9b23cb68286866d026a92930
>  prerequisite-patch-id: e3ee77500c9aa70248e7ee814662d01f79d0dcdb
>  prerequisite-patch-id: 6d831e23e33075681e6b74553151a32b73092013
>  (ehabkost@localhost:~/rh/proj/git (ok) 1j)
>  $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 b7b1fca17~2..b7b1fca17 | egrep 'base-commit|prereq'
>  base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef
>  $ git --version
>  git version 2.17.1
>  $ git log --graph --pretty=oneline -6 b7b1fca17
>  * b7b1fca175f1ed7933f361028c631b9ac86d868d fsck: complain when .gitmodules is a symlink
>  * 73c3f0f704a91b6792e0199a3f3ab6e3a1971675 index-pack: check .gitmodules files with --strict
>  * 6e328d6caef218db320978e3e251009135d87d0e unpack-objects: call fsck_finish() after fscking objects
>  * 1995b5e03e1cc97116be58cdc0502d4a23547856 fsck: call fsck_finish() after fscking objects
>  * ed8b10f631c9a71df3351d46187bf7f3fa4f9b7e fsck: check .gitmodules content
>  * 2738744426c161a98c2ec494d41241a4c5eef9ef fsck: handle promisor objects in .gitmodules check
>  $ 
>
>If I understand the documentation correctly, both "-3 C" or "Z..C" were
>supposed to be equivalent:
>
>> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
>> `--cover-letter` or using `Z..C` instead of `-3 C` to specify the
>> range), the base tree information block is shown at the end of the
>> first message the command outputs (either the first patch, or the
>> cover letter), like this:
>> 
>> ------------
>> base-commit: P
>> prerequisite-patch-id: X
>> prerequisite-patch-id: Y
>> prerequisite-patch-id: Z
>> ------------
>
>-- 
>Eduardo

Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range

[Ye Xiaolong]

Hi, Junio

On 05/29, Eduardo Habkost wrote:
>Hi,
>
>I'm trying to use git-format-patch --base to generate the list of
>prerequisite patches for a series, but the behavior of git
>doesn't seem to match the documentation:
>
>When using a commit count (e.g.: "-2"), git-format-patch generates the
>prerequisite-patch-id lines as expected.  But when using a commit range like
>"Z..C", the prerequisite-patch-id lines are missing.
>

I narrowed down the problem to revision walk, if users specify the commit range
via "Z..C" pattern, the first prepare_revision_walk function called in
cmd_format_patch would mark all parents (ancestors) of Z to be uninteresting,
thus the next revision walk in prepare_bases wouldn't be able to reach
prerequisite patches, one quick solution I can think of is to clear
UNINTERESTING flag in reset_revision_walk, like below:

void reset_revision_walk(void)
{
	clear_object_flags(SEEN | ADDED | SHOWN| UNINTERESTING);
}

Though I'm not sure whether it has some side effects, or whether it would impact
behavior of other reference of reset_revision_walk. If you think it's a sensible
solution, I'll submit a patch.

Thanks,
Xiaolong


>Is this intentional, or it is a bug?
>
>Example using git.git commits:
>
>  $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 -2 b7b1fca17 | egrep 'base-commit|prereq'
>  base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef
>  prerequisite-patch-id: 080ac2faf21a6a7f9b23cb68286866d026a92930
>  prerequisite-patch-id: e3ee77500c9aa70248e7ee814662d01f79d0dcdb
>  prerequisite-patch-id: 6d831e23e33075681e6b74553151a32b73092013
>  (ehabkost@localhost:~/rh/proj/git (ok) 1j)
>  $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 b7b1fca17~2..b7b1fca17 | egrep 'base-commit|prereq'
>  base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef
>  $ git --version
>  git version 2.17.1
>  $ git log --graph --pretty=oneline -6 b7b1fca17
>  * b7b1fca175f1ed7933f361028c631b9ac86d868d fsck: complain when .gitmodules is a symlink
>  * 73c3f0f704a91b6792e0199a3f3ab6e3a1971675 index-pack: check .gitmodules files with --strict
>  * 6e328d6caef218db320978e3e251009135d87d0e unpack-objects: call fsck_finish() after fscking objects
>  * 1995b5e03e1cc97116be58cdc0502d4a23547856 fsck: call fsck_finish() after fscking objects
>  * ed8b10f631c9a71df3351d46187bf7f3fa4f9b7e fsck: check .gitmodules content
>  * 2738744426c161a98c2ec494d41241a4c5eef9ef fsck: handle promisor objects in .gitmodules check
>  $ 
>
>If I understand the documentation correctly, both "-3 C" or "Z..C" were
>supposed to be equivalent:
>
>> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
>> `--cover-letter` or using `Z..C` instead of `-3 C` to specify the
>> range), the base tree information block is shown at the end of the
>> first message the command outputs (either the first patch, or the
>> cover letter), like this:
>> 
>> ------------
>> base-commit: P
>> prerequisite-patch-id: X
>> prerequisite-patch-id: Y
>> prerequisite-patch-id: Z
>> ------------
>
>-- 
>Eduardo

Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range

[Junio C Hamano]

Ye Xiaolong <xiaolong.ye@intel.com> writes:

> I narrowed down the problem to revision walk, if users specify the commit range
> via "Z..C" pattern, the first prepare_revision_walk function called in
> cmd_format_patch would mark all parents (ancestors) of Z to be uninteresting,
> thus the next revision walk in prepare_bases wouldn't be able to reach
> prerequisite patches, one quick solution I can think of is to clear
> UNINTERESTING flag in reset_revision_walk, like below:
>
> void reset_revision_walk(void)
> {
> 	clear_object_flags(SEEN | ADDED | SHOWN| UNINTERESTING);
> }

When you are done with objects that are UNINTERESTING in your
application (i.e. only when "format-patch" is told to compute list
of prereq patches by doing an extra revision walk), your application
can call clear_object_flags() on the flags you are done with, I
would think.

But the current callers of reset_revision_walk() do not expect any
flags other than the ones that are used to keep track of the
traversal state, so it is likely you will break them if you suddenly
started to clear flags randomly.

Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range

[Ye Xiaolong]

On 06/04, Junio C Hamano wrote:
>Ye Xiaolong <xiaolong.ye@intel.com> writes:
>
>> I narrowed down the problem to revision walk, if users specify the commit range
>> via "Z..C" pattern, the first prepare_revision_walk function called in
>> cmd_format_patch would mark all parents (ancestors) of Z to be uninteresting,
>> thus the next revision walk in prepare_bases wouldn't be able to reach
>> prerequisite patches, one quick solution I can think of is to clear
>> UNINTERESTING flag in reset_revision_walk, like below:
>>
>> void reset_revision_walk(void)
>> {
>> 	clear_object_flags(SEEN | ADDED | SHOWN| UNINTERESTING);
>> }
>
>When you are done with objects that are UNINTERESTING in your
>application (i.e. only when "format-patch" is told to compute list
>of prereq patches by doing an extra revision walk), your application
>can call clear_object_flags() on the flags you are done with, I
>would think.
>
>But the current callers of reset_revision_walk() do not expect any
>flags other than the ones that are used to keep track of the
>traversal state, so it is likely you will break them if you suddenly
>started to clear flags randomly.

Got it, I'll try to call clear_object_flags in format-patch related codepatch
only, not to touch the global reset_revision_walk.

Thanks,
Xiaolong

[PATCH] format-patch: clear UNINTERESTING flag before prepare_bases

[Xiaolong Ye]

When users specify the commit range with 'Z..C' pattern for format-patch, all
the parents of Z (including Z) would be marked as UNINTERESTING which would
prevent revision walk in prepare_bases from getting the prerequisite commits,
thus `git format-patch --base <base_commit_sha> Z..C` won't be able to generate
the list of prerequisite patch ids. Clear UNINTERESTING flag with
clear_object_flags solves this issue.

Reported-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 builtin/log.c           | 1 +
 t/t4014-format-patch.sh | 6 ++++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4686f68594..01993de6fe 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1746,6 +1746,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (base_commit || base_auto) {
 		struct commit *base = get_base_commit(base_commit, list, nr);
 		reset_revision_walk();
+		clear_object_flags(UNINTERESTING);
 		prepare_bases(&bases, base, list, nr);
 	}
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 028d5507a6..53880da7bb 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1554,13 +1554,15 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
 
 test_expect_success 'format-patch --base' '
 	git checkout side &&
-	git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&
+	git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&
+	git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&
 	echo >expected &&
 	echo "base-commit: $(git rev-parse HEAD~3)" >>expected &&
 	echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected &&
 	echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected &&
 	signature >> expected &&
-	test_cmp expected actual
+	test_cmp expected actual1 &&
+	test_cmp expected actual2
 '
 
 test_expect_success 'format-patch --base errors out when base commit is in revision list' '
-- 
2.16.GIT

Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases

[Eduardo Habkost]

On Mon, Jun 04, 2018 at 11:05:43PM +0800, Xiaolong Ye wrote:
> When users specify the commit range with 'Z..C' pattern for format-patch, all
> the parents of Z (including Z) would be marked as UNINTERESTING which would
> prevent revision walk in prepare_bases from getting the prerequisite commits,
> thus `git format-patch --base <base_commit_sha> Z..C` won't be able to generate
> the list of prerequisite patch ids. Clear UNINTERESTING flag with
> clear_object_flags solves this issue.
> 
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>

Thanks!  The fix works for me.

Tested-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases

[Ye Xiaolong]

Hi, Junio

Could you help review this patch?

Thanks,
Xiaolong

On 06/04, Xiaolong Ye wrote:
>When users specify the commit range with 'Z..C' pattern for format-patch, all
>the parents of Z (including Z) would be marked as UNINTERESTING which would
>prevent revision walk in prepare_bases from getting the prerequisite commits,
>thus `git format-patch --base <base_commit_sha> Z..C` won't be able to generate
>the list of prerequisite patch ids. Clear UNINTERESTING flag with
>clear_object_flags solves this issue.
>
>Reported-by: Eduardo Habkost <ehabkost@redhat.com>
>Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>---
> builtin/log.c           | 1 +
> t/t4014-format-patch.sh | 6 ++++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/builtin/log.c b/builtin/log.c
>index 4686f68594..01993de6fe 100644
>--- a/builtin/log.c
>+++ b/builtin/log.c
>@@ -1746,6 +1746,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> 	if (base_commit || base_auto) {
> 		struct commit *base = get_base_commit(base_commit, list, nr);
> 		reset_revision_walk();
>+		clear_object_flags(UNINTERESTING);
> 		prepare_bases(&bases, base, list, nr);
> 	}
> 
>diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>index 028d5507a6..53880da7bb 100755
>--- a/t/t4014-format-patch.sh
>+++ b/t/t4014-format-patch.sh
>@@ -1554,13 +1554,15 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
> 
> test_expect_success 'format-patch --base' '
> 	git checkout side &&
>-	git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&
>+	git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&
>+	git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&
> 	echo >expected &&
> 	echo "base-commit: $(git rev-parse HEAD~3)" >>expected &&
> 	echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected &&
> 	echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected &&
> 	signature >> expected &&
>-	test_cmp expected actual
>+	test_cmp expected actual1 &&
>+	test_cmp expected actual2
> '
> 
> test_expect_success 'format-patch --base errors out when base commit is in revision list' '
>-- 
>2.16.GIT
>

Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases

[Stefan Beller]

On Mon, Jun 4, 2018 at 8:09 AM Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>
> When users specify the commit range with 'Z..C' pattern for format-patch, all
> the parents of Z (including Z) would be marked as UNINTERESTING which would
> prevent revision walk in prepare_bases from getting the prerequisite commits,
> thus `git format-patch --base <base_commit_sha> Z..C` won't be able to generate
> the list of prerequisite patch ids. Clear UNINTERESTING flag with
> clear_object_flags solves this issue.

This makes sense;
Reviewed-by: Stefan Beller <sbeller@google.com>