Re: [PATCH v2 2/7] subtree: exclude commits predating add from recursive processing
From: Johannes Schindelin <hidden>
Date: 2020-10-07 15:36:39
Hi Tom, On Tue, 6 Oct 2020, Tom Clarkson via GitGitGadget wrote:
From: Tom Clarkson <redacted> Include recursion depth in debug logs so we can see when the recursion is getting out of hand. Making the cache handle null mappings correctly and adding older commits to the cache allows the recursive algorithm to terminate at any point on mainline rather than needing to reach either the add point or the initial commit.
Makes sense.
quoted hunk ↗ jump to hunk
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 9867718503..160bad95c1 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh@@ -244,7 +244,7 @@ check_parents () { do if ! test -r "$cachedir/notree/$miss" then - debug " incorrect order: $miss" + debug " unprocessed parent commit: $miss ($indent)"
Without any context, it is hard to understand what the `$indent` variable is supposed to mean, so it is unclear why we need to print it here. I _guess_ it is the degree removed from the first-parent lineage? In any case, it does not hurt here, so I trust that it is good to include it in the debug output.
quoted hunk ↗ jump to hunk
process_split_commit "$miss" "" "$indent" fi done@@ -392,6 +392,24 @@ find_existing_splits () { done } +find_mainline_ref () { + debug "Looking for first split..." + dir="$1" + revs="$2"
The `git-subtree` script seems to rely on the `local` construct, using it in plenty of other circumstances. How about using it here, too?
+ + git log --reverse --grep="^git-subtree-dir: $dir/*\$" \ + --no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |
Since all you are interested in is the `git-subtree-mainline:` trailer, wouldn't a format like `%(trailers:key=git-subtree-mainline)` instead of `START %H%n%s%n%n%b%nEND%n`? See https://git-scm.com/docs/git-log#Documentation/git-log.txt-emtrailersoptionsem for more information about pretty formats. BTW I am super unfamiliar with `git subtree`'s inner workings, and therefore it would help me incredibly if the commit message talked a bit about the commit message layout (with a particular eye on `git-subtree-dir` and `git-subtree-mainline` which I guess are trailers added by `git subtree`?)...
quoted hunk ↗ jump to hunk
+ while read a b junk + do + case "$a" in + git-subtree-mainline:) + echo "$b" + return + ;; + esac + done +} + copy_commit () { # We're going to set some environment vars here, so # do it in a subshell to get rid of them safely later@@ -646,9 +664,9 @@ process_split_commit () { progress "$revcount/$revmax ($createcount) [$extracount]" - debug "Processing commit: $rev" + debug "Processing commit: $rev ($indent)" exists=$(cache_get "$rev") - if test -n "$exists" + if test -z "$(cache_miss "$rev")" then debug " prior: $exists"
I do not see the `exists` variable being used other than for the debug statement. Maybe better something like this? debug " prior found for $rev"
quoted hunk ↗ jump to hunk
return@@ -773,6 +791,17 @@ cmd_split () { unrevs="$(find_existing_splits "$dir" "$revs")" + mainline="$(find_mainline_ref "$dir" "$revs")" + if test -n "$mainline" + then + debug "Mainline $mainline predates subtree add" + git rev-list --topo-order --skip=1 $mainline | + while read rev + do + cache_set "$rev" ""
Ah, so they are not really "null mappings", but mapped to an empty string. Makes sense. Maybe adjust the commit message?
+ done || exit $? + fi + # We can't restrict rev-list to only $dir here, because some of our # parents have the $dir contents the root, and those won't match. # (and rev-list --follow doesn't seem to solve this) -- gitgitgadget