threads / rfc / 31223

RFC patch[git-subtree.sh] Use raw subject and body modifier "%B" instead of "%s%n%n%b" for commit

Subject: [PATCH/RFC] [git-subtree.sh] Use raw subject and body modifier "%B" instead of "%s%n%n%b" for commit

## tl;dr

6 messages between Aug 10, 2012 and Jan 1, 2013. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Techlive Zheng· Aug 10, 2012, 01:15 UTC · lore

I don't know if it is the right place to post this patch, I have sended an email to the original author apenwarr and have no response. According to <https://github.com/apenwarr/git-subtree/blob/master/THIS-REPO-IS-OBSOLETE>, this is the place, but <https://github.com/git/git/blob/master/contrib/README> says different, which is really confusing. Anyway, here I am.

Recently, I imported a foreign git project as a sub directory into a main repo which I intend to maintain as primary.

Due to the project I imported has its own remote repo which hosted on the github, I expected after a 'git-subtree.sh split' the newly generated subtree branch would be exactly identical to the original branch. Unfortunately, it is not. I have fixed the committer date and make everything looks the same with the original branch, but they just did not end up with same commit sha1 hash. Then, I used `git cat-file -p` to view the raw output of the both commits and found that the commit generate by git-subtree has a extra 'new-line' character appended at the end of the subject which causes the problem.

I checked the source and found "%s%n%n%b" were used to generate the commit message, this works the fine when a commit has a subject as well as a body, but most of my commits only have a subject under which condition a extra 'new-line' character is appended.

Instead, a raw subject and body message modifier '%B' should be used.

Though I think this patch should be applied by default, but the mistake has been there for a long time, applying this patch may cause the patched git-subtree generate a different branch for those whose subtree branch has already been generated using the old git-subtree. Maybe this should be explained in the help or man page, and add a condition check or a compatible mode somehow.

Techlive Zheng (1):
  subtree.sh: Use raw subject and body modifier "%B" instead of "%s%n%n%b"
 contrib/subtree/git-subtree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
-- 
1.7.11.4
Techlive Zheng· Aug 10, 2012, 01:15 UTC · re: Techlive Zheng · lore

[PATCH] subtree.sh: Use raw subject and body modifier "%B" instead of "%s%n%n%b"

"%s%n%n%b" is not always equal to "%B". If the commit msg does not have a body, this will append an extra new-line character to the msg title which would cause the splited commit has a new sha1 hash. In most cases, this does not matter, but for a project which did not merged using this script initially, the 'split' command would not genereate the same commits as the orginal which may cause conflicts.

Signed-off-by: Techlive Zheng <techlivezheng@gmail.com>
---
 contrib/subtree/git-subtree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/subtree/git-subtree.sh +1 −1
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 920c664..5598210 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -296,7 +296,7 @@ copy_commit()
 	# We're going to set some environment vars here, so
 	# do it in a subshell to get rid of them safely later
 	debug copy_commit "{$1}" "{$2}" "{$3}"
-	git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%s%n%n%b' "$1" |
+	git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%B' "$1" |
 	(
 		read GIT_AUTHOR_NAME
 		read GIT_AUTHOR_EMAIL
-- 
1.7.11.4
greened@obbligato.org· Aug 21, 2012, 01:56 UTC · re: Techlive Zheng · lore

Re: [PATCH] subtree.sh: Use raw subject and body modifier "%B" instead of "%s%n%n%b"

Techlive Zheng <techlivezheng@gmail.com> writes:
Show 25 quoted lines
> "%s%n%n%b" is not always equal to "%B". If the commit msg does not have
> a body, this will append an extra new-line character to the msg title
> which would cause the splited commit has a new sha1 hash. In most cases,
> this does not matter, but for a project which did not merged using this
> script initially, the 'split' command would not genereate the same
> commits as the orginal which may cause conflicts.
>
> Signed-off-by: Techlive Zheng <techlivezheng@gmail.com>
> ---
>  contrib/subtree/git-subtree.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 920c664..5598210 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -296,7 +296,7 @@ copy_commit()
>  	# We're going to set some environment vars here, so
>  	# do it in a subshell to get rid of them safely later
>  	debug copy_commit "{$1}" "{$2}" "{$3}"
> -	git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%s%n%n%b' "$1" |
> +	git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%B' "$1" |
>  	(
>  		read GIT_AUTHOR_NAME
>  		read GIT_AUTHOR_EMAIL

This looks good to me. I assume this passes all the tests. Can you add a test for this bug so we don't regress? Junio, I am good with this patch as soon as we get a test for the problem.

Thanks!
                             -Dave
greened@obbligato.org· Dec 31, 2012, 22:57 UTC · re: greened@obbligato.org · lore

Re: [PATCH] subtree.sh: Use raw subject and body modifier "%B" instead of "%s%n%n%b"

greened@obbligato.org writes:
Show 31 quoted lines
> Techlive Zheng <techlivezheng@gmail.com> writes:
>
>> "%s%n%n%b" is not always equal to "%B". If the commit msg does not have
>> a body, this will append an extra new-line character to the msg title
>> which would cause the splited commit has a new sha1 hash. In most cases,
>> this does not matter, but for a project which did not merged using this
>> script initially, the 'split' command would not genereate the same
>> commits as the orginal which may cause conflicts.
>>
>> Signed-off-by: Techlive Zheng <techlivezheng@gmail.com>
>> ---
>>  contrib/subtree/git-subtree.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 920c664..5598210 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -296,7 +296,7 @@ copy_commit()
>>  	# We're going to set some environment vars here, so
>>  	# do it in a subshell to get rid of them safely later
>>  	debug copy_commit "{$1}" "{$2}" "{$3}"
>> -	git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%s%n%n%b' "$1" |
>> +	git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%B' "$1" |
>>  	(
>>  		read GIT_AUTHOR_NAME
>>  		read GIT_AUTHOR_EMAIL
>
> This looks good to me.  I assume this passes all the tests.  Can you add
> a test for this bug so we don't regress?  Junio, I am good with this
> patch as soon as we get a test for the problem.

I've applied this patch to my working copy but I'm not finding that I can recreate the original problem when the patch is disabled.

I assumed the scenario you're trying to fix is:
- Make some commit C to project A with a one-line message
- work, commit, work...
- Add project A as a subproject
- work, commit, work...
- Split project A off into a separate repository

After this, commit C with the one-line message in the split-off projet should have the same hash it had before project A was incorporated as a subproject.

As I understad it, you saw the post-split commit having a different hash?

Is that right?  I am not seeing that problem even without your patch.

I want to make sure I understand what the problem is so I can test for it.

Thanks!
                      -David
greened@obbligato.org· Jan 1, 2013, 03:59 UTC · re: greened@obbligato.org · lore

Re: [PATCH] subtree.sh: Use raw subject and body modifier "%B" instead of "%s%n%n%b"

greened@obbligato.org writes:
> I've applied this patch to my working copy but I'm not finding that I
> can recreate the original problem when the patch is disabled.
     ^ not
                          -David
greened@obbligato.org· Aug 21, 2012, 01:52 UTC · re: Techlive Zheng · lore

Re: [PATCH/RFC] [git-subtree.sh] Use raw subject and body modifier "%B" instead of "%s%n%n%b" for commit

Techlive Zheng <techlivezheng@gmail.com> writes:
Show 5 quoted lines
> I don't know if it is the right place to post this patch, I have sended
> an email to the original author apenwarr and have no response. According
> to <https://github.com/apenwarr/git-subtree/blob/master/THIS-REPO-IS-OBSOLETE>,
> this is the place, but <https://github.com/git/git/blob/master/contrib/README> says
> different, which is really confusing. Anyway, here I am.
This is the place.
> Recently, I imported a foreign git project as a sub directory into a
> main repo which I intend to maintain as primary.
Ok.
> Due to the project I imported has its own remote repo which hosted
> on the github, I expected after a 'git-subtree.sh split' the newly
> generated subtree branch would be exactly identical to the original
> branch. 
I would have thought so too.
Show 6 quoted lines
> Unfortunately, it is not. I have fixed the committer date and make
> everything looks the same with the original branch, but they just did
> not end up with same commit sha1 hash. Then, I used `git cat-file -p`
> to view the raw output of the both commits and found that the commit
> generate by git-subtree has a extra 'new-line' character appended at
> the end of the subject which causes the problem.
Hmm.
> I checked the source and found "%s%n%n%b" were used to generate the
> commit message, this works the fine when a commit has a subject as
> well as a body, but most of my commits only have a subject under
> which condition a extra 'new-line' character is appended.
Ah.  Yes, we should fix this.
> Instead, a raw subject and body message modifier '%B' should be used.
Ok.
Show 6 quoted lines
> Though I think this patch should be applied by default, but the mistake
> has been there for a long time, applying this patch may cause the patched
> git-subtree generate a different branch for those whose subtree branch
> has already been generated using the old git-subtree. Maybe this should
> be explained in the help or man page, and add a condition check or a
> compatible mode somehow.

The problem is in the split code? I'm not sure this is a big issue. I can run some experiments.

                      -Dave

← back to recent threads