From: Colin Stagner <ask+git@howdoi.land>
Date: Wed, 10 Sep 2025 01:56:07 GMT
Subject: Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees
Message-ID: <8d341a51-2135-4c62-9df1-5be351e73275@howdoi.land>
In-Reply-To: <b78639ee-021d-49fc-8b8d-0140ed8fc010@gmail.com>

Phillip,

Hello again! I have adopted your recommendations everywhere except for 
`git checkout @{-1}`. Details below.

On 9/8/25 10:21, Phillip Wood wrote:
> On 05/09/2025 03:27, Colin Stagner wrote:

>> +    while read -r trailer val
>> +    do
>> +        case "$trailer" in
>> +        (git-subtree-dir:)
>> +            subtree_dir="${val%/}" ;;
>> +        (git-subtree-mainline:)
>> +            have_mainline=y ;;
>> +        esac
>> +    done
> 
> We do not use the optional '(' in case statements

Will fix in v3.

> 
>> -    if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)"
>> +    if test -n "${subtree_dir:-}" &&
>> +        test -z "${have_mainline:-}" &&
>> +        test "${subtree_dir}" != "$arg_prefix"
> 
> What's the idea behind using "${var:-}" rather than "{var}"?

I write a lot of shell scripts that run "set -u" (aka "set -o nounset"), 
so I do this a lot when testing for empty vars. In this case, it's not 
actually necessary since `have_mainline` is explicitly defined above. 
And we don't run `set -u` anyway.

Will remove from v3.


>> +test_create_subtree_add () {
>> +    (
>> +        cd "$1" &&
>> +        orphan="$2" &&
>> +        prefix="$3" &&
>> +        filename="$4" &&
>> +        shift 4 &&
>> +        last="$(git branch --show-current)" &&
>> +        git checkout --orphan "$orphan" &&
>> +        git rm -rf . &&
> 
> If you use "git switch --orphan" that clears the worktree for you

Very useful. I'll start using it in v3.


>> +        test_commit "$filename" &&
>> +        git checkout "$last" &&
> 
> I think this could be "git checkout @{-1}" and then we'd avoid having to 
> run "git branch" above

I experimented with this, but I couldn't get it to work on git 2.44. 
Although the reflog shows the refs I expect, using

     git switch '@{-1}'

dies with

     fatal: invalid reference: @{-1}

checkout doesn't work either. Perhaps there is something about --orphan 
that is messing up the history.

I could make `test_create_subtree_add` take a mainline branch name, 
but... unless there's something unsound about v2, I think we should just 
keep v2. `git branch --show-current` looks like well-defined porcelain.

Any other ideas?


>> +# The test covers:
>> +# - An initial `subtree add`; and
>> +# - A follow-up `subtree merge`
>> +# both with and without `--squashed`.
>> +for is_squashed in '' 'y';
> 
> no need for ';' at the end of the line

Fixed for v3.

>> +        subtree_test_create_repo "$test_count" &&
>> +        (
>> +            cd "$test_count" &&
>> +            mkdir subA &&
>> +            test_commit subA/file1 &&
>> +            git branch -m main &&
> 
> For tests that depend on the default branch name you can add
> 
>      GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>      export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> 
> to the start of the file before it sources test-lib.sh.

GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main looks very common, so I'll go 
with that for v3.


>> +            test_create_subtree_add \
>> +                . mksubtree subA/subB file2 ${is_squashed:+--squash} &&
>> +            test -e subA/file1.t &&
> 
> We have test_path_is_file() for this which prints a useful diagnostic 
> message

Fixed all occurrences in v3.


Colin