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

[Ian Jackson <ijackson@chiark.greenend.org.uk>]

Hi.  Thanks for the review.  I'll go through it point by point:

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.

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.

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.

-- 
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.