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

From: Junio C Hamano <junkio@cox.net>
Date: Feb 23, 2007, 09:53 UTC
Message-ID: <7vabz56vyq.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <87wt29i7hg.fsf@gmail.com>

Santi Béjar <sbejar@gmail.com> writes:

Show 6 quoted lines

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

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

Loses more lines than it adds (+16 lines to test does not count), which is a very good sign.

Show 15 quoted lines

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

Again why "sort -u"?

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

Show 13 quoted lines

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

Show 5 quoted lines

> 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 () {

Show 5 quoted lines

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

Show 6 quoted lines

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