Re: [PATCH] subtree: fix split processing with multiple subtrees present
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 19, 2023, 01:04 UTC
- Message-ID
- <xmqqpm2fht2x.fsf@gitster.g>
- In-Reply-To
- <pull.1587.git.1695067516192.gitgitgadget@gmail.com>
"Zach FettersMoore via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 20 quoted lines
> In the diagram below, 'M' represents the mainline repo branch, 'A' > represents one subtree, and 'B' represents another. M3 and B1 represent > a split commit for subtree B that was created from commit M4. M2 and A1 > represent a split commit made from subtree A that was also created > based on changes back to and including M4. M1 represents new changes to > the repo, in this scenario if you try to run a 'git subtree split > --rejoin' for subtree B, commits M1, M2, and A1, will be included in > the processing of changes for the new split commit since the last > split/rejoin for subtree B was at M3. The issue is that by having A1 > included in this processing the command ends up needing to processing > every commit down tree A even though none of that is needed or relevant > to the current command and result. > > M1 > | \ \ > M2 | | > | A1 | > M3 | | > | | B1 > M4 | |
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?
Show 9 quoted lines
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index e0c5d3b0de6..e9250dfb019 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -778,12 +778,29 @@ 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 splitsWay overlong line. Please split them accordingly. I won't comment on what CodingGuidelines tells us already, in this review, but have a few comments here:
> +should_ignore_subtree_commit () {
> + if [ "$(git log -1 --grep="git-subtree-dir:" $1)" ]
> + then
> + if [[ -z "$(git log -1 --grep="git-subtree-mainline:" $1)" && -z "$(git log -1 --grep="git-subtree-dir: $dir$" $1)" ]]Here $dir is a free variable that comes from outside. The caller does not supply it as a parameter to this function (and the caller does not receive it as its parameter from its caller). Yet the file as a whole seems to liberally make assignments to it ("git grep dir=" on the file counts 7 assignments). Are we sure we are looking for the right $dir in this particular grep?
Side note: I am not familiar with this part of the code at all, so do not take it as "here is a bug", but more as "this smells error prone."
Also can $dir have regular expressions special characters? "The existing code and new code alike, git-subtree is not prepared to handle directory names with RE special characters well at all, so do not use them if you do not want your history broken" is an acceptable answer.
The caller of this function process_split_commit is cmd_split and process_split_commit (hence this function) is called repeatedly inside a loop. This function makes a traversal over the entire history for each and every iteration in "good" cases where there is no 'mainline' or 'subtree-dir' commits for the given $dir.
I wonder if it is more efficient to enumerate all commits that hits these grep criteria in the cmd_split before it starts to call process_split_commit repeatedly. If it knows which commit can be ignored beforehand, it can skip and not call process_split_commit, no?
Show 12 quoted lines
> + then
> + return 0
> + fi
> + fi
> + return 1
> +}
> +
> # Usage: process_split_commit REV PARENTS
> process_split_commit () {
> assert test $# = 2
> local rev="$1"
> local parents="$2"These seem to assume that $1 and $2 can have $IFS in them, so shouldn't ...
> + if should_ignore_subtree_commit $rev
... this call too enclose $rev inside a pair of double-quotes for consistency? We know the loop in the cmd_split that calls this function is reading from "rev-list --parents" and $rev is a 40-hex commit object name (and $parents can have more than one 40-hex commit object names separated with SP), so it is safe to leave $rev unquoted, but it pays to be consistent to help make the code more readable.
Show 9 quoted lines
> + then > + return > + fi > + > if test $indent -eq 0 > then > revcount=$(($revcount + 1)) > > base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518