Re: [PATCH] subtree: fix split processing with multiple subtrees present

>> 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?

>> +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?

>> +    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.

>
> "Zach FettersMoore via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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?
>
> > 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 splits
>
> Way 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?
>
> > +    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.
>
> > +    then
> > +         return
> > +    fi
> > +
> >       if test $indent -eq 0
> >       then
> >               revcount=$(($revcount + 1))
> >
> > base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518