[PATCH 0/2] git-subtree: Bail out if we find output from Rust rewrite

[PATCH 0/2] git-subtree: Bail out if we find output from Rust rewrite

[Ian Jackson]

My rewrite of git-subtree is coming along fairly nicely.  I have done
a bunch of data model design work, as well as successfully implemented
the "split" operation.  (It's several orders of magnitude faster than
the shell script implementation, and produces much better output.)

I have concluded that it is going to be too difficult to have
bidirectional interoperability, for a number of reasons.  One reason
is that I want to change the way split commits are constructed and
they should be as stable as we can make them.

Another, bigger, reason is that current git-subtree generates unmarked
subtree merges (ie, without any git-subtree trailers); reliably
figuring out whether something is such a merge requires a complete
history walk, which is very unfortunate.  To avoid having to do this
in perpetuity, I plan to have new git-subtree assume that commits
which show signs of use of new git-subtree are not unmarked subtree
merges generated by old git-subtree.

So mycompatibility plan is:

 - New git-subtree can read old git-subtree data
 - Old git-subtree (this one here) will *not* handle new data

New git-subtree needs an in-downstream-tree config file with at least
an indication of the downstream project name, because otherwise split
commits can be quite inscrutable.  So the presence of that file is a
convenient way to signal the change.


The purpose of the present patch is to help prevent messes, where old
git-subtree is used *afer* new git-subtree, generating output that no
tooling can handle correctly.  We change old git-subtree to spot the
new git-subtree's config file, and bail out.

Right now there is no new data in existence, becauwe new git-subtree
is not useable.  So right now this change has no effect.

But if we ship this change now, users with this change will be
defended from this lossage, as their collaborators start to use new
git-subtree.  I'm hoping to possibly persuade distros etc. to take
this patch as a backport.

I wasn't sure how precisely to word the error message.  I chose to
refer to the new tool, as if it really exists.  By the time anyone
actually seex this message, it will do.


Ian Jackson (2):
  git-subtree: Bail out if we find output from Rust rewrite
  git-subtree: Bail out if we find output from Rust rewrite (test)

 contrib/subtree/git-subtree.sh     | 18 ++++++++++++++++++
 contrib/subtree/t/t7900-subtree.sh | 18 ++++++++++++++++++
 2 files changed, 36 insertions(+)

-- 
2.47.3

[PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite

[Ian Jackson]

This is going to be forward compatible, but not backward compatible:
projects are expected to adopt the new tool, but not go back to this
old one.

CC: Colin Stagner <ask+git@howdoi.land>
CC: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 contrib/subtree/git-subtree.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 791fd8260c..e9c7ca7cf5 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -278,6 +278,20 @@ main () {
 	"cmd_$arg_command" "$@"
 }
 
+# Usage: reject_if_v2_config REV
+#
+# Bails if we find .git-subtree/config.  This file is used by the RIIR
+# git-subtree, which can read data from this script, but which generates
+# data that this script cannot cope with.  So if we find that the user's
+# project has already been processed with the new tool, we stop, to
+# avoid generating broken output.
+reject_if_v2_config () {
+	local config=.git-subtree/config
+	if git rev-parse --verify -q "$rev:$config"; then
+		die "fatal: tree contains $config: has been processed with new standalone (Rust) git-subtree; use that tool instead of this one.  See https://codeberg.org/diziet/git-subtree https://crates.io/crates/git-subtree"
+	fi
+}
+
 # Usage: cache_setup
 cache_setup () {
 	assert test $# = 0
@@ -846,6 +860,7 @@ process_split_commit () {
 #    Or: cmd_add REPOSITORY REF
 cmd_add () {
 
+	reject_if_v2_config HEAD
 	ensure_clean
 
 	if test $# -eq 1
@@ -934,6 +949,8 @@ cmd_split () {
 		die "fatal: you must provide exactly one revision, and optionally a repository.  Got: '$*'"
 	fi
 
+	reject_if_v2_config "$rev"
+
 	# Now validate prefix against the commit, not the working tree
 	if ! git cat-file -e "$rev:$dir" 2>/dev/null
 	then
@@ -1034,6 +1051,7 @@ cmd_merge () {
 	then
 		repository="$2"
 	fi
+	reject_if_v2_config HEAD
 	ensure_clean
 
 	if test -n "$arg_addmerge_squash"
-- 
2.47.3

Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite

[Junio C Hamano]

Ian Jackson <ijackson@chiark.greenend.org.uk> writes:

> +# Usage: reject_if_v2_config REV
> +#
> +# Bails if we find .git-subtree/config.  This file is used by the RIIR
> +# git-subtree, which can read data from this script, but which generates
> +# data that this script cannot cope with.  So if we find that the user's
> +# project has already been processed with the new tool, we stop, to
> +# avoid generating broken output.
> +reject_if_v2_config () {
> +	local config=.git-subtree/config
> +	if git rev-parse --verify -q "$rev:$config"; then
> +		die "fatal: tree contains $config: has been processed with new standalone (Rust) git-subtree; use that tool instead of this one.  See https://codeberg.org/diziet/git-subtree https://crates.io/crates/git-subtree"
> +	fi
> +}

[warning: I have no idea what is going on in the code we see here,
as I do not use subtree script at all]

The above helper may work for one caller that passes "$rev" but not
for the other caller that passes "HEAD", no?


	if git rev-parse --verify -q "$1:$config"
	then
		die "fatal: tree contains $config: has been processed with new standalone (Rust) git-subtree; use that tool instead of this one.  See https://codeberg.org/diziet/git-subtree https://crates.io/crates/git-subtree"
	fi

Overly long output does not look very easy to read, but I kept it
the same as the original.

> @@ -846,6 +860,7 @@ process_split_commit () {
>  #    Or: cmd_add REPOSITORY REF
>  cmd_add () {
>  
> +	reject_if_v2_config HEAD
>  	ensure_clean

If (global) $rev is not set here, we'd check :.git-subtree/config in
the index in order to detect the v2's configuration.  It seems to me
that this code however wants to inspect HEAD's tree.

> @@ -934,6 +949,8 @@ cmd_split () {
>  		die "fatal: you must provide exactly one revision, and optionally a repository.  Got: '$*'"
>  	fi
>  
> +	reject_if_v2_config "$rev"

This would happen to work, as the global "$rev" visible here is the
same one as what the new helper function sees and uses.

>  	# Now validate prefix against the commit, not the working tree
>  	if ! git cat-file -e "$rev:$dir" 2>/dev/null
>  	then
> @@ -1034,6 +1051,7 @@ cmd_merge () {
>  	then
>  		repository="$2"
>  	fi
> +	reject_if_v2_config HEAD
>  	ensure_clean

The same comment as the one for cmd_add's usage.

>  	if test -n "$arg_addmerge_squash"

Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite

[Ian Jackson]

Hi.  Thanks for the quick review.

Junio C Hamano writes ("Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite"):
> If (global) $rev is not set here, we'd check :.git-subtree/config in
> the index in order to detect the v2's configuration.  It seems to me
> that this code however wants to inspect HEAD's tree.

This was a slip.  The code in reject_if_v2_config is supposed to use
its argument (as per the usage comment I added), not a global.  I'll
fix this with a respin.

(I think it may somehow work by accident in my tests.)

> The above helper may work for one caller that passes "$rev" but not
> for the other caller that passes "HEAD", no?

HEAD is a valid revision spec for git-rev-parse, but the
function should use $1 (which in that case would be HEAD), not $rev.

> 	if git rev-parse --verify -q "$1:$config"
> 	then
> 		die "fatal: tree contains $config: has been processed with new standalone (Rust) git-subtree; use that tool instead of this one.  See https://codeberg.org/diziet/git-subtree https://crates.io/crates/git-subtree"
> 	fi
> 
> Overly long output does not look very easy to read, but I kept it
> the same as the original.

I'm not a great fan of the long error message myself, but it seemed to
be what the rest of the script was doing.  I didn't find any
multi-line calls to die, so that's why I did it this way.

I'm happy to reformat this to your taste.

Thanks,
Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.  

Pronouns: they/he.  If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.

Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite

[Junio C Hamano]

Ian Jackson <ijackson@chiark.greenend.org.uk> writes:

>> 	if git rev-parse --verify -q "$1:$config"
>> 	then
>> 		die "fatal: tree contains $config: has been processed with new standalone (Rust) git-subtree; use that tool instead of this one.  See https://codeberg.org/diziet/git-subtree https://crates.io/crates/git-subtree"
>> 	fi
>> 
>> Overly long output does not look very easy to read, but I kept it
>> the same as the original.
>
> I'm not a great fan of the long error message myself, but it seemed to
> be what the rest of the script was doing.  I didn't find any
> multi-line calls to die, so that's why I did it this way.
>
> I'm happy to reformat this to your taste.

Nah, it seems your plan is to deprecate this script over time and
move everybody to a newer implementation, so as long as "die" does
its job to stop and prevent breakages from spreading, that would be
fine.

Thanks.

[PATCH 2/2] git-subtree: Bail out if we find output from Rust rewrite (test)

[Ian Jackson]

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 contrib/subtree/t/t7900-subtree.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 4194687cfb..e8fa640166 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -439,6 +439,24 @@ test_expect_success 'split sub dir/ with --rejoin' '
 	)
 '
 
+test_expect_success 'split fail on RIIR git subtree data' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD &&
+		# simulate RIIR git-subtree generated data
+		mkdir .git-subtree &&
+		echo "# sabotage" >.git-subtree/config &&
+		git add .git-subtree/config &&
+		git commit -m sabotage &&
+		test_must_fail git subtree split -P "sub dir" HEAD
+	)
+'
+
 # Tests that commits from other subtrees are not processed as
 # part of a split.
 #
-- 
2.47.3