From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 09 Jul 2026 13:19:35 GMT
Subject: Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
Message-ID: <0fc3a36f-dd43-43d1-b260-8e30cf46d845@gmail.com>
In-Reply-To: <27215.27575.968985.583226@chiark.greenend.org.uk>

Hi Ian

On 09/07/2026 10:36, Ian Jackson wrote:
> 
> Colin Stagner writes ("Re: [PATCH 2/2] git-subtree: Bail out if we find output from Rust rewrite (test)"):
>> It may be slightly faster to create only one repo and just make orphan
>> branches, like `test_create_subtree_add()` does.
> ...
>> `test_commit()` from test-lib-functions.sh may be superior to manually
>> writing and committing this file.
> 
> Thanks for the suggestions.  I'll take a look.

I think

     test_commit --no-tag sabotage .git-subtree/config "# sabotage"

is the equivalent of what you have in the test at the moment

> TBH I found this test framework quite awkward to work with.  Maybe
> folks here have some tips:
> 
> One thing I was missing was a primitive for "check this fails *and
> produces an error message matching this regexp*".  test_must_fail
> makes it easy for a slips in the command (or some kinds of regression)
> to go undetected: the test then passes because the command *does* fail
> with a usage error or whatever.  And AFAICT there isn't a way to
> manually inspect the output when the tests pass?  I resorted to
> sabotaging the test by adding `&& false` to the end of the shell
> snippet string, and eyeballing t/test-results/t7900-subtree.out.

The usual approach to checking that a command fails for the expected 
reason is

     test_must_fail git ... 2>err &&
     test_grep regexp err

which prints the contents of err if it does not match regexp. To see the 
output of the tests run them with "-v". I frequently use "-v -i -x" to 
debug test failures. "-i" stops the test run at the first failure so you 
can inspect the test repository and "-x" turns on tracing so you can see 
which command failed which is useful when I test has not been written 
with debugging in mind.

Thanks

Phillip
	
> Colin Stagner writes ("Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite"):
>>> +reject_if_v2_config () {
>>> +	local config=.git-subtree/config
>>
>> This is a nit, but `local` is not specified by POSIX. I know it is used
>> elsewhere within git-subtree, but it is specifically discouraged.
> 
> There are 7 existing uses of `local`.  I think I prefer to use it here
> too.  In practice I think there are no shells we might want to use
> that don't have local.  The alternative is to change all the variable
> names to be obviously globally unique, which is clumsy and also seems
> to me to put us at greater risk of bugs.
> 
>>> +	if git rev-parse --verify -q "$rev:$config"; then
>>
>> For subtree split, should we also test for this file in tree you are
>> splitting: i.e., "$dir/$config"? The answer might be no.
> 
> You're right that we should consider this question.  The answer is:
> no, we should not.  Briefly, whether to use the new or old algorithms
> depends on whether the downstream has adopted the new git-subtree, not
> on whether the upstream has added some optional config.
> 
> https://codeberg.org/diziet/git-subtree/src/branch/main/DATA-MODEL.md#control-of-unmarked-subtree-merges-guessing-config
> 
>> I think that subtree merge should only test the top-level project, as
>> this patch does now.
> 
> By "top-level" I think you mean what I've taken to calling the
> "downstream": the project where the subtree is in a subdir, and whose
> top-level has other stuff.  In which case I agree.
> 
>> On 7/6/26 06:58, Ian Jackson wrote:
>>> Another, bigger, reason is that current git-subtree generates unmarked
>>> subtree merges (ie, without any git-subtree trailers)
>>
>> Subtree merges can be performed without git-subtree, via the `-X
>> subtree` merge strategy option. While the design of RIIR git-subtree is
>> outside the scope of this patch series, this may be worth thinking about
>> in your rewrite.
> 
> This is what I'm calling an "unmarked subtree merge".  My rewrite is
> not going to support this user behaviour.  The problem is that it is
> not possible to reliably determine whetheer something is an unmarked
> subtree merge.
> 
> It is possible to guess based on tree similarity, but that's a
> heuristic.  It's also possible to guess based on root commits.
> Both of these approaches can go wrong in some cases.  I prefer to
> write reliable software, which doesn't guess.
> 
> I'll advise against this practice in the documentation, but I'm
> reasonably confident that if a user does this anyway the results won't
> be terrible.  The upstream input to an unmarked subtree merge in a
> downstream that has already used my rewrite, will be treated as if it
> were a downstream branch that predates the subtree addition.  The
> effect on split (in most cases) is a missing parent relationship,
> which is undesirable but not catastrophic.I've made a note to add a
> test case for this scenario.
> 
> Combining manual -X subtree merges with git-subtree --squash merges
> could easily produce quite weird and wrong results in the tree (even
> before anyone tries split, or something).  I don't think I can even
> reliably detect this situation after the user has done it, and of
> course since that user is using plain git, I certainly can't prevent
> it.  This is another reason why manual use of -X subtree should be
> discouraged.
> 
> Regards,
> Ian.
>