Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Michael Haggerty <mhagger@alum.mit.edu>]

On 10/24/2013 10:51 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Previously, fetch's "--tags" option was considered equivalent to
>> specifying the refspec "refs/tags/*:refs/tags/*" on the command line;
>> in particular, it caused the remote.<name>.refspec configuration to be
>> ignored.
>>
>> But it is not very useful to fetch tags without also fetching other
>> references, whereas it *is* quite useful to be able to fetch tags *in
>> addition to* other references.
> 
> True but when fetching other references, tags relevant to the
> history being fetched by default should automatically follow, so the
> above explains why "fetch --tags" is not a useful thing to do daily.

Maybe not necessary in many scenarios, but is it harmful for the common
case of cloning from and then periodically fetching from an upstream?
ISTM that the result of the declarative --tags option

    we have all upstream tags

is easier to reason about than the history-dependent tag-following behavior

    we have all upstream tags that point to commits that were
    reachable from an upstream branch at the time of this or one
    of the previous fetches

I just noticed that this is not explained very clearly in the fetch man
page.  I will try to improve it.

>> If a user wants to fetch *only* tags, then it is still possible to
>> specifying an explicit refspec:
>>
>>     git fetch <remote> 'refs/tags/*:refs/tags/*'
>>
>> Please note that the documentation prior to 1.8.0.3 was ambiguous
>> about this aspect of "fetch --tags" behavior.  Commit
>>
>>     f0cb2f137c 2012-12-14 fetch --tags: clarify documentation
>>
>> made the documentation match the old behavior.  This commit changes
>> the documentation to match the new behavior.
> 
> The "old behaviour as designed".  The documentation change was not
> about refusing to fix a bug but the above makes it sound as if it
> were a such commit.

I didn't mean to imply this.  My point in mentioning the documentation
change was that even though the old behavior has been in effect for a
while, it was not clearly documented until quite recently.  But I guess
it is a moot point.

>> The change to builtin/fetch.c:get_ref_map() has the side effect of
>> changing the order that the (struct ref) objects are listed in
>> ref_map.  It seems to me that this could probably only matter in the
>> case of duplicates.  But later ref_remove_duplicates() is called, so
>> duplicates are eliminated.  However, ref_remove_duplicates() always
>> retains the first instance of duplicates and discards the rest.  It is
>> conceivable that the boolean flags (which are not inspected by
>> ref_remove_duplicates()) could differ among the duplicates, and that
>> therefore changing the order of the items in the original list has the
>> effect of changing the flags on the items that are retained.
>>
>> I don't know this code well enough to judge whether this might be a
>> problem.
> 
>> If it is, then the correct approach is probably either to teach
>> ref_remove_duplicates() to ensure that the flags are also consistent
>> across duplicates, or to somehow combine the flags from all duplicates
>> into the one that is retained.  Please let me know if this is needed.
> 
> I do not think either of these two is sufficient if you want to fix
> it; ref_remove_duplicates() needs to be taught to retain the first
> one it encounters among the duplicates, not "ensure the flags are
> consistent" by erroring out when they are not among duplicates, nor
> "somehow combine" flags from later one to the first one.
> 
> But I suspect that, if this behaviour change were a problem, such a
> configuration was a problem before this change to most people; the
> order of duplicated [remote "foo"] section would not be under
> control of those who only use "git remote" without using an editor
> to tweak .git/config file anyway. So I do not think this regression
> is too big a deal; it is something you can fix later on top.

I'll address your second point first: I agree that the order among
explicit refspecs couldn't really be a problem because it is already
arbitrary.  However, get_ref_map() adds references from other sources to
the result list.  For example, if there are command-line arguments, it adds

1. command-line arguments (with fetch_head_status=FETCH_HEAD_MERGE)

2. configured refspecs (with fetch_head_status=FETCH_HEAD_IGNORE)

3. if (--tags)
   then tags_refspec (with fetch_head_status=FETCH_HEAD_NOT_FOR_MERGE)
   else the result of find_non_local_tags() (with
            fetch_head_status=FETCH_HEAD_NOT_FOR_MERGE)

It could very well be that the order matters across different classes of
reference.

Regarding your first point: if it matters which of the duplicates is
chosen, then it can only matter because they differ in some other way
than their reference names, for example in their flags.  So a robust way
of de-duping them (if it is possible) would be to compare the two
records and decide which one should take precedence based on this other
information rather than based on which one happened to come earlier in
the list.  Then the list order would be immaterial and (for example) it
wouldn't be a problem to reorder the items.

I'm being very wishy-washy because I haven't yet invested the time to
learn how these lists of refs are used.  Maybe I need to do so.

I have run out of time, so I will address your comments about the
documentation changes in a separate email.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/