Re: [PATCH v4] subtree: fix split processing with multiple subtrees present
Show 20 quoted lines
>> In the diagram below, 'M' represents the mainline repo branch, 'A'
>> represents one subtree, and 'B' represents another. M3 and B1 represent
>> a split commit for subtree B that was created from commit M4. M2 and A1
>> represent a split commit made from subtree A that was also created
>> based on changes back to and including M4. M1 represents new changes to
>> the repo, in this scenario if you try to run a 'git subtree split
>> --rejoin' for subtree B, commits M1, M2, and A1, will be included in
>> the processing of changes for the new split commit since the last
>> split/rejoin for subtree B was at M3. The issue is that by having A1
>> included in this processing the command ends up needing to processing
>> every commit down tree A even though none of that is needed or relevant
>> to the current command and result.
>>
>> M1
>> | \ \
>> M2 | |
>> | A1 |
>> M3 | |
>> | | B1
>> M4 | |
Show 15 quoted lines
> About the above, Junio already commented the following:
>
> -> The above paragraph explains which different things you drew in the
> -> diagram are representing, but it is not clear how they relate to
> -> each other. Do they for example depict parent-child commit
> -> relationship? What are the wide gaps between these three tracks and
> -> what are the short angled lines leaning to the left near the tip?
> -> Is the time/topology flowing from bottom to top?
>
> and it doesn't look like you have addressed that comment.
>
> When you say "M3 and B1 represent a split commit for subtree B that
> was created from commit M4." I am not sure what it means exactly.
> Could you give example commands that could have created the M3 and B1
> commits?
I removed the diagram from the commit message since it seems a little unclear, and in its place I added an example of an open source repo (which I am currently using the fix in) and the commands to replicate the issue. Hopefully that better illustrates how I came across the issue and what it is.
Show 7 quoted lines
>> So this commit makes a change to the processing of commits for the split
>> command in order to ignore non-mainline commits from other subtrees such
>> as A1 in the diagram by adding a new function
>> 'should_ignore_subtree_commit' which is called during
>> 'process_split_commit'. This allows the split/rejoin processing to still
>> function as expected but removes all of the unnecessary processing that
>> takes place currently which greatly inflates the processing time.
> Could you tell a bit more what kind of processing time reduction is or
> would be possible on what kind of repo? Have you benchmark-ed or just
> timed this somehow on one of your repos or better on an open source
> repo (so that we could reproduce if we wanted)?
I added some extra info for this to the commit message as well, but to answer your question yes I discovered and benchmarked this issue in a repo I help maintain. I was seeing splits take upwards of 12 minutes before the fix, and after they were taking only seconds. Also provided infor on the repo and how to reproduce in the updated commit message.
Show 8 quoted lines
>> Added a test to validate that the proposed fix
>> solves the issue.
>>
>> The test accomplishes this by checking the output
>> of the split command to ensure the output from
>> the progress of 'process_split_commit' function
>> that represents the 'extracount' of commits
>> processed does not increment.
> Does not increment compared to what?
I reworded this to say the 'extracount' remains at 0 since there should be no extra processed commits from the second subtree in the test.
Show 11 quoted lines
>> This was tested against the original functionality
>> to show the test failed, and then with this fix
>> to show the test passes.
>>
>> This illustrated that when using multiple subtrees,
>> A and B, when doing a split on subtree B, the
>> processing does not traverse the entire history
>> of subtree A which is unnecessary and would cause
>> the 'extracount' of processed commits to climb
>> based on the number of commits in the history of
>> subtree A.
> Does this mean that the test checks that the extracount is the same
> when subtree A exists as when it doesn't exist?
This means the test is checking that the 'extracount' remains at 0, because anything above 0 would mean commits from subtree A were being processed, which is where the issue stems from.
Show 16 quoted lines
>> contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-
>> contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++
>> 2 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree=
>.sh
>> index e0c5d3b0de6..e69991a9d80 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -778,6 +778,20 @@ ensure_valid_ref_format () {
>> die "fatal: '$1' does not look like a ref"
>> }
>>
>> +# Usage: check if a commit from another subtree should be
>> +# ignored from processing for splits
>> +should_ignore_subtree_split_commit () {Show 7 quoted lines
> Maybe adding:
>
> assert test $# =3D 1
> local rev=3D"$1"
>
> here, and using $rev instead of $1 in this function could make things
> a bit clearer and similar to what is done in other functions.
Show 11 quoted lines
>> + if test -n "$(git log -1 --grep=3D"git-subtree-dir:" $1)"
>> + then
>> + if test -z "$(git log -1 --grep=3D"git-subtree-mainline:" $1)" &&
>> + test -z "$(git log -1 --grep=3D"git-subtree-dir: =
>> $arg_prefix$" $1)"
>> + then
>> + return 0
>> + fi
>> + fi
>> + return 1
>> +}
> The above doesn't seem to be properly indented. We use tabs not spaces.
Show 13 quoted lines
>> # Usage: process_split_commit REV PARENTS
>> process_split_commit () {
>> assert test $# =3D 2
>> @@ -963,7 +977,20 @@ cmd_split () {
>> eval "$grl" |
>> while read rev parents
>> do
>> - process_split_commit "$rev" "$parents"
>> + if should_ignore_subtree_split_commit "$rev"
>> + then
>> + continue
>> + fi
>> + parsedParents=3D''> It seems to me that we name variables "parsed_parents" (or sometimes
> "parsedparents") rather than "parsedParents".
>> + for parent in $parents
>> + do
>> + should_ignore_subtree_split_commit "$parent"
>> + if test $? -eq 1
> I think the 2 lines above could be replaced by:
>
> + if ! should_ignore_subtree_split_commit "$parent"
>> + then
>> + parsedParents+=3D"$parent "
Show 10 quoted lines
> It doesn't seem to me that we use "+=3D" much in our shell scripts.
> https://www.shellcheck.net/ emits the following:
>
> (warning): In POSIX sh, +=3D is undefined.
>
> so I guess we don't use it because it's not available in some usual shells.
>
> (I haven't checked the script with https://www.shellcheck.net/ before
> and after your patch, but it might help avoid bash-isms and such
> issues.)
Updated this to remove the '+=' usage.
>> + fi
>> + done
>> + process_split_commit "$rev" "$parsedParents"
>> done || exit $?
> It looks like we use "exit $?" a lot in git-subtree.sh while we use
> just "exit" most often elsewhere. Not sure why.
Yea I am unsure of the reasoning of that, I was just trying to follow the what the existing script was already doing.
Show 36 quoted lines
>> latest_new=3D$(cache_get latest_new) || exit $?
>> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900=
>> -subtree.sh
>> index 49a21dd7c9c..87d59afd761 100755
>> --- a/contrib/subtree/t/t7900-subtree.sh
>> +++ b/contrib/subtree/t/t7900-subtree.sh
>> @@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '
>> )
>> '
>>
>> +test_expect_success 'split with multiple subtrees' '
>> + subtree_test_create_repo "$test_count" &&
>> + subtree_test_create_repo "$test_count/subA" &&
>> + subtree_test_create_repo "$test_count/subB" &&
>> + test_create_commit "$test_count" main1 &&
>> + test_create_commit "$test_count/subA" subA1 &&
>> + test_create_commit "$test_count/subA" subA2 &&
>> + test_create_commit "$test_count/subA" subA3 &&
>> + test_create_commit "$test_count/subB" subB1 &&
>> + (
>> + cd "$test_count" &&
>> + git fetch ./subA HEAD &&
>> + git subtree add --prefix=3DsubADir FETCH_HEAD
>> + ) &&
>> + (
>> + cd "$test_count" &&
>> + git fetch ./subB HEAD &&
>> + git subtree add --prefix=3DsubBDir FETCH_HEAD
>> + ) &&
>> + test_create_commit "$test_count" subADir/main-subA1 &&
>> + test_create_commit "$test_count" subBDir/main-subB1 &&
>> + (
>> + cd "$test_count" &&
>> + git subtree split --prefix=3DsubADir --squash --rejoin -m=
>> "Sub A Split 1"
>> + ) &&
Show 8 quoted lines
> Not sure why there are so many sub-shells used, and why the -C option
> is not used instead to tell Git to work in a subdirectory. I guess you
> copied what most existing (old) tests in this test script do.
>
> For example perhaps the 4 line above could be replaced by just:
>
> + git -C "$test_count" subtree split --prefix=3DsubADir
> --squash --rejoin -m "Sub A Split 1" &&
Yea I was following what was being done in other existing tests, although this seems like a better way to do this so I updated the test to remove the extra sub-shells.
Show 19 quoted lines
>> + (
>> + cd "$test_count" &&
>> + git subtree split --prefix=3DsubBDir --squash --rejoin -m=
>> "Sub B Split 1"
>> + ) &&
>> + test_create_commit "$test_count" subADir/main-subA2 &&
>> + test_create_commit "$test_count" subBDir/main-subB2 &&
>> + (
>> + cd "$test_count" &&
>> + git subtree split --prefix=3DsubADir --squash --rejoin -m=
>> "Sub A Split 2"
>> + ) &&
>> + (
>> + cd "$test_count" &&
>> + test "$(git subtree split --prefix=3DsubBDir --squash --r=
>> ejoin \
>> + -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" =3D ""
>> + )
>> +'
> It's not clear to me what the test is doing. Maybe you could split it
> into 2 tests. Perhaps one setting up a repo with multiple subtrees and
> one checking that a new split ignores other subtree split commits.
> Perhaps adding a few comments would help too.
Added some comments before the test to describe the steps the test is taking in order to verify the desired behavior.
On Sat, Nov 18, 2023 at 6:28 AM Christian Couder <christian.couder@gmail.com> wrote:
Show 256 quoted lines
>
> On Thu, Oct 26, 2023 at 9:18 PM Zach FettersMoore via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Zach FettersMoore <zach.fetters@apollographql.com>
> >
> > When there are multiple subtrees present in a repository and they are
> > all using 'git subtree split', the 'split' command can take a
> > significant (and constantly growing) amount of time to run even when
> > using the '--rejoin' flag. This is due to the fact that when processing
> > commits to determine the last known split to start from when looking
> > for changes, if there has been a split/merge done from another subtree
> > there will be 2 split commits, one mainline and one subtree, for the
> > second subtree that are part of the processing. The non-mainline
> > subtree split commit will cause the processing to always need to search
> > the entire history of the given subtree as part of its processing even
> > though those commits are totally irrelevant to the current subtree
> > split being run.
>
> Thanks for your continued work on this!
>
> I am not familiar with git subtree so I might miss obvious things. On
> the other hand, my comments might help increase a bit the number of
> people who could review this patch.
>
> > In the diagram below, 'M' represents the mainline repo branch, 'A'
> > represents one subtree, and 'B' represents another. M3 and B1 represent
> > a split commit for subtree B that was created from commit M4. M2 and A1
> > represent a split commit made from subtree A that was also created
> > based on changes back to and including M4. M1 represents new changes to
> > the repo, in this scenario if you try to run a 'git subtree split
> > --rejoin' for subtree B, commits M1, M2, and A1, will be included in
> > the processing of changes for the new split commit since the last
> > split/rejoin for subtree B was at M3. The issue is that by having A1
> > included in this processing the command ends up needing to processing
> > every commit down tree A even though none of that is needed or relevant
> > to the current command and result.
> >
> > M1
> > | \ \
> > M2 | |
> > | A1 |
> > M3 | |
> > | | B1
> > M4 | |
>
> About the above, Junio already commented the following:
>
> -> The above paragraph explains which different things you drew in the
> -> diagram are representing, but it is not clear how they relate to
> -> each other. Do they for example depict parent-child commit
> -> relationship? What are the wide gaps between these three tracks and
> -> what are the short angled lines leaning to the left near the tip?
> -> Is the time/topology flowing from bottom to top?
>
> and it doesn't look like you have addressed that comment.
>
> When you say "M3 and B1 represent a split commit for subtree B that
> was created from commit M4." I am not sure what it means exactly.
> Could you give example commands that could have created the M3 and B1
> commits?
>
> > So this commit makes a change to the processing of commits for the split
> > command in order to ignore non-mainline commits from other subtrees such
> > as A1 in the diagram by adding a new function
> > 'should_ignore_subtree_commit' which is called during
> > 'process_split_commit'. This allows the split/rejoin processing to still
> > function as expected but removes all of the unnecessary processing that
> > takes place currently which greatly inflates the processing time.
>
> Could you tell a bit more what kind of processing time reduction is or
> would be possible on what kind of repo? Have you benchmark-ed or just
> timed this somehow on one of your repos or better on an open source
> repo (so that we could reproduce if we wanted)?
>
> > Added a test to validate that the proposed fix
> > solves the issue.
> >
> > The test accomplishes this by checking the output
> > of the split command to ensure the output from
> > the progress of 'process_split_commit' function
> > that represents the 'extracount' of commits
> > processed does not increment.
>
> Does not increment compared to what?
>
> > This was tested against the original functionality
> > to show the test failed, and then with this fix
> > to show the test passes.
> >
> > This illustrated that when using multiple subtrees,
> > A and B, when doing a split on subtree B, the
> > processing does not traverse the entire history
> > of subtree A which is unnecessary and would cause
> > the 'extracount' of processed commits to climb
> > based on the number of commits in the history of
> > subtree A.
>
> Does this mean that the test checks that the extracount is the same
> when subtree A exists as when it doesn't exist?
>
> [...]
>
> > contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-
> > contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++
> > 2 files changed, 70 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> > index e0c5d3b0de6..e69991a9d80 100755
> > --- a/contrib/subtree/git-subtree.sh
> > +++ b/contrib/subtree/git-subtree.sh
> > @@ -778,6 +778,20 @@ ensure_valid_ref_format () {
> > die "fatal: '$1' does not look like a ref"
> > }
> >
> > +# Usage: check if a commit from another subtree should be
> > +# ignored from processing for splits
> > +should_ignore_subtree_split_commit () {
>
> Maybe adding:
>
> assert test $# = 1
> local rev="$1"
>
> here, and using $rev instead of $1 in this function could make things
> a bit clearer and similar to what is done in other functions.
>
> > + if test -n "$(git log -1 --grep="git-subtree-dir:" $1)"
> > + then
> > + if test -z "$(git log -1 --grep="git-subtree-mainline:" $1)" &&
> > + test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $1)"
> > + then
> > + return 0
> > + fi
> > + fi
> > + return 1
> > +}
>
> The above doesn't seem to be properly indented. We use tabs not spaces.
>
> > # Usage: process_split_commit REV PARENTS
> > process_split_commit () {
> > assert test $# = 2
> > @@ -963,7 +977,20 @@ cmd_split () {
> > eval "$grl" |
> > while read rev parents
> > do
> > - process_split_commit "$rev" "$parents"
> > + if should_ignore_subtree_split_commit "$rev"
> > + then
> > + continue
> > + fi
> > + parsedParents=''
>
> It seems to me that we name variables "parsed_parents" (or sometimes
> "parsedparents") rather than "parsedParents".
>
> > + for parent in $parents
> > + do
> > + should_ignore_subtree_split_commit "$parent"
> > + if test $? -eq 1
>
> I think the 2 lines above could be replaced by:
>
> + if ! should_ignore_subtree_split_commit "$parent"
>
> > + then
> > + parsedParents+="$parent "
>
> It doesn't seem to me that we use "+=" much in our shell scripts.
> https://www.shellcheck.net/ emits the following:
>
> (warning): In POSIX sh, += is undefined.
>
> so I guess we don't use it because it's not available in some usual shells.
>
> (I haven't checked the script with https://www.shellcheck.net/ before
> and after your patch, but it might help avoid bash-isms and such
> issues.)
>
> > + fi
> > + done
> > + process_split_commit "$rev" "$parsedParents"
> > done || exit $?
>
> It looks like we use "exit $?" a lot in git-subtree.sh while we use
> just "exit" most often elsewhere. Not sure why.
>
> > latest_new=$(cache_get latest_new) || exit $?
> > diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> > index 49a21dd7c9c..87d59afd761 100755
> > --- a/contrib/subtree/t/t7900-subtree.sh
> > +++ b/contrib/subtree/t/t7900-subtree.sh
> > @@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '
> > )
> > '
> >
> > +test_expect_success 'split with multiple subtrees' '
> > + subtree_test_create_repo "$test_count" &&
> > + subtree_test_create_repo "$test_count/subA" &&
> > + subtree_test_create_repo "$test_count/subB" &&
> > + test_create_commit "$test_count" main1 &&
> > + test_create_commit "$test_count/subA" subA1 &&
> > + test_create_commit "$test_count/subA" subA2 &&
> > + test_create_commit "$test_count/subA" subA3 &&
> > + test_create_commit "$test_count/subB" subB1 &&
> > + (
> > + cd "$test_count" &&
> > + git fetch ./subA HEAD &&
> > + git subtree add --prefix=subADir FETCH_HEAD
> > + ) &&
> > + (
> > + cd "$test_count" &&
> > + git fetch ./subB HEAD &&
> > + git subtree add --prefix=subBDir FETCH_HEAD
> > + ) &&
> > + test_create_commit "$test_count" subADir/main-subA1 &&
> > + test_create_commit "$test_count" subBDir/main-subB1 &&
> > + (
> > + cd "$test_count" &&
> > + git subtree split --prefix=subADir --squash --rejoin -m "Sub A Split 1"
> > + ) &&
>
> Not sure why there are so many sub-shells used, and why the -C option
> is not used instead to tell Git to work in a subdirectory. I guess you
> copied what most existing (old) tests in this test script do.
>
> For example perhaps the 4 line above could be replaced by just:
>
> + git -C "$test_count" subtree split --prefix=subADir
> --squash --rejoin -m "Sub A Split 1" &&
>
> > + (
> > + cd "$test_count" &&
> > + git subtree split --prefix=subBDir --squash --rejoin -m "Sub B Split 1"
> > + ) &&
> > + test_create_commit "$test_count" subADir/main-subA2 &&
> > + test_create_commit "$test_count" subBDir/main-subB2 &&
> > + (
> > + cd "$test_count" &&
> > + git subtree split --prefix=subADir --squash --rejoin -m "Sub A Split 2"
> > + ) &&
> > + (
> > + cd "$test_count" &&
> > + test "$(git subtree split --prefix=subBDir --squash --rejoin \
> > + -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
> > + )
> > +'
>
> It's not clear to me what the test is doing. Maybe you could split it
> into 2 tests. Perhaps one setting up a repo with multiple subtrees and
> one checking that a new split ignores other subtree split commits.
> Perhaps adding a few comments would help too.
>
> Best,
> Christian.
Zach FettersMoore via GitGitGadget
