threads / patch / 42896

patchgit-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

Subject: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

## tl;dr

6 messages between Jul 21, 2016 and Jul 22, 2016. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Brett Cundal· Jul 21, 2016, 00:56 UTC · lore

---
 contrib/subtree/git-subtree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to contrib/subtree/git-subtree.sh +1 −2

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7a39b30..556cd92 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -661,7 +661,7 @@ cmd_split()
 	if [ -n "$rejoin" ]; then
 		debug "Merging split branch into HEAD..."
 		latest_old=$(cache_get latest_old)
-		git merge -s ours \
+		git merge -s ours --allow-unrelated-histories \
 			-m "$(rejoin_msg "$dir" $latest_old $latest_new)" \
 			$latest_new >&2 || exit $?
 	fi

--
https://github.com/git/git/pull/274

David Aguilar· Jul 21, 2016, 05:26 UTC · re: Brett Cundal · lore

Re: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

[cc'd Roberto for submitGit q's]

On Thu, Jul 21, 2016 at 12:56:51AM +0000, Brett Cundal wrote:

> ---

<emtpy commit message>

The message on the pull request[1] has a better justification for this change, which would have been nice in the commit message itself:

	Git 2.9 added a check against merging unrelated histories, which
	is exactly what git subtree with --rejoin does. Adding the
	--allow-unrelated-histories flag to merge will override this
	check.

Is it possible that maybe submitGit can detect an empty commit message for single-commit PRs and transplant that message onto it?

As-is, the commit itself should probably be amended to contain that information.

Show 18 quoted lines

>  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 7a39b30..556cd92 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -661,7 +661,7 @@ cmd_split()
>  	if [ -n "$rejoin" ]; then
>  		debug "Merging split branch into HEAD..."
>  		latest_old=$(cache_get latest_old)
> -		git merge -s ours \
> +		git merge -s ours --allow-unrelated-histories \
>  			-m "$(rejoin_msg "$dir" $latest_old $latest_new)" \
>  			$latest_new >&2 || exit $?
>  	fi
> 
> --

With the above description this change makes more sense, but it seems that the existing tests do not detect the breakage fixed by this patch.

Can you please add a test case in t/t7900-subtree.sh demonstrating the breakage?

Looks good otherwise.

[1] https://github.com/git/git/pull/274

-- 
David

Johannes Schindelin· Jul 21, 2016, 15:25 UTC · re: David Aguilar · lore

Re: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

Hi David,

On Wed, 20 Jul 2016, David Aguilar wrote:

> As-is, the commit itself should probably be amended to contain
> that information [the better explanation].

Definitely. Keep in mind: if this gets merged or cherry-picked elsewhere, the Pull Request's message is just as lost.

Ciao, Johannes

Brett Cundal· Jul 21, 2016, 18:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

[re-sending as plain-text so the list software doesn't reject as spam... what decade is this?]

Hey,

Sorry about the empty commit message... there was one originally (albeit not as detailed as the pull request), but I guess it got stripped? As you have probably guessed, I have no idea how you guys do patch submission.

I'm not currently able to spend much time getting up to speed with git development just to submit this fix... I would have just filed a bug, but I couldn't figure out how (I guess it's just this mailing list?) and I figured a patch would be more useful. I had a look at writing a test initially, but I didn't see any existing tests for 'git subtree', and I'm not familiar with the test framework.

The case that fails is basically any usage of split --rejoin... e.g. make a new repo, make changes to directory a, make changes to directory b... do a 'git subtree split -P a --rejoin' and it will fail at the end when attempting to merge the split commits back to the main line, due to the two lines being unrelated (now disallowed by default in git 2.9).

Hopefully I'm not just doing something wrong - I'm surprised such a major bug has not been fixed already... I guess noone else uses --rejoin? Anyhow, this patch fixes the issue for me.

Hope that helps,

-- Brett

On 21 July 2016 at 08:25, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 12 quoted lines

> Hi David,
>
> On Wed, 20 Jul 2016, David Aguilar wrote:
>
>> As-is, the commit itself should probably be amended to contain
>> that information [the better explanation].
>
> Definitely. Keep in mind: if this gets merged or cherry-picked elsewhere,
> the Pull Request's message is just as lost.
>
> Ciao,
> Johannes

Johannes Schindelin· Jul 22, 2016, 08:11 UTC · re: Brett Cundal · lore

Re: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

Hi Brett,

On Thu, 21 Jul 2016, Brett Cundal wrote:

> [re-sending as plain-text so the list software doesn't reject as
> spam... what decade is this?]

I know it feels good to get frustration off of your chest by ranting. I am very sympathetic to that. Please note, however, that it puts the people who are ready to help you immediately into a defensive corner. Probably unintentional?

> Sorry about the empty commit message... there was one originally (albeit
> not as detailed as the pull request), but I guess it got stripped? As
> you have probably guessed, I have no idea how you guys do patch
> submission.

It is okay. For historical reasons, the patch submission is a bit more involved than simply opening a Pull Request. We also frown upon top-posting, among other rules of netiquette. You may dislike it, but you would have to build more of a "street cred" if you truly wanted to try to convince the Git developers to change it.

Back to the patch you wanted to submit: since this is an important bug fix, I spent the time to clean it up. The only missing bit that requires further effort from your side is that we need your Sign-off. See https://github.com/git/git/blob/v2.9.2/Documentation/SubmittingPatches#L239-L291 for an explanation. Essentially, you are stating explicitly that you are not legally prohibited from contributing this patch. If you can say that, simply reply with a

	Signed-off-by: Brett Cundal <brett.cundal@iugome.com>

We can take it from there by inserting it into the following patch:

-- snipsnap --
From: Brett Cundal <brett.cundal@iugome.com>
Subject: [PATCH] subtree: allow unrelated histories when splitting with --rejoin

Git 2.9 added a check against merging unrelated histories, which is
exactly what git subtree with --rejoin does. Adding the
--allow-unrelated-histories flag to merge will override this check.
---
 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 7a39b30..556cd92 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -661,7 +661,7 @@ cmd_split()
 	if [ -n "$rejoin" ]; then
 		debug "Merging split branch into HEAD..."
 		latest_old=$(cache_get latest_old)
-		git merge -s ours \
+		git merge -s ours --allow-unrelated-histories \
 			-m "$(rejoin_msg "$dir" $latest_old $latest_new)" \
 			$latest_new >&2 || exit $?
 	fi

-- 
2.9.0.281.g286a8d9

Brett Cundal· Jul 22, 2016, 17:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

On 22 July 2016 at 01:11, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> I know it feels good to get frustration off of your chest by ranting. I am
> very sympathetic to that. Please note, however, that it puts the people
> who are ready to help you immediately into a defensive corner. Probably
> unintentional?

Shortest rant ever. :) Sorry if I caused any offense. It just seemed very odd to block multi-part MIME that happened to contain a text/html part in addition to text/plain, given that (AFAIK) the vast majority of email clients in recent years produce such a thing. Anyhow, off-topic... your list policy is your business.

Show 11 quoted lines

> Back to the patch you wanted to submit: since this is an important bug
> fix, I spent the time to clean it up. The only missing bit that requires
> further effort from your side is that we need your Sign-off. See
> https://github.com/git/git/blob/v2.9.2/Documentation/SubmittingPatches#L239-L291
> for an explanation. Essentially, you are stating explicitly that you are
> not legally prohibited from contributing this patch. If you can say that,
> simply reply with a
>
>         Signed-off-by: Brett Cundal <brett.cundal@iugome.com>
>
> We can take it from there by inserting it into the following patch:

So, again I'll have to apologise... I should have submitted this as a bug report rather than a patch, since the ownership is not legally clear. Didn't even occur to me for such a trivial fix. If you can just treat this as a bug report at this point and 'reimplement' it, that would probably be the simplest thing for everyone... I guess this may be tricky since there are limited ways to possibly implement this, but for the same reason it would be impossible IMO for me or anyone else to legally claim authorship of it (but IANAL). If this is going to cause this fix to be jammed in limbo for all eternity, then let me know and I'll see what I can do to get the proper authorization.

-- Brett