* Re: [PATCH v4] subtree: fix split processing with multiple subtrees present
From: Zach FettersMoore @ 2023-11-28 21:04 UTC (permalink / raw)
To: Christian Couder; +Cc: Zach FettersMoore via GitGitGadget, git
In-Reply-To: <CAP8UFD18Hh=m8HQibAgZW1KNAn6zg_rxe9asg0ViC5z27W=Smw@mail.gmail.com>
>> 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?
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.
>> 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.
>> 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.
>> 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.
>> 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 $# =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.
Updated.
>> + 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.
Fixed.
>> # 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".
Fixed.
>> + 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"
Updated.
>> + then
>> + parsedParents+=3D"$parent "
> 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.
>> 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"
>> + ) &&
> 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.
>> + (
>> + 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:
>
> 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.
^ permalink raw reply
* [PATCH v5] subtree: fix split processing with multiple subtrees present
From: Zach FettersMoore via GitGitGadget @ 2023-11-28 21:17 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Zach FettersMoore, Zach FettersMoore
In-Reply-To: <pull.1587.v4.git.1698347871200.gitgitgadget@gmail.com>
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.
To see this in practice you can use the open source GitHub repo
'apollo-ios-dev' and do the following in order:
-Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'
directories
-Create a commit containing these changes
-Do a split on apollo-ios-codegen
- git subtree split --prefix=apollo-ios-codegen --squash --rejoin
-Do a split on apollo-ios
- git subtree split --prefix=apollo-ios --squash --rejoin
-Make changes to a file in apollo-ios-codegen
-Create a commit containing the change(s)
-Do a split on apollo-ios-codegen
- git subtree split --prefix=apollo-ios-codegen --squash --rejoin
You will see that the final split is looking for the last split
on apollo-ios-codegen to use as it's starting point to process
commits. Since there is a split commit from apollo-ios in between the
2 splits run on apollo-ios-codegen, the processing ends up traversing
the entire history of apollo-ios which increases the time it takes to
do a split based on how long of a history apollo-ios has, while none
of these commits are relevant to the split being done on
apollo-ios-codegen.
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 apollo-ios in the above breakdown 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. In the above example, previously the final split
would take ~10-12 minutes, while after this fix it takes seconds.
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 remains at 0, meaning none of the commits
from the second subtree were processed.
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.
Signed-off-by: Zach FettersMoore <zach.fetters@apollographql.com>
---
subtree: fix split processing with multiple subtrees present
When there are multiple subtrees in a repo and git subtree split
--rejoin is being used for the subtrees, the processing of commits for a
new split can take a significant (and constantly growing) amount of time
because the split commits from other subtrees cause the processing to
have to scan the entire history of the other subtree(s). This patch
filters out the other subtree split commits that are unnecessary for the
split commit processing.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1587
Range-diff vs v4:
1: 353152910eb ! 1: e7445a95f30 subtree: fix split processing with multiple subtrees present
@@ Commit message
though those commits are totally irrelevant to the current subtree
split being run.
- 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.
+ To see this in practice you can use the open source GitHub repo
+ 'apollo-ios-dev' and do the following in order:
- M1
- | \ \
- M2 | |
- | A1 |
- M3 | |
- | | B1
- M4 | |
+ -Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'
+ directories
+ -Create a commit containing these changes
+ -Do a split on apollo-ios-codegen
+ - git subtree split --prefix=apollo-ios-codegen --squash --rejoin
+ -Do a split on apollo-ios
+ - git subtree split --prefix=apollo-ios --squash --rejoin
+ -Make changes to a file in apollo-ios-codegen
+ -Create a commit containing the change(s)
+ -Do a split on apollo-ios-codegen
+ - git subtree split --prefix=apollo-ios-codegen --squash --rejoin
- 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.
+ You will see that the final split is looking for the last split
+ on apollo-ios-codegen to use as it's starting point to process
+ commits. Since there is a split commit from apollo-ios in between the
+ 2 splits run on apollo-ios-codegen, the processing ends up traversing
+ the entire history of apollo-ios which increases the time it takes to
+ do a split based on how long of a history apollo-ios has, while none
+ of these commits are relevant to the split being done on
+ apollo-ios-codegen.
+
+ 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 apollo-ios in the above breakdown 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. In the above example, previously the final split
+ would take ~10-12 minutes, while after this fix it takes seconds.
Added a test to validate that the proposed fix
solves the issue.
@@ Commit message
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.
+ processed remains at 0, meaning none of the commits
+ from the second subtree were processed.
This was tested against the original functionality
to show the test failed, and then with this fix
@@ contrib/subtree/git-subtree.sh: ensure_valid_ref_format () {
+# Usage: check if a commit from another subtree should be
+# ignored from processing for splits
+should_ignore_subtree_split_commit () {
-+ 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
++ assert test $# = 1
++ local rev="$1"
++ if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)"
++ then
++ if test -z "$(git log -1 --grep="git-subtree-mainline:" $rev)" &&
++ test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $rev)"
++ then
++ return 0
++ fi
++ fi
++ return 1
+}
+
# Usage: process_split_commit REV PARENTS
@@ contrib/subtree/git-subtree.sh: cmd_split () {
+ then
+ continue
+ fi
-+ parsedParents=''
++ parsedparents=''
+ for parent in $parents
+ do
-+ should_ignore_subtree_split_commit "$parent"
-+ if test $? -eq 1
++ if ! should_ignore_subtree_split_commit "$parent"
+ then
-+ parsedParents+="$parent "
++ parsedparents="$parsedparents$parent "
+ fi
+ done
-+ process_split_commit "$rev" "$parsedParents"
++ process_split_commit "$rev" "$parsedparents"
done || exit $?
latest_new=$(cache_get latest_new) || exit $?
@@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --r
)
'
++# Tests that commits from other subtrees are not processed as
++# part of a split.
++#
++# This test performs the following:
++# - Creates Repo with subtrees 'subA' and 'subB'
++# - Creates commits in the repo including changes to subtrees
++# - Runs the following 'split' and commit' commands in order:
++# - Perform 'split' on subtree A
++# - Perform 'split' on subtree B
++# - Create new commits with changes to subtree A and B
++# - Perform split on subtree A
++# - Check that the commits in subtree B are not processed
++# as part of the subtree A split
+test_expect_success 'split with multiple subtrees' '
+ subtree_test_create_repo "$test_count" &&
+ subtree_test_create_repo "$test_count/subA" &&
@@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --r
+ 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
-+ ) &&
++ git -C "$test_count" fetch ./subA HEAD &&
++ git -C "$test_count" subtree add --prefix=subADir FETCH_HEAD &&
++ git -C "$test_count" fetch ./subB HEAD &&
++ git -C "$test_count" 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"
-+ ) &&
-+ (
-+ cd "$test_count" &&
-+ git subtree split --prefix=subBDir --squash --rejoin -m "Sub B Split 1"
-+ ) &&
++ git -C "$test_count" subtree split --prefix=subADir \
++ --squash --rejoin -m "Sub A Split 1" &&
++ git -C "$test_count" 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\]")" = ""
-+ )
++ git -C "$test_count" subtree split --prefix=subADir \
++ --squash --rejoin -m "Sub A Split 2" &&
++ test "$(git -C "$test_count" subtree split --prefix=subBDir \
++ --squash --rejoin -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
+'
+
test_expect_success 'split sub dir/ with --rejoin from scratch' '
contrib/subtree/git-subtree.sh | 30 +++++++++++++++++++++-
contrib/subtree/t/t7900-subtree.sh | 40 ++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..a0bf958ea66 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -778,6 +778,22 @@ 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 () {
+ assert test $# = 1
+ local rev="$1"
+ if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)"
+ then
+ if test -z "$(git log -1 --grep="git-subtree-mainline:" $rev)" &&
+ test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $rev)"
+ then
+ return 0
+ fi
+ fi
+ return 1
+}
+
# Usage: process_split_commit REV PARENTS
process_split_commit () {
assert test $# = 2
@@ -963,7 +979,19 @@ 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=''
+ for parent in $parents
+ do
+ if ! should_ignore_subtree_split_commit "$parent"
+ then
+ parsedparents="$parsedparents$parent "
+ fi
+ done
+ process_split_commit "$rev" "$parsedparents"
done || exit $?
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..ca4df5be832 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -385,6 +385,46 @@ test_expect_success 'split sub dir/ with --rejoin' '
)
'
+# Tests that commits from other subtrees are not processed as
+# part of a split.
+#
+# This test performs the following:
+# - Creates Repo with subtrees 'subA' and 'subB'
+# - Creates commits in the repo including changes to subtrees
+# - Runs the following 'split' and commit' commands in order:
+# - Perform 'split' on subtree A
+# - Perform 'split' on subtree B
+# - Create new commits with changes to subtree A and B
+# - Perform split on subtree A
+# - Check that the commits in subtree B are not processed
+# as part of the subtree A split
+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 &&
+ git -C "$test_count" fetch ./subA HEAD &&
+ git -C "$test_count" subtree add --prefix=subADir FETCH_HEAD &&
+ git -C "$test_count" fetch ./subB HEAD &&
+ git -C "$test_count" subtree add --prefix=subBDir FETCH_HEAD &&
+ test_create_commit "$test_count" subADir/main-subA1 &&
+ test_create_commit "$test_count" subBDir/main-subB1 &&
+ git -C "$test_count" subtree split --prefix=subADir \
+ --squash --rejoin -m "Sub A Split 1" &&
+ git -C "$test_count" 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 &&
+ git -C "$test_count" subtree split --prefix=subADir \
+ --squash --rejoin -m "Sub A Split 2" &&
+ test "$(git -C "$test_count" subtree split --prefix=subBDir \
+ --squash --rejoin -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
+'
+
test_expect_success 'split sub dir/ with --rejoin from scratch' '
subtree_test_create_repo "$test_count" &&
test_create_commit "$test_count" main1 &&
base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
--
gitgitgadget
^ permalink raw reply related
* My hub
From: Mo Mu @ 2023-11-28 23:50 UTC (permalink / raw)
To: git
Sent from my iPhone
^ permalink raw reply
* [PATCH 00/10] t: more compatibility fixes with reftables
From: Patrick Steinhardt @ 2023-11-29 7:24 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]
Hi,
this is the second patch series that refactors tests to become
compatible with the upcoming reftables backend. It's in the same spirit
as the first round of patches [1], where most of the refactorings are to
use plumbing tools to access refs or the reflog instead of modifying
on-disk data structures directly.
Patrick
[1]: <cover.1697607222.git.ps@pks.im>
Patrick Steinhardt (10):
t0410: mark tests to require the reffiles backend
t1400: split up generic reflog tests from the reffile-specific ones
t1401: stop treating FETCH_HEAD as real reference
t1410: use test-tool to create empty reflog
t1417: make `reflog --updateref` tests backend agnostic
t3310: stop checking for reference existence via `test -f`
t4013: simplify magic parsing and drop "failure"
t5401: speed up creation of many branches
t5551: stop writing packed-refs directly
t6301: write invalid object ID via `test-tool ref-store`
t/t0410-partial-clone.sh | 4 +--
t/t1400-update-ref.sh | 41 +++++++++++++++++++++++----
t/t1401-symbolic-ref.sh | 4 +--
t/t1410-reflog.sh | 4 +--
t/t1417-reflog-updateref.sh | 10 +++++--
t/t3310-notes-merge-manual-resolve.sh | 6 ++--
t/t4013-diff-various.sh | 27 ++++++++----------
t/t5401-update-hooks.sh | 6 ++--
t/t5551-http-fetch-smart.sh | 4 ++-
t/t6301-for-each-ref-errors.sh | 13 ++++-----
10 files changed, 74 insertions(+), 45 deletions(-)
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 01/10] t0410: mark tests to require the reffiles backend
From: Patrick Steinhardt @ 2023-11-29 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]
Two of our tests in t0410 verify whether partial clones end up with the
correct repository format version and extensions. These checks require
the reffiles backend because every other backend would by necessity bump
the repository format version to be at least 1.
Mark the tests accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t0410-partial-clone.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 5b7bee888d..6b6424b3df 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -49,7 +49,7 @@ test_expect_success 'convert shallow clone to partial clone' '
test_cmp_config -C client 1 core.repositoryformatversion
'
-test_expect_success SHA1 'convert to partial clone with noop extension' '
+test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
@@ -60,7 +60,7 @@ test_expect_success SHA1 'convert to partial clone with noop extension' '
git -C client fetch --unshallow --filter="blob:none"
'
-test_expect_success SHA1 'converting to partial clone fails with unrecognized extension' '
+test_expect_success SHA1,REFFILES 'converting to partial clone fails with unrecognized extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 02/10] t1400: split up generic reflog tests from the reffile-specific ones
From: Patrick Steinhardt @ 2023-11-29 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3840 bytes --]
We have a bunch of tests in t1400 that check whether we correctly read
reflog entries. These tests create the reflog by manually writing to the
respective loose file, which makes them specific to the files backend.
But while some of them do indeed exercise very specific edge cases in
the reffiles backend, most of the tests exercise generic functionality
that should be common to all backends.
Unfortunately, we can't easily adapt all of the tests to work with all
backends. Instead, split out the reffile-specific tests from the ones
that should work with all backends and refactor the generic ones to not
write to the on-disk files directly anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1400-update-ref.sh | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 9ac4b7036b..c41cd9b6bf 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -354,14 +354,28 @@ test_expect_success "verifying $m's log (logged by config)" '
'
test_expect_success 'set up for querying the reflog' '
+ git update-ref -d $m &&
+ test-tool ref-store main delete-reflog $m &&
+
+ GIT_COMMITTER_DATE="1117150320 -0500" git update-ref $m $C &&
+ GIT_COMMITTER_DATE="1117150350 -0500" git update-ref $m $A &&
+ GIT_COMMITTER_DATE="1117150380 -0500" git update-ref $m $B &&
+ GIT_COMMITTER_DATE="1117150680 -0500" git update-ref $m $F &&
+ GIT_COMMITTER_DATE="1117150980 -0500" git update-ref $m $E &&
git update-ref $m $D &&
- cat >.git/logs/$m <<-EOF
+ # Delete the last reflog entry so that the tip of m and the reflog for
+ # it disagree.
+ git reflog delete $m@{0} &&
+
+ cat >expect <<-EOF &&
$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
- $F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
- $Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
+ $B $F $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+ $F $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
EOF
+ test-tool ref-store main for-each-reflog-ent $m >actual &&
+ test_cmp expect actual
'
ed="Thu, 26 May 2005 18:32:00 -0500"
@@ -409,13 +423,12 @@ test_expect_success 'Query "main@{2005-05-26 23:33:01}" (middle of history with
test_when_finished "rm -f o e" &&
git rev-parse --verify "main@{2005-05-26 23:33:01}" >o 2>e &&
echo "$B" >expect &&
- test_cmp expect o &&
- test_grep -F "warning: log for ref $m has gap after $gd" e
+ test_cmp expect o
'
test_expect_success 'Query "main@{2005-05-26 23:38:00}" (middle of history)' '
test_when_finished "rm -f o e" &&
git rev-parse --verify "main@{2005-05-26 23:38:00}" >o 2>e &&
- echo "$Z" >expect &&
+ echo "$F" >expect &&
test_cmp expect o &&
test_must_be_empty e
'
@@ -436,6 +449,22 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
rm -f .git/$m .git/logs/$m expect
+test_expect_success REFFILES 'query reflog with gap' '
+ test_when_finished "git update-ref -d $m" &&
+
+ git update-ref $m $F &&
+ cat >.git/logs/$m <<-EOF &&
+ $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
+ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
+ $D $F $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+ EOF
+
+ git rev-parse --verify "main@{2005-05-26 23:33:01}" >actual 2>stderr &&
+ echo "$B" >expect &&
+ test_cmp expect actual &&
+ test_grep -F "warning: log for ref $m has gap after $gd" stderr
+'
+
test_expect_success 'creating initial files' '
test_when_finished rm -f M &&
echo TEST >F &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 03/10] t1401: stop treating FETCH_HEAD as real reference
From: Patrick Steinhardt @ 2023-11-29 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]
One of the tests in t1401 asserts that we can create a symref from a
symbolic reference to a top-level reference, which is done by linking
from `refs/heads/top-level` to `FETCH_HEAD`. But `FETCH_HEAD` is not a
proper reference and doesn't even follow the loose reference format, so
it is not a good candidate for the logic under test.
Refactor the test to use `ORIG_HEAD` instead of `FETCH_HEAD`. This also
works with other backends than the reffiles one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1401-symbolic-ref.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index c7745e1bf6..3241d35917 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -171,8 +171,8 @@ test_expect_success 'symbolic-ref refuses invalid target for non-HEAD' '
'
test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '
- git symbolic-ref refs/heads/top-level FETCH_HEAD &&
- git update-ref FETCH_HEAD HEAD &&
+ git symbolic-ref refs/heads/top-level ORIG_HEAD &&
+ git update-ref ORIG_HEAD HEAD &&
test_cmp_rev top-level HEAD
'
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 04/10] t1410: use test-tool to create empty reflog
From: Patrick Steinhardt @ 2023-11-29 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]
One of the tests in t1410 is marked to be specific to the files
reference backend, which is because we create a reflog manually by
creating the respective file. Refactor the test to instead use our
`test-tool ref-store` helper to create the reflog so that it works with
other reference backends, as well.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1410-reflog.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index aeddc2fb3f..a0ff8d51f0 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -469,11 +469,11 @@ test_expect_success 'expire one of multiple worktrees' '
)
'
-test_expect_success REFFILES 'empty reflog' '
+test_expect_success 'empty reflog' '
test_when_finished "rm -rf empty" &&
git init empty &&
test_commit -C empty A &&
- >empty/.git/logs/refs/heads/foo &&
+ test-tool ref-store main create-reflog refs/heads/foo &&
git -C empty reflog expire --all 2>err &&
test_must_be_empty err
'
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 05/10] t1417: make `reflog --updateref` tests backend agnostic
From: Patrick Steinhardt @ 2023-11-29 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]
The tests for `git reflog delete --updateref` are currently marked to
only run with the reffiles backend. There is no inherent reason that
this should be the case other than the fact that the setup messes with
the on-disk reflogs directly.
Refactor the test to stop doing so and drop the REFFILES prerequisite.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1417-reflog-updateref.sh | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/t/t1417-reflog-updateref.sh b/t/t1417-reflog-updateref.sh
index 14f13b57c6..0eb5e674bc 100755
--- a/t/t1417-reflog-updateref.sh
+++ b/t/t1417-reflog-updateref.sh
@@ -14,9 +14,13 @@ test_expect_success 'setup' '
test_commit B &&
test_commit C &&
- cp .git/logs/HEAD HEAD.old &&
+ git reflog HEAD >expect &&
git reset --hard HEAD~ &&
- cp HEAD.old .git/logs/HEAD
+ # Make sure that the reflog does not point to the same commit
+ # as HEAD.
+ git reflog delete HEAD@{0} &&
+ git reflog HEAD >actual &&
+ test_cmp expect actual
)
'
@@ -25,7 +29,7 @@ test_reflog_updateref () {
shift
args="$@"
- test_expect_success REFFILES "get '$exp' with '$args'" '
+ test_expect_success "get '$exp' with '$args'" '
test_when_finished "rm -rf copy" &&
cp -R repo copy &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 06/10] t3310: stop checking for reference existence via `test -f`
From: Patrick Steinhardt @ 2023-11-29 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]
One of the tests in t3310 exercises whether the special references
`NOTES_MERGE_PARTIAL` and `NOTES_MERGE_REF` exist as expected when the
notes subsystem runs into a merge conflict. This is done by checking
on-disk data structures directly though instead of asking the reference
backend.
Refactor the test to use git-rev-parse(1) instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t3310-notes-merge-manual-resolve.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 60d6ed2dc8..597df5ebc0 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -561,9 +561,9 @@ y and z notes on 4th commit
EOF
# Fail to finalize merge
test_must_fail git notes merge --commit >output 2>&1 &&
- # .git/NOTES_MERGE_* must remain
- test -f .git/NOTES_MERGE_PARTIAL &&
- test -f .git/NOTES_MERGE_REF &&
+ # NOTES_MERGE_* refs and .git/NOTES_MERGE_* state files must remain
+ git rev-parse --verify NOTES_MERGE_PARTIAL &&
+ git rev-parse --verify NOTES_MERGE_REF &&
test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 07/10] t4013: simplify magic parsing and drop "failure"
From: Patrick Steinhardt @ 2023-11-29 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]
In t14013, we have various different tests that verify whether certain
diffs are generated as expected. As much of the logic is the same across
many of the tests we some common code in there that generates the actual
test cases for us.
As some diffs are more special than others depending on the command line
parameters passed to git-diff(1), these tests need to adapt behaviour to
the specific test case sometimes. This is done via colon-prefixed magic
commands, of which we currently know "failure" and "noellipses". The
logic to parse this magic is a bit convoluted though and hard to grasp,
also due to the rather unnecessary nesting.
Un-nest the cases so that it becomes a bit more straightfoward. The
logic is further simplified by removing support for the "failure" magic,
which is not actually used anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t4013-diff-various.sh | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 5cc17c2e0d..76b8619e2e 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -178,32 +178,29 @@ process_diffs () {
V=$(git version | sed -e 's/^git version //' -e 's/\./\\./g')
while read magic cmd
do
- status=success
case "$magic" in
'' | '#'*)
continue ;;
- :*)
- magic=${magic#:}
+ :noellipses)
+ magic=noellipses
label="$magic-$cmd"
- case "$magic" in
- noellipses) ;;
- failure)
- status=failure
- magic=
- label="$cmd" ;;
- *)
- BUG "unknown magic $magic" ;;
- esac ;;
+ ;;
+ :*)
+ BUG "unknown magic $magic"
+ ;;
*)
- cmd="$magic $cmd" magic=
- label="$cmd" ;;
+ cmd="$magic $cmd"
+ magic=
+ label="$cmd"
+ ;;
esac
+
test=$(echo "$label" | sed -e 's|[/ ][/ ]*|_|g')
pfx=$(printf "%04d" $test_count)
expect="$TEST_DIRECTORY/t4013/diff.$test"
actual="$pfx-diff.$test"
- test_expect_$status "git $cmd # magic is ${magic:-(not used)}" '
+ test_expect_success "git $cmd # magic is ${magic:-(not used)}" '
{
echo "$ git $cmd"
case "$magic" in
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 08/10] t5401: speed up creation of many branches
From: Patrick Steinhardt @ 2023-11-29 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]
One of the tests in t5401 creates a bunch of branches by calling
git-branch(1) for every one of them. This is quite inefficient and takes
a comparatively long time even on Unix systems where spawning processes
is comparatively fast. Refactor it to instead use git-update-ref(1),
which leads to an almost 10-fold speedup:
```
Benchmark 1: ./t5401-update-hooks.sh (rev = HEAD)
Time (mean ± σ): 983.2 ms ± 97.6 ms [User: 328.8 ms, System: 679.2 ms]
Range (min … max): 882.9 ms … 1078.0 ms 3 runs
Benchmark 2: ./t5401-update-hooks.sh (rev = HEAD~)
Time (mean ± σ): 9.312 s ± 0.398 s [User: 2.766 s, System: 6.617 s]
Range (min … max): 8.885 s … 9.674 s 3 runs
Summary
./t5401-update-hooks.sh (rev = HEAD) ran
9.47 ± 1.02 times faster than ./t5401-update-hooks.sh (rev = HEAD~)
```
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5401-update-hooks.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 001b7a17ad..8b8bc47dc0 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -133,10 +133,8 @@ test_expect_success 'pre-receive hook that forgets to read its input' '
EOF
rm -f victim.git/hooks/update victim.git/hooks/post-update &&
- for v in $(test_seq 100 999)
- do
- git branch branch_$v main || return
- done &&
+ printf "create refs/heads/branch_%d main\n" $(test_seq 100 999) >input &&
+ git update-ref --stdin <input &&
git push ./victim.git "+refs/heads/*:refs/heads/*"
'
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 09/10] t5551: stop writing packed-refs directly
From: Patrick Steinhardt @ 2023-11-29 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]
We have multiple tests in t5551 that write thousands of tags. To do so
efficiently we generate the tags by writing the `packed-refs` file
directly, which of course assumes that the reference database is backed
by the files backend.
Refactor the code to instead use a single `git update-ref --stdin`
command to write the tags. While the on-disk end result is not the same
as we now have a bunch of loose refs instead of a single packed-refs
file, the distinction shouldn't really matter for any of the tests that
use this helper.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5551-http-fetch-smart.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8a41adf1e1..e069737b80 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -359,7 +359,9 @@ create_tags () {
# now assign tags to all the dangling commits we created above
tag=$(perl -e "print \"bla\" x 30") &&
- sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
+ sed -e "s|^:\([^ ]*\) \(.*\)$|create refs/tags/$tag-\1 \2|" <marks >input &&
+ git update-ref --stdin <input &&
+ rm input
}
test_expect_success 'create 2,000 tags in the repo' '
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 10/10] t6301: write invalid object ID via `test-tool ref-store`
From: Patrick Steinhardt @ 2023-11-29 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701242407.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
One of the tests in t6301 verifies that the reference backend correctly
warns about the case where a reference points to a non-existent object.
This is done by writing the object ID into the loose reference directly,
which is quite intimate with how the files backend works.
Refactor the code to instead use `test-tool ref-store` to write the
reference, which is backend-agnostic.
There are two more tests in this file which write loose files directly,
as well. But both of them are indeed quite specific to the loose files
backend and cannot be easily ported to other backends. We thus mark them
as requiring the REFFILES prerequisite.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t6301-for-each-ref-errors.sh | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 2667dd13fe..83b8a19d94 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -15,7 +15,7 @@ test_expect_success setup '
git for-each-ref --format="%(objectname) %(refname)" >brief-list
'
-test_expect_success 'Broken refs are reported correctly' '
+test_expect_success REFFILES 'Broken refs are reported correctly' '
r=refs/heads/bogus &&
: >.git/$r &&
test_when_finished "rm -f .git/$r" &&
@@ -25,7 +25,7 @@ test_expect_success 'Broken refs are reported correctly' '
test_cmp broken-err err
'
-test_expect_success 'NULL_SHA1 refs are reported correctly' '
+test_expect_success REFFILES 'NULL_SHA1 refs are reported correctly' '
r=refs/heads/zeros &&
echo $ZEROS >.git/$r &&
test_when_finished "rm -f .git/$r" &&
@@ -39,15 +39,14 @@ test_expect_success 'NULL_SHA1 refs are reported correctly' '
'
test_expect_success 'Missing objects are reported correctly' '
- r=refs/heads/missing &&
- echo $MISSING >.git/$r &&
- test_when_finished "rm -f .git/$r" &&
- echo "fatal: missing object $MISSING for $r" >missing-err &&
+ test_when_finished "git update-ref -d refs/heads/missing" &&
+ test-tool ref-store main update-ref msg refs/heads/missing "$MISSING" "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
+ echo "fatal: missing object $MISSING for refs/heads/missing" >missing-err &&
test_must_fail git for-each-ref 2>err &&
test_cmp missing-err err &&
(
cat brief-list &&
- echo "$MISSING $r"
+ echo "$MISSING refs/heads/missing"
) | sort -k 2 >missing-brief-expected &&
git for-each-ref --format="%(objectname) %(refname)" >brief-out 2>brief-err &&
test_cmp missing-brief-expected brief-out &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/4] refs: improve handling of special refs
From: Patrick Steinhardt @ 2023-11-29 8:14 UTC (permalink / raw)
To: git; +Cc: hanwenn
[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]
Hi,
there are a bunch of "special" refs in Git that sometimes behave like a
normal reference and sometimes they don't. These references are written
to directly via the filesystem without going through the reference
backend, but the expectation is that those references can then be read
by things like git-rev-parse(1).
We do not currently have a single source of truth for what those special
refs are, and we also don't have clear rules for how they should be
written to. This works in the context of the files backend, because it
is able to read back such manually-written loose references just fine.
But once the reftable backend lands this will stop working.
This patch series tries to improve this state by doing two things:
1. We explicitly mark these references as special by introducing a
new `is_special_ref()` function. This serves as documentation,
but will also cause us to explicitly read all of these special
refs via loose files regardless of the actual backend.
2. We document a new rule around writing refs. Namely, normal
references are _always_ written via the reference backend,
whereas special references are _always_ written directly via the
filesystem. This rule is not enforced anywhere, but at least it's
now made more explicit.
The last patch fixes one of the instances where we treat a reference
inconsistently by converting it to a normal reference. We can eventually
migrate more of the special refs to become normal refs as we deem fit,
but I consider this to be out of scope for this patch series.
These patches improve compatibility with the new reftable backend.
Patrick
Patrick Steinhardt (4):
wt-status: read HEAD and ORIG_HEAD via the refdb
refs: propagate errno when reading special refs fails
refs: complete list of special refs
bisect: consistently write BISECT_EXPECTED_REV via the refdb
bisect.c | 25 +++------------
builtin/bisect.c | 8 ++---
refs.c | 64 +++++++++++++++++++++++++++++++++++--
t/t1403-show-ref.sh | 9 ++++++
t/t6030-bisect-porcelain.sh | 2 +-
wt-status.c | 17 +++++-----
6 files changed, 86 insertions(+), 39 deletions(-)
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb
From: Patrick Steinhardt @ 2023-11-29 8:14 UTC (permalink / raw)
To: git; +Cc: hanwenn
In-Reply-To: <cover.1701243201.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]
We read both the HEAD and ORIG_HEAD references directly from the
filesystem in order to figure out whether we're currently splitting a
commit. If both of the following are true:
- HEAD points to the same object as "rebase-merge/amend".
- ORIG_HEAD points to the same object as "rebase-merge/orig-head".
Then we are currently splitting commits.
The current code only works by chance because we only have a single
reference backend implementation. Refactor it to instead read both refs
via the refdb layer so that we'll also be compatible with alternate
reference backends.
Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
because we didn't resolve symrefs before either, and in practice none of
the refs in "rebase-merge/" would be symbolic. We thus don't want to
resolve symrefs with the new code either to retain the old behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
wt-status.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..fe9e590b80 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
static int split_commit_in_progress(struct wt_status *s)
{
int split_in_progress = 0;
- char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+ struct object_id head_oid, orig_head_oid;
+ char *rebase_amend, *rebase_orig_head;
if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
!s->branch || strcmp(s->branch, "HEAD"))
return 0;
- head = read_line_from_git_path("HEAD");
- orig_head = read_line_from_git_path("ORIG_HEAD");
+ if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
+ read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
+ return 0;
+
rebase_amend = read_line_from_git_path("rebase-merge/amend");
rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
- if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+ if (!rebase_amend || !rebase_orig_head)
; /* fall through, no split in progress */
else if (!strcmp(rebase_amend, rebase_orig_head))
- split_in_progress = !!strcmp(head, rebase_amend);
- else if (strcmp(orig_head, rebase_orig_head))
+ split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
+ else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
split_in_progress = 1;
- free(head);
- free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/4] refs: propagate errno when reading special refs fails
From: Patrick Steinhardt @ 2023-11-29 8:14 UTC (permalink / raw)
To: git; +Cc: hanwenn
In-Reply-To: <cover.1701243201.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]
Some refs in Git are more special than others due to reasons explained
in the next commit. These refs are read via `refs_read_special_head()`,
but this function doesn't behave the same as when we try to read a
normal ref. Most importantly, we do not propagate `failure_errno` in the
case where the reference does not exist, which is behaviour that we rely
on in many parts of Git.
Fix this bug by propagating errno when `strbuf_read_file()` fails.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 6 +++++-
t/t1403-show-ref.sh | 9 +++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index fcae5dddc6..7d4a057f36 100644
--- a/refs.c
+++ b/refs.c
@@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,
int result = -1;
strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
- if (strbuf_read_file(&content, full_path.buf, 0) < 0)
+ errno = 0;
+
+ if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
+ *failure_errno = errno;
goto done;
+ }
result = parse_loose_ref_contents(content.buf, oid, referent, type,
failure_errno);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..37af154d27 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -266,4 +266,13 @@ test_expect_success '--exists with directory fails with generic error' '
test_cmp expect err
'
+test_expect_success '--exists with non-existent special ref' '
+ test_expect_code 2 git show-ref --exists FETCH_HEAD
+'
+
+test_expect_success '--exists with existing special ref' '
+ git rev-parse HEAD >.git/FETCH_HEAD &&
+ git show-ref --exists FETCH_HEAD
+'
+
test_done
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/4] refs: complete list of special refs
From: Patrick Steinhardt @ 2023-11-29 8:14 UTC (permalink / raw)
To: git; +Cc: hanwenn
In-Reply-To: <cover.1701243201.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4174 bytes --]
We have some references that are more special than others. The reason
for them being special is that they either do not follow the usual
format of references, or that they are written to the filesystem
directly by the respective owning subsystem and thus circumvent the
reference backend.
This works perfectly fine right now because the reffiles backend will
know how to read those refs just fine. But with the prospect of gaining
a new reference backend implementation we need to be a lot more careful
here:
- We need to make sure that we are consistent about how those refs are
written. They must either always be written via the filesystem, or
they must always be written via the reference backend. Any mixture
will lead to inconsistent state.
- We need to make sure that such special refs are always handled
specially when reading them.
We're already mostly good with regard to the first item, except for
`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
But the current list of special refs is missing a lot of refs that
really should be treated specially. Right now, we only treat
`FETCH_HEAD` and `MERGE_HEAD` specially here.
Introduce a new function `is_special_ref()` that contains all current
instances of special refs to fix the reading path.
Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 56 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 7d4a057f36..2d39d3fe80 100644
--- a/refs.c
+++ b/refs.c
@@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,
return result;
}
+static int is_special_ref(const char *refname)
+{
+ /*
+ * Special references get written and read directly via the filesystem
+ * by the subsystems that create them. Thus, they must not go through
+ * the reference backend but must instead be read directly. It is
+ * arguable whether this behaviour is sensible, or whether it's simply
+ * a leaky abstraction enabled by us only having a single reference
+ * backend implementation. But at least for a subset of references it
+ * indeed does make sense to treat them specially:
+ *
+ * - FETCH_HEAD may contain multiple object IDs, and each one of them
+ * carries additional metadata like where it came from.
+ *
+ * - MERGE_HEAD may contain multiple object IDs when merging multiple
+ * heads.
+ *
+ * - "rebase-apply/" and "rebase-merge/" contain all of the state for
+ * rebases, where keeping it closely together feels sensible.
+ *
+ * There are some exceptions that you might expect to see on this list
+ * but which are handled exclusively via the reference backend:
+ *
+ * - CHERRY_PICK_HEAD
+ * - HEAD
+ * - ORIG_HEAD
+ *
+ * Writing or deleting references must consistently go either through
+ * the filesystem (special refs) or through the reference backend
+ * (normal ones).
+ */
+ const char * const special_refs[] = {
+ "AUTO_MERGE",
+ "BISECT_EXPECTED_REV",
+ "FETCH_HEAD",
+ "MERGE_AUTOSTASH",
+ "MERGE_HEAD",
+ };
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(special_refs); i++)
+ if (!strcmp(refname, special_refs[i]))
+ return 1;
+
+ /*
+ * git-rebase(1) stores its state in `rebase-apply/` or
+ * `rebase-merge/`, including various reference-like bits.
+ */
+ if (starts_with(refname, "rebase-apply/") ||
+ starts_with(refname, "rebase-merge/"))
+ return 1;
+
+ return 0;
+}
+
int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
struct object_id *oid, struct strbuf *referent,
unsigned int *type, int *failure_errno)
{
assert(failure_errno);
- if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
+ if (is_special_ref(refname))
return refs_read_special_head(ref_store, refname, oid, referent,
type, failure_errno);
- }
return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
type, failure_errno);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb
From: Patrick Steinhardt @ 2023-11-29 8:14 UTC (permalink / raw)
To: git; +Cc: hanwenn
In-Reply-To: <cover.1701243201.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5645 bytes --]
We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem
and via the refdb, which violates the newly established rules for how
special refs must be treated. This works alright in practice with the
reffiles reference backend, but will cause bugs once we gain additional
backends.
Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb
so that it is no longer a special ref.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
bisect.c | 25 ++++---------------------
builtin/bisect.c | 8 ++------
refs.c | 2 +-
t/t6030-bisect-porcelain.sh | 2 +-
4 files changed, 8 insertions(+), 29 deletions(-)
diff --git a/bisect.c b/bisect.c
index 1be8e0a271..985b96ed13 100644
--- a/bisect.c
+++ b/bisect.c
@@ -471,7 +471,6 @@ static int read_bisect_refs(void)
}
static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,
static int is_expected_rev(const struct object_id *oid)
{
- const char *filename = git_path_bisect_expected_rev();
- struct stat st;
- struct strbuf str = STRBUF_INIT;
- FILE *fp;
- int res = 0;
-
- if (stat(filename, &st) || !S_ISREG(st.st_mode))
+ struct object_id expected_oid;
+ if (read_ref("BISECT_EXPECTED_REV", &expected_oid))
return 0;
-
- fp = fopen_or_warn(filename, "r");
- if (!fp)
- return 0;
-
- if (strbuf_getline_lf(&str, fp) != EOF)
- res = !strcmp(str.buf, oid_to_hex(oid));
-
- strbuf_release(&str);
- fclose(fp);
-
- return res;
+ return oideq(oid, &expected_oid);
}
enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)
struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+ string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF);
refs_for_removal.strdup_strings = 1;
string_list_clear(&refs_for_removal, 0);
- unlink_or_warn(git_path_bisect_expected_rev());
unlink_or_warn(git_path_bisect_ancestors_ok());
unlink_or_warn(git_path_bisect_log());
unlink_or_warn(git_path_bisect_names());
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 35938b05fd..4e2c43daf5 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -17,7 +17,6 @@
#include "revision.h"
static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
const char *state;
int i, verify_expected = 1;
struct object_id oid, expected;
- struct strbuf buf = STRBUF_INIT;
struct oid_array revs = OID_ARRAY_INIT;
if (!argc)
@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
oid_array_append(&revs, &commit->object.oid);
}
- if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
- get_oid_hex(buf.buf, &expected) < 0)
+ if (read_ref("BISECT_EXPECTED_REV", &expected))
verify_expected = 0; /* Ignore invalid file contents */
- strbuf_release(&buf);
for (i = 0; i < revs.nr; i++) {
if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {
@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
}
if (verify_expected && !oideq(&revs.oid[i], &expected)) {
unlink_or_warn(git_path_bisect_ancestors_ok());
- unlink_or_warn(git_path_bisect_expected_rev());
+ delete_ref(NULL, "BISECT_EXPECTED_REV", NULL, REF_NO_DEREF);
verify_expected = 0;
}
}
diff --git a/refs.c b/refs.c
index 2d39d3fe80..0290bb0c67 100644
--- a/refs.c
+++ b/refs.c
@@ -1845,6 +1845,7 @@ static int is_special_ref(const char *refname)
* There are some exceptions that you might expect to see on this list
* but which are handled exclusively via the reference backend:
*
+ * - BISECT_EXPECTED_REV
* - CHERRY_PICK_HEAD
* - HEAD
* - ORIG_HEAD
@@ -1855,7 +1856,6 @@ static int is_special_ref(const char *refname)
*/
const char * const special_refs[] = {
"AUTO_MERGE",
- "BISECT_EXPECTED_REV",
"FETCH_HEAD",
"MERGE_AUTOSTASH",
"MERGE_HEAD",
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2a5b7d8379..792c1504bc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
git bisect bad $HASH4 &&
git bisect reset &&
test -z "$(git for-each-ref "refs/bisect/*")" &&
- test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
+ test_ref_missing BISECT_EXPECTED_REV &&
test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
test_path_is_missing ".git/BISECT_LOG" &&
test_path_is_missing ".git/BISECT_RUN" &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH] setup: recognize bare repositories with packed-refs
From: Patrick Steinhardt @ 2023-11-29 10:13 UTC (permalink / raw)
To: Jeff King; +Cc: Adam Majer, git
In-Reply-To: <20231128190446.GA10477@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 5736 bytes --]
On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:
> On Tue, Nov 28, 2023 at 03:28:45PM +0100, Adam Majer wrote:
>
> > In a garbage collected bare git repository, the refs/ subdirectory is
> > empty. In use-cases when such a repository is directly added into
> > another repository, it no longer is detected as valid. Git doesn't
> > preserve empty paths so refs/ subdirectory is not present. Simply
> > creating an empty refs/ subdirectory fixes this problem.
>
> I understand your use case, but I still have a vague feeling that this
> is bending some assumptions in a way that may create problems or
> confusion later. In particular:
>
> > Looking more carefully, there are two backends to handle various refs in
> > git -- the files backend that uses refs/ subdirectory and the
> > packed-refs backend that uses packed-refs file. If references are not
> > found in refs/ subdirectory (or directory doesn't exist), the
> > packed-refs directory will be consulted. Garbage collected repository
> > will have all its references in packed-refs file.
>
> This second paragraph doesn't seem totally accurate to me. There are not
> really two backends that Git can use. For production use, there is just
> one, the "files" backend, which happens to also use packed-refs under
> the hood (and a convenient way for the code to structure this was a
> subordinate backend). But it has never been possible to have a repo that
> just uses packed-refs.
>
> There is also the experimental reftable, of course. And there we have
> not yet loosened is_git_directory(), and it has to create an unused
> "refs/" directory (there has been some discussion about allowing it to
> be an empty file, though no patches have been merged).
As I'm currently working on the reftable backend this thought has also
crossed my mind. The reftable backend doesn't only create "refs/", but
it also creates "HEAD" with contents "ref: refs/heads/.invalid" so that
Git commands recognize the Git directory properly. Longer-term I would
really love to see us doing a better job of detecting Git repositories
so that we don't have to carry this legacy baggage around.
I can see different ways for how to do this:
- Either we iterate through all known reference backends, asking
each of them whether they recognize the directory as something
they understand.
- Or we start parsing the gitconfig of the repository so that we can
learn about which reference backend to expect, and then ask that
specific backend whether it thinks that the directory indeed looks
like something it can handle.
I'd personally prefer the latter, but I'm not sure whether we really
want to try and parse any file that happens to be called "config".
> So with regards to the loosening in your patch, my questions would be:
>
> - if we are going to change the rules for repository detection, is
> this where we want to end up? We haven't changed them (yet) for
> reftables. If we are going to do so, should we have a scheme that
> will work for that transition, too? The "refs is an empty file"
> scheme would fix your use case, too (though see below).
>
> - is the rest of Git ready to handle a missing "refs/" directory? It
> looks like making a ref will auto-create it (since we may have to
> make refs/foo/bar/... anyway).
>
> - what about other implementations? Your embedded repos will
> presumably not work with libgit2, jgit, etc, until they also get
> similar patches.
>
> - what about empty repositories? In that case there will be no "refs/"
> file and no "packed-refs" file (such a repository is less likely, of
> course, but it may contain objects but no refs, or the point may be
> to have an empty repo as a test vector). Likewise, it is possible
> for a repository to have an empty "objects" directory (even with a
> non-empty refs directory, if there are only symrefs), and your patch
> doesn't address that.
Just throwing this out there, but we could use this as an excuse to
introduce "extensions.refFormat". If it's explicitly configured to be
"reffiles" then we accept repositories even if they don't have the
"refs/" directory or a "packed-refs" file. This would still require work
in alternative implementations of Git, but this work will need to happen
anyway when the reftable backend lands.
I'd personally love for this extension to be introduced before I'm
sending the reftable backend upstream so that we can have discussions
around it beforehand.
Patrick
> > To allow the use-case when packed-refs is the only source of refs and
> > refs/ subdirectory is simply not present, augment 'is_git_directory()'
> > setup function to look for packed-refs file as an alternative to refs/
> > subdirectory.
>
> Getting back to your use case, I'd suggest one of:
>
> - do the usual "touch refs/.gitignore" trick to explicitly track the
> empty directory. It looks like the ref code will ignore this (we
> don't allow ref names to start with "." in a path component)
>
> - whatever is consuming the embedded repos could "mkdir -p refs
> objects" as needed. This is a minor pain, but I think in the long
> term we are moving to a world where you have to explicitly do
> "GIT_DIR=$PWD/embedded.git" to access an embedded bare repo. So
> they're already special and require some setup; adding an extra step
> may not be so bad.
>
> Now it may be that neither of those solutions is acceptable for various
> reasons. But it is probably worth detailing those reasons in your commit
> message.
>
> -Peff
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Draft of Git Rev News edition 105
From: Christian Couder @ 2023-11-29 18:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen, Kaartic Sivaraam,
Taylor Blau, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Bruno Brito,
Alexander Shopov, René Scharfe, Philip Oakley, Jeff King,
Jason Hatton, brian m. carlson, Eric Sunshine,
Carlo Marcelo Arenas Belón, Torsten Bögershausen
Hi everyone!
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-105.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/670
You can also reply to this email.
In general all kinds of contributions, for example proofreading,
suggestions for articles or links, help on the issues in GitHub,
volunteering for being interviewed and so on, are very much
appreciated.
I tried to Cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.
Jakub, Markus, Kaartic and I plan to publish this edition on
Friday December 1st.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH] setup: recognize bare repositories with packed-refs
From: Taylor Blau @ 2023-11-29 21:30 UTC (permalink / raw)
To: Jeff King; +Cc: Adam Majer, git
In-Reply-To: <20231128190446.GA10477@coredump.intra.peff.net>
On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:
> - whatever is consuming the embedded repos could "mkdir -p refs
> objects" as needed. This is a minor pain, but I think in the long
> term we are moving to a world where you have to explicitly do
> "GIT_DIR=$PWD/embedded.git" to access an embedded bare repo. So
> they're already special and require some setup; adding an extra step
> may not be so bad.
I hope not. I suppose that using embedded bare repositories in a test
requires additional setup at least to "cd" into the directory (if they
are not using `$GIT_DIR` or `--git-dir` already). But I fear that
imposing even a small change like this is too tall an order for how many
millions of these exist in the wild across all sorts of projects.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb
From: Taylor Blau @ 2023-11-29 21:45 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, hanwenn
In-Reply-To: <35b74eb972eed7e08190e826fabcf6b7a241f285.1701243201.git.ps@pks.im>
On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:
> We read both the HEAD and ORIG_HEAD references directly from the
> filesystem in order to figure out whether we're currently splitting a
> commit. If both of the following are true:
>
> - HEAD points to the same object as "rebase-merge/amend".
>
> - ORIG_HEAD points to the same object as "rebase-merge/orig-head".
>
> Then we are currently splitting commits.
>
> The current code only works by chance because we only have a single
> reference backend implementation. Refactor it to instead read both refs
> via the refdb layer so that we'll also be compatible with alternate
> reference backends.
>
> Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
> because we didn't resolve symrefs before either, and in practice none of
> the refs in "rebase-merge/" would be symbolic. We thus don't want to
> resolve symrefs with the new code either to retain the old behaviour.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> wt-status.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 9f45bf6949..fe9e590b80 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
> static int split_commit_in_progress(struct wt_status *s)
> {
> int split_in_progress = 0;
> - char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> + struct object_id head_oid, orig_head_oid;
> + char *rebase_amend, *rebase_orig_head;
>
> if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> !s->branch || strcmp(s->branch, "HEAD"))
> return 0;
>
> - head = read_line_from_git_path("HEAD");
> - orig_head = read_line_from_git_path("ORIG_HEAD");
> + if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> + read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
Switching to read_ref_full() here is going to have some slightly
different behavior than just reading out the contents of
"$GIT_DIR/HEAD", but I think that it should be OK.
Before we would not have complained, if, for example, the contents of
"$GIT_DIR/HEAD" were malformed, but now we will. I think that's OK,
especially given that if that file is bogus, we'll have other problems
before we get here ;-).
Are there any other gotchas that we should be thinking about?
> + return 0;
> +
> rebase_amend = read_line_from_git_path("rebase-merge/amend");
> rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> - if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> + if (!rebase_amend || !rebase_orig_head)
> ; /* fall through, no split in progress */
> else if (!strcmp(rebase_amend, rebase_orig_head))
> - split_in_progress = !!strcmp(head, rebase_amend);
> - else if (strcmp(orig_head, rebase_orig_head))
> + split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
> + else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
I did a double take at these strcmp(oid_to_hex(...)) calls, but I think
that they are the best that we can do given that we're still reading the
contents of "rebase-merge/amend" and "rebase-merge/orig-head" directly.
I suppose we could go the other way and turn their contents into
object_ids and then use oidcmp(), but it doesn't seem worth it IMHO.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 2/4] refs: propagate errno when reading special refs fails
From: Taylor Blau @ 2023-11-29 21:51 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, hanwenn
In-Reply-To: <691552a17ec587b0c03e758437c33d58767803aa.1701243201.git.ps@pks.im>
On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:
> diff --git a/refs.c b/refs.c
> index fcae5dddc6..7d4a057f36 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,
> int result = -1;
> strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
>
> - if (strbuf_read_file(&content, full_path.buf, 0) < 0)
> + errno = 0;
Do we need to set errno to 0 here? Looking at the implementation of
strbuf_read_file(), it looks like we return early in two cases. Either
open() fails, in which errno is set for us, or strbuf_read() fails, in
which case we set errno to whatever it was right after the failed read
(preventing the subsequent close() call from tainting the value of errno).
So I think in either case, we have the right value in errno, and don't
need to worry about setting it to "0" ahead of time.
> +test_expect_success '--exists with existing special ref' '
> + git rev-parse HEAD >.git/FETCH_HEAD &&
> + git show-ref --exists FETCH_HEAD
> +'
I don't think that it matters here, but do we need to worry about
cleaning up .git/FETCH_HEAD for future tests?
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/4] refs: complete list of special refs
From: Taylor Blau @ 2023-11-29 21:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, hanwenn
In-Reply-To: <0e38103114a206bedbbbd7ea97cb77fa05fd3c29.1701243201.git.ps@pks.im>
On Wed, Nov 29, 2023 at 09:14:20AM +0100, Patrick Steinhardt wrote:
> We have some references that are more special than others. The reason
> for them being special is that they either do not follow the usual
> format of references, or that they are written to the filesystem
> directly by the respective owning subsystem and thus circumvent the
> reference backend.
>
> This works perfectly fine right now because the reffiles backend will
> know how to read those refs just fine. But with the prospect of gaining
> a new reference backend implementation we need to be a lot more careful
> here:
>
> - We need to make sure that we are consistent about how those refs are
> written. They must either always be written via the filesystem, or
> they must always be written via the reference backend. Any mixture
> will lead to inconsistent state.
>
> - We need to make sure that such special refs are always handled
> specially when reading them.
>
> We're already mostly good with regard to the first item, except for
> `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
> But the current list of special refs is missing a lot of refs that
> really should be treated specially. Right now, we only treat
> `FETCH_HEAD` and `MERGE_HEAD` specially here.
>
> Introduce a new function `is_special_ref()` that contains all current
> instances of special refs to fix the reading path.
>
> Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 7d4a057f36..2d39d3fe80 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,
> return result;
> }
>
> +static int is_special_ref(const char *refname)
> +{
> + /*
> + * Special references get written and read directly via the filesystem
> + * by the subsystems that create them. Thus, they must not go through
> + * the reference backend but must instead be read directly. It is
> + * arguable whether this behaviour is sensible, or whether it's simply
> + * a leaky abstraction enabled by us only having a single reference
> + * backend implementation. But at least for a subset of references it
> + * indeed does make sense to treat them specially:
> + *
> + * - FETCH_HEAD may contain multiple object IDs, and each one of them
> + * carries additional metadata like where it came from.
> + *
> + * - MERGE_HEAD may contain multiple object IDs when merging multiple
> + * heads.
> + *
> + * - "rebase-apply/" and "rebase-merge/" contain all of the state for
> + * rebases, where keeping it closely together feels sensible.
> + *
> + * There are some exceptions that you might expect to see on this list
> + * but which are handled exclusively via the reference backend:
> + *
> + * - CHERRY_PICK_HEAD
> + * - HEAD
> + * - ORIG_HEAD
> + *
> + * Writing or deleting references must consistently go either through
> + * the filesystem (special refs) or through the reference backend
> + * (normal ones).
> + */
> + const char * const special_refs[] = {
> + "AUTO_MERGE",
> + "BISECT_EXPECTED_REV",
> + "FETCH_HEAD",
> + "MERGE_AUTOSTASH",
> + "MERGE_HEAD",
> + };
Is there a reason that we don't want to declare this statically? If we
did, I think we could drop one const, since the strings would instead
reside in the .rodata section.
> + int i;
Not that it matters for this case, but it may be worth declaring i to be
an unsigned type, since it's used as an index into an array. size_t
seems like an appropriate choice there.
> + for (i = 0; i < ARRAY_SIZE(special_refs); i++)
> + if (!strcmp(refname, special_refs[i]))
> + return 1;
> +
> + /*
> + * git-rebase(1) stores its state in `rebase-apply/` or
> + * `rebase-merge/`, including various reference-like bits.
> + */
> + if (starts_with(refname, "rebase-apply/") ||
> + starts_with(refname, "rebase-merge/"))
Do we care about case sensitivity here? Definitely not on case-sensitive
filesystems, but I'm not sure about case-insensitive ones. For instance,
on macOS, I can do:
$ git rev-parse hEAd
and get the same value as "git rev-parse HEAD" (on my Linux workstation,
this fails as expected).
I doubt that there are many users in the wild asking to resolve
reBASe-APPLY/xyz, but I think that after this patch that would no longer
work as-is, so we may want to replace this with istarts_with() instead.
Thanks,
Taylor
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox