git-fetch: Split fetch and merge logic

It makes git-parse-remote.sh and almost all git-fetch independent of the merge logic.

git-fetch fetches the branches from the remote and saves this information in .git/FETCH_FETCHED, and at the end it generates the file .git/FETCH_HEAD.

The current merge behaviour is unchanged.

Santi Béjar <[email protected]> writes:
...

Quoted 1 line

The current merge behaviour is unchanged.

I overlooked your earlier huge testcase patch, but I noticed it does not include any test that fetches an annotated tag that points at a commit or a non-commit, nor an unannotated tag that is a tree or a blob.

It makes git-parse-remote.sh and almost all git-fetch independent of the merge logic.

git-fetch fetches the branches from the remote and saves this information in .git/FETCH_FETCHED, and at the end it generates the file .git/FETCH_HEAD.

 git-fetch.sh        |   79 +++++++++++++++++++++++++++++++--------------------
 git-parse-remote.sh |   64 ++++++++++++-----------------------------
 t/t5510-fetch.sh    |   16 ++++++++++
 3 files changed, 83 insertions(+), 76 deletions(-)

git-fetch.sh +0 -0

index d230995..637d732 100755

Quoted 6 lines · Santi Béjar

It makes git-parse-remote.sh and almost all git-fetch independent of the merge logic.

git-fetch fetches the branches from the remote and saves this information in .git/FETCH_FETCHED, and at the end it generates the file .git/FETCH_HEAD.

I might have some more comments after actually applying this and reviewing it with wider contexts, but it looks nice overall.

I am wondering if FETCH_FETCHED is purely for internal use by git-fetch (it appears so), and if so if it is worth trying to do without the temporary file, but that is a minor detail.

Quoted 10 lines

git-fetch.sh +2 -0

index d230995..637d732 100755
@@ -491,3 +467,44 @@ case "$orig_head" in
      fi
      ;;
 esac
+
+# Generate $GIT_DIR/FETCH_HEAD

git-fetch.sh case "$orig_head" in · line 476

Quoted 5 lines

git-fetch.sh +5 -0

+case ",$#,$remote_nick," in
+,1,$origin,)
+     curr_branch=$(git-symbolic-ref -q HEAD | sed -e 's|^refs/heads/||')
+     merge_branches=$(git-repo-config \
+             --get-all "branch.${curr_branch}.merge" | sort -u)

Why "sort -u" (instead of erroring out if the repository is misconfigured)?

git-fetch.sh case "$orig_head" in · line 478

Quoted 3 lines

git-fetch.sh +2 -0

+     fetch_branches=$(get_remote_default_refs_for_fetch -n $remote_nick |
+             sed 's/:.*$//g;s/^+//' | sort -u)

GNU sed users do not have problems with this, but I recall that we had to rewrite our sed scripts not to use multiple commands concatenated with ';' for portability, like:

        sed -e 's/:.*$//g' -e 's/^+//'

Again why "sort -u"?

git-fetch.sh case "$orig_head" in · line 479

Quoted 2 lines

git-fetch.sh +1 -0

+     test "$merge_branches" && test "$fetch_branches" &&

We probably would want to be defensive by saying "test -n".

git-fetch.sh case "$orig_head" in · line 480

Quoted 1 line

git-fetch.sh +1 -0

+     merge_branches=$(echo -e "$merge_branches\n$fetch_branches" | sort | uniq -d)

I appreciate the cleverness of the intersection. However, is "echo -e" portable? I think we have avoided it so far (we have avoided even "echo -n" which is traditionally much more available).

Quoted 4 lines

git-fetch.sh +3 -0

+cat "$GIT_DIR"/FETCH_FETCHED | while IFS='   ' read sha1 ref note ; do
+     remote_branch=$(expr "z$ref" : 'z\([^:]*\):')
+     for merge_branch in $merge_branches ; do

git-fetch.sh case "$orig_head" in · line 510

Quoted 10 lines

git-fetch.sh +10 -0

+             [ "$merge_branch" == "$remote_branch" ] &&
+                     echo "$sha1             $note" && continue 2
+     done
+     if ! test "$merge_first" || test "$merge_first" == "done" ; then
+             echo "$sha1     not-for-merge   $note"
+     else
+             echo "$sha1             $note"
+             merge_first=done
+     fi
+done >> "$GIT_DIR/FETCH_HEAD"

You can do:

while ...
do
done < "$GIT_DIR/FETCH_FETCHED"

which is easier on the eye.

I often see a buggy shell script that expects assignment in a while loop to survive after the loop finished, when the loop is on the downstream side of a pipe (e.g. the loop is run in a subshell so merge_first after this loop is finished will never be 'done'). You do not use the variable after the loop so your script is not buggy, but avoiding a pipe into while loop is a good habit to get into.

git-parse-remote.sh canon_refs_list_for_fetch () { · line 162

Quoted 12 lines

git-parse-remote.sh +2 -2

index 5208ee6..691d46c 100755
@@ -196,32 +159,43 @@ canon_refs_list_for_fetch () {
 
      config)
-             canon_refs_list_for_fetch -d "$1" \
-                     $(git-config --get-all "remote.$1.fetch") ;;
+             set $(expand_refs_wildcard "$1" \
+                     $(git-repo-config --get-all "remote.$1.fetch")) ;;

Oops? It is not buggy but it's better to set an example by using git-config consistenty. You have another mention of repo-config above.

git-parse-remote.sh canon_refs_list_for_fetch () { · line 185

Quoted 7 lines

git-parse-remote.sh +2 -2

      remotes)
-             canon_refs_list_for_fetch -d "$1" $(sed -ne '/^Pull: */{
+             set $(expand_refs_wildcard "$1" $(sed -ne '/^Pull: */{
                                              s///p
-                                     }' "$GIT_DIR/remotes/$1")
+                                     }' "$GIT_DIR/remotes/$1"))

Hmph. I wonder why the original author did not do '/^Pull: */s///p'...

Quoted 3 lines · Junio C Hamano

I am wondering if FETCH_FETCHED is purely for internal use by git-fetch (it appears so), and if so if it is worth trying to do without the temporary file, but that is a minor detail.

Yes, it's purely internal. With "without the temporary file" you mean to put the content in a variable or removing at the end?

Quoted 4 lines

I appreciate the cleverness of the intersection. However, is "echo -e" portable? I think we have avoided it so far (we have avoided even "echo -n" which is traditionally much more available).

printf '%s\n%s' "$merge_branches" "$fetch_branches"

is OK?