Santi Béjar <sbejar@xxxxxxxxx> 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.
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.
> 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).
> +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'...
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html