From: Colin Stagner <ask+git@howdoi•land>
To: phillip.wood@dunelm•org.uk, git@vger•kernel.org
Cc: Zach FettersMoore <zach.fetters@apollographql•com>,
Christian Couder <chriscool@tuxfamily•org>,
Patrik Weiskircher <patrik@pspdfkit•com>
Subject: Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees
Date: Tue, 9 Sep 2025 20:56:07 -0500 [thread overview]
Message-ID: <8d341a51-2135-4c62-9df1-5be351e73275@howdoi.land> (raw)
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
next prev parent reply other threads:[~2025-09-10 1:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-24 19:10 [PATCH] contrib/subtree: fix split with squashed subtrees Colin Stagner
2025-09-01 13:54 ` Phillip Wood
2025-09-01 20:43 ` Colin Stagner
2025-09-02 13:22 ` Phillip Wood
2025-09-02 14:57 ` Phillip Wood
2025-09-04 1:34 ` Colin Stagner
2025-09-05 2:27 ` [PATCH v2] " Colin Stagner
2025-09-08 15:21 ` Phillip Wood
2025-09-10 1:56 ` Colin Stagner [this message]
2025-09-10 2:02 ` Junio C Hamano
2025-09-10 3:00 ` Colin Stagner
2025-09-10 15:10 ` Junio C Hamano
2025-09-10 3:11 ` [PATCH v3] " Colin Stagner
2025-09-10 9:39 ` Phillip Wood
2025-09-11 16:01 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8d341a51-2135-4c62-9df1-5be351e73275@howdoi.land \
--to=ask+git@howdoi$(echo .)land \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=patrik@pspdfkit$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=zach.fetters@apollographql$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox