Re: [PATCHv3] git-fetch: Split fetch and merge logic

> Santi Béjar <sbejar@gmail.com> writes:
>
> > 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.

> > diff --git a/git-fetch.sh b/git-fetch.sh
> > index d230995..637d732 100755
> > --- a/git-fetch.sh
> > +++ b/git-fetch.sh
> > @@ -491,3 +467,44 @@ case "$orig_head" in
> >       fi
> >       ;;
> >  esac
> > +
> > +# Generate $GIT_DIR/FETCH_HEAD
> > +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)?

>
> > +     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/^+//'

>
> > +     test "$merge_branches" && test "$fetch_branches" &&
>
> We probably would want to be defensive by saying "test -n".
>

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

>
> > +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
> > +             [ "$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.

>
> > diff --git a/git-parse-remote.sh b/git-parse-remote.sh
> > index 5208ee6..691d46c 100755
> > --- a/git-parse-remote.sh
> > +++ b/git-parse-remote.sh
> > @@ -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.

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