fetch: Auto-following tags should check connectivity, not existence

fetch: Auto-following tags should check connectivity, not existence

[Johannes Sixt]

git-fetch's auto-following of tags fetches all tags for which it finds
objects in the local repository. I feel it were better if not object
existence, but connectivity to the existing refs was checked, like this:

diff --git a/git-fetch.sh b/git-fetch.sh
index fd70696..1b3c459 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -314,11 +314,12 @@ case "$no_tags$tags" in
        taglist=$(IFS=' ' &&
        echo "$ls_remote_result" |
        git-show-ref --exclude-existing=refs/tags/ |
        while read sha1 name
        do
-           git-cat-file -t "$sha1" >/dev/null 2>&1 || continue
+           t=$(git-rev-list --max-count=1 "$sha1" --not --all 2>
/dev/null) &&
+           test -z "$t" || continue
            echo >&2 "Auto-following $name"
            echo ".${name}:${name}"
        done)
    esac
    case "$taglist" in

Is this considered in the shell-to-C rewrite that's currently going on?

The background is that I sometimes fetch a branch with tags, but then
decide to remove it (and the tags as well). git-gc and git-prune do not
guarantee that the objects go away because they could be reachable from
reflogs. Now the next git-fetch will fetch the tags again even though I
do not have any refs from which they would be reachable.

-- Hannes

Re: fetch: Auto-following tags should check connectivity, not existence

[Junio C Hamano]

Johannes Sixt <J.Sixt@eudaptics.com> writes:

> git-fetch's auto-following of tags fetches all tags for which it finds
> objects in the local repository. I feel it were better if not object
> existence, but connectivity to the existing refs was checked, like this:
>
> diff --git a/git-fetch.sh b/git-fetch.sh
> index fd70696..1b3c459 100755
> --- a/git-fetch.sh
> +++ b/git-fetch.sh
> @@ -314,11 +314,12 @@ case "$no_tags$tags" in
>         taglist=$(IFS=' ' &&
>         echo "$ls_remote_result" |
>         git-show-ref --exclude-existing=refs/tags/ |
>         while read sha1 name
>         do
> -           git-cat-file -t "$sha1" >/dev/null 2>&1 || continue
> +           t=$(git-rev-list --max-count=1 "$sha1" --not --all 2>
> /dev/null) &&
> +           test -z "$t" || continue
>             echo >&2 "Auto-following $name"
>             echo ".${name}:${name}"
>         done)
>     esac
>     case "$taglist" in

I think "something like this" is a sensible thing to do, on top
of the updates to "rev-list --objects" to verify the blob we
list actually exists.

However, I think --max-count=1 defeats what you are trying to
do.  Revision limiting will only look at commits, and if you
have all commits that lead to the "$sha1" commit from some of
the existing refs, but lack some blobs or trees that belong to
some of the commits that are not the first commit that will be
listed, their absense will not be noticed.

Re: fetch: Auto-following tags should check connectivity, not existence

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Johannes Sixt <J.Sixt@eudaptics.com> writes:
>
>> git-fetch's auto-following of tags fetches all tags for which it finds
>> objects in the local repository. I feel it were better if not object
>> existence, but connectivity to the existing refs was checked, like this:
> ...
> However, I think --max-count=1 defeats what you are trying to
> do.  Revision limiting will only look at commits, and if you
> have all commits that lead to the "$sha1" commit from some of
> the existing refs, but lack some blobs or trees that belong to
> some of the commits that are not the first commit that will be
> listed, their absense will not be noticed.

Sorry, but I have to take this back, after looking at your patch
once again.  In this case, all you are interested in is to see
if the commit ancestry is connected, and you do not mind if the
chain is somewhat incomplete in blobs and trees, as you will
re-fetch the chain in a safe manner in the later round.  In
other words, the code does not have to be as strict as my
quickfetch series, which tries to *omit* re-fetching
altogether.

So --max-count=1 is fine, although it may not be much of an
optimization in practice, it does not harm correctness in any
way, as my previous message suggested.