* [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs @ 2014-02-27 9:00 Carlos Martín Nieto 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2014-02-27 9:00 UTC (permalink / raw) To: git From: Carlos Martín Nieto <cmn@dwim.me> When a remote has multiple fetch refspecs and these overlap in the target namespace, fetch may prune a remote-tracking branch which still exists in the remote. The test uses a popular form of this, by putting pull requests as stored in a popular hosting platform alongside "real" remote-tracking branches. The fetch command makes a decision of whether to prune based on the first matching refspec, which in this case is insufficient, as it covers the pull request names. This pair of refspecs does work as expected if the more "specific" refspec is the first in the list. Signed-off-by: Carlos Martín Nieto <cmn@elego.de> --- This setup is used by GitHub for Windows, but nobody has noticed this break because it puts the PR refspec in the system config, which makes that one the first. I was alerted to this by someone who had done this setup manually and thus added the PR refspec after the default one. t/t5510-fetch.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 1f0f8e6..4949e3d 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' git rev-parse origin/master ' +test_expect_failure 'fetch --prune handles overlapping refspecs' ' + cd "$D" && + git update-ref refs/pull/42/head master && + git clone . prune-overlapping && + cd prune-overlapping && + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && + + git fetch --prune origin && + git rev-parse origin/master && + git rev-parse origin/pr/42 && + + git config --unset-all remote.origin.fetch + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && + + git fetch --prune origin && + git rev-parse origin/master && + git rev-parse origin/pr/42 +' + test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' ' cd "$D" && git clone . prune-tags && -- 1.9.0.rc3.244.g3497008 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 9:00 [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Carlos Martín Nieto @ 2014-02-27 9:00 ` Carlos Martín Nieto 2014-02-27 10:21 ` Michael Haggerty ` (2 more replies) 2014-02-27 20:18 ` [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Eric Sunshine 2014-02-27 20:19 ` Eric Sunshine 2 siblings, 3 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2014-02-27 9:00 UTC (permalink / raw) To: git From: Carlos Martín Nieto <cmn@dwim.me> We need to consider that a remote-tracking branch may match more than one rhs of a fetch refspec. In such a case, it is not enough to stop at the first match but look at all of the matches in order to determine whether a head is stale. To this goal, introduce a variant of query_refspecs which returns all of the matching refspecs and loop over those answers to check for staleness. Signed-off-by: Carlos Martín Nieto <cmn@elego.de> --- There is an unfortunate duplication of code here, as query_refspecs_multiple is mostly query_refspecs but we only care about the other side of matching refspecs and disregard the 'force' information which query_refspecs does want. I thought about putting both together via callbacks and having query_refspecs stop at the first one, but I'm not sure that it would make it easier to read or manage. remote.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- t/t5510-fetch.sh | 2 +- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/remote.c b/remote.c index 9f1a8aa..26140c7 100644 --- a/remote.c +++ b/remote.c @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name, return ret; } +static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results) +{ + int i; + int find_src = !query->src; + + if (find_src && !query->dst) + error("query_refspecs_multiple: need either src or dst"); + + for (i = 0; i < ref_count; i++) { + struct refspec *refspec = &refs[i]; + const char *key = find_src ? refspec->dst : refspec->src; + const char *value = find_src ? refspec->src : refspec->dst; + const char *needle = find_src ? query->dst : query->src; + char **result = find_src ? &query->src : &query->dst; + + if (!refspec->dst) + continue; + if (refspec->pattern) { + if (match_name_with_pattern(key, needle, value, result)) { + string_list_append_nodup(results, *result); + } + } else if (!strcmp(needle, key)) { + string_list_append(results, value); + } + } +} + static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query) { int i; @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname, const unsigned char *sha1, int flags, void *cb_data) { struct stale_heads_info *info = cb_data; + struct string_list matches = STRING_LIST_INIT_DUP; struct refspec query; + int i, stale = 1; memset(&query, 0, sizeof(struct refspec)); query.dst = (char *)refname; - if (query_refspecs(info->refs, info->ref_count, &query)) + query_refspecs_multiple(info->refs, info->ref_count, &query, &matches); + if (matches.nr == 0) return 0; /* No matches */ /* * If we did find a suitable refspec and it's not a symref and * it's not in the list of refs that currently exist in that - * remote we consider it to be stale. + * remote we consider it to be stale. In order to deal with + * overlapping refspecs, we need to go over all of the + * matching refs. */ - if (!((flags & REF_ISSYMREF) || - string_list_has_string(info->ref_names, query.src))) { + if (flags & REF_ISSYMREF) + return 0; + + for (i = 0; i < matches.nr; i++) { + if (string_list_has_string(info->ref_names, matches.items[i].string)) { + stale = 0; + break; + } + } + + string_list_clear(&matches, 0); + + if (stale) { struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail); hashcpy(ref->new_sha1, sha1); } - free(query.src); return 0; } diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4949e3d..a86f6bc 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -113,7 +113,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' git rev-parse origin/master ' -test_expect_failure 'fetch --prune handles overlapping refspecs' ' +test_expect_success 'fetch --prune handles overlapping refspecs' ' cd "$D" && git update-ref refs/pull/42/head master && git clone . prune-overlapping && -- 1.9.0.rc3.244.g3497008 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto @ 2014-02-27 10:21 ` Michael Haggerty 2014-02-27 19:29 ` Carlos Martín Nieto 2014-02-27 20:19 ` Junio C Hamano 2014-03-24 19:24 ` Junio C Hamano 2 siblings, 1 reply; 11+ messages in thread From: Michael Haggerty @ 2014-02-27 10:21 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote: > From: Carlos Martín Nieto <cmn@dwim.me> > > We need to consider that a remote-tracking branch may match more than > one rhs of a fetch refspec. In such a case, it is not enough to stop at > the first match but look at all of the matches in order to determine > whether a head is stale. > > To this goal, introduce a variant of query_refspecs which returns all of > the matching refspecs and loop over those answers to check for > staleness. > > Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > --- > > There is an unfortunate duplication of code here, as > query_refspecs_multiple is mostly query_refspecs but we only care > about the other side of matching refspecs and disregard the 'force' > information which query_refspecs does want. > > I thought about putting both together via callbacks and having > query_refspecs stop at the first one, but I'm not sure that it would > make it easier to read or manage. > > remote.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- > t/t5510-fetch.sh | 2 +- > 2 files changed, 48 insertions(+), 6 deletions(-) > > diff --git a/remote.c b/remote.c > index 9f1a8aa..26140c7 100644 > --- a/remote.c > +++ b/remote.c > @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name, > return ret; > } > > +static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results) > +{ > + int i; > + int find_src = !query->src; > + > + if (find_src && !query->dst) > + error("query_refspecs_multiple: need either src or dst"); > + > + for (i = 0; i < ref_count; i++) { > + struct refspec *refspec = &refs[i]; > + const char *key = find_src ? refspec->dst : refspec->src; > + const char *value = find_src ? refspec->src : refspec->dst; > + const char *needle = find_src ? query->dst : query->src; > + char **result = find_src ? &query->src : &query->dst; > + > + if (!refspec->dst) > + continue; > + if (refspec->pattern) { > + if (match_name_with_pattern(key, needle, value, result)) { > + string_list_append_nodup(results, *result); > + } > + } else if (!strcmp(needle, key)) { > + string_list_append(results, value); > + } > + } > +} > + > static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query) > { > int i; > @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname, > const unsigned char *sha1, int flags, void *cb_data) > { > struct stale_heads_info *info = cb_data; > + struct string_list matches = STRING_LIST_INIT_DUP; > struct refspec query; > + int i, stale = 1; > memset(&query, 0, sizeof(struct refspec)); > query.dst = (char *)refname; > > - if (query_refspecs(info->refs, info->ref_count, &query)) > + query_refspecs_multiple(info->refs, info->ref_count, &query, &matches); > + if (matches.nr == 0) > return 0; /* No matches */ > > /* > * If we did find a suitable refspec and it's not a symref and > * it's not in the list of refs that currently exist in that > - * remote we consider it to be stale. > + * remote we consider it to be stale. In order to deal with > + * overlapping refspecs, we need to go over all of the > + * matching refs. > */ > - if (!((flags & REF_ISSYMREF) || > - string_list_has_string(info->ref_names, query.src))) { > + if (flags & REF_ISSYMREF) > + return 0; > + > + for (i = 0; i < matches.nr; i++) { > + if (string_list_has_string(info->ref_names, matches.items[i].string)) { > + stale = 0; > + break; > + } > + } > + > + string_list_clear(&matches, 0); > + > + if (stale) { > struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail); > hashcpy(ref->new_sha1, sha1); > } > > - free(query.src); > return 0; > } I didn't have time to review this fully, but I think you are missing calls to string_list_clear(&matches) on a couple of code paths. Michael -- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 10:21 ` Michael Haggerty @ 2014-02-27 19:29 ` Carlos Martín Nieto 0 siblings, 0 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2014-02-27 19:29 UTC (permalink / raw) To: Michael Haggerty; +Cc: git On Thu, 2014-02-27 at 11:21 +0100, Michael Haggerty wrote: > On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote: > > From: Carlos Martín Nieto <cmn@dwim.me> > > > > We need to consider that a remote-tracking branch may match more than > > one rhs of a fetch refspec. In such a case, it is not enough to stop at > > the first match but look at all of the matches in order to determine > > whether a head is stale. > > > > To this goal, introduce a variant of query_refspecs which returns all of > > the matching refspecs and loop over those answers to check for > > staleness. > > > > Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > > --- > > > > There is an unfortunate duplication of code here, as > > query_refspecs_multiple is mostly query_refspecs but we only care > > about the other side of matching refspecs and disregard the 'force' > > information which query_refspecs does want. > > > > I thought about putting both together via callbacks and having > > query_refspecs stop at the first one, but I'm not sure that it would > > make it easier to read or manage. > > > > remote.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- > > t/t5510-fetch.sh | 2 +- > > 2 files changed, 48 insertions(+), 6 deletions(-) > > > > diff --git a/remote.c b/remote.c > > index 9f1a8aa..26140c7 100644 > > --- a/remote.c > > +++ b/remote.c > > @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name, > > return ret; > > } > > > > +static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results) > > +{ > > + int i; > > + int find_src = !query->src; > > + > > + if (find_src && !query->dst) > > + error("query_refspecs_multiple: need either src or dst"); > > + > > + for (i = 0; i < ref_count; i++) { > > + struct refspec *refspec = &refs[i]; > > + const char *key = find_src ? refspec->dst : refspec->src; > > + const char *value = find_src ? refspec->src : refspec->dst; > > + const char *needle = find_src ? query->dst : query->src; > > + char **result = find_src ? &query->src : &query->dst; > > + > > + if (!refspec->dst) > > + continue; > > + if (refspec->pattern) { > > + if (match_name_with_pattern(key, needle, value, result)) { > > + string_list_append_nodup(results, *result); > > + } > > + } else if (!strcmp(needle, key)) { > > + string_list_append(results, value); > > + } > > + } > > +} > > + > > static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query) > > { > > int i; > > @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname, > > const unsigned char *sha1, int flags, void *cb_data) > > { > > struct stale_heads_info *info = cb_data; > > + struct string_list matches = STRING_LIST_INIT_DUP; > > struct refspec query; > > + int i, stale = 1; > > memset(&query, 0, sizeof(struct refspec)); > > query.dst = (char *)refname; > > > > - if (query_refspecs(info->refs, info->ref_count, &query)) > > + query_refspecs_multiple(info->refs, info->ref_count, &query, &matches); > > + if (matches.nr == 0) > > return 0; /* No matches */ > > > > /* > > * If we did find a suitable refspec and it's not a symref and > > * it's not in the list of refs that currently exist in that > > - * remote we consider it to be stale. > > + * remote we consider it to be stale. In order to deal with > > + * overlapping refspecs, we need to go over all of the > > + * matching refs. > > */ > > - if (!((flags & REF_ISSYMREF) || > > - string_list_has_string(info->ref_names, query.src))) { > > + if (flags & REF_ISSYMREF) > > + return 0; > > + > > + for (i = 0; i < matches.nr; i++) { > > + if (string_list_has_string(info->ref_names, matches.items[i].string)) { > > + stale = 0; > > + break; > > + } > > + } > > + > > + string_list_clear(&matches, 0); > > + > > + if (stale) { > > struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail); > > hashcpy(ref->new_sha1, sha1); > > } > > > > - free(query.src); > > return 0; > > } > > I didn't have time to review this fully, but I think you are missing > calls to string_list_clear(&matches) on a couple of code paths. Yep, you're right. I'll fix this and hold off new version for a bit to see if there's more input. cmn ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto 2014-02-27 10:21 ` Michael Haggerty @ 2014-02-27 20:19 ` Junio C Hamano 2014-02-27 20:41 ` Junio C Hamano 2014-02-28 12:21 ` Carlos Martín Nieto 2014-03-24 19:24 ` Junio C Hamano 2 siblings, 2 replies; 11+ messages in thread From: Junio C Hamano @ 2014-02-27 20:19 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git Carlos Martín Nieto <cmn@elego.de> writes: > From: Carlos Martín Nieto <cmn@dwim.me> > > We need to consider that a remote-tracking branch may match more than > one rhs of a fetch refspec. Hmph, do we *need* to, really? Do you mean fetching one ref on the remote side and storing that in multiple remote-tracking refs on our side? What benefit does such an arrangement give the user? When we "git fetch $there $that_ref" to obtain that single ref, do we update both remote-tracking refs? When the user asks "git log $that_ref@{upstream}", which one of two or more remote-tracking refs should we consult? Should we report an error if these remote-tracking refs that are supposed to track the same remote ref not all match? Does "git push $there $that_ref" to update that remote ref update all of these remote-tracking refs on our side? Should it? My knee-jerk reaction is that it may not be worth supporting such an arrangement as broken (we may even want to diagnose it as an error), but assuming we do need to, the approach to solve it, i.e. this... > In such a case, it is not enough to stop at > the first match but look at all of the matches in order to determine > whether a head is stale. ... sounds sensible. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 20:19 ` Junio C Hamano @ 2014-02-27 20:41 ` Junio C Hamano 2014-02-28 12:21 ` Carlos Martín Nieto 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2014-02-27 20:41 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Carlos Martín Nieto <cmn@elego.de> writes: > >> From: Carlos Martín Nieto <cmn@dwim.me> >> >> We need to consider that a remote-tracking branch may match more than >> one rhs of a fetch refspec. > > Hmph, do we *need* to, really? > > Do you mean fetching one ref on the remote side and storing that in > multiple remote-tracking refs on our side? What benefit does such > an arrangement give the user? When we "git fetch $there $that_ref" > to obtain that single ref, do we update both remote-tracking refs? > When the user asks "git log $that_ref@{upstream}", which one of two > or more remote-tracking refs should we consult? Should we report > an error if these remote-tracking refs that are supposed to track > the same remote ref not all match? Does "git push $there $that_ref" > to update that remote ref update all of these remote-tracking refs > on our side? Should it? > > My knee-jerk reaction is that it may not be worth supporting such an > arrangement as broken (we may even want to diagnose it as an error), > but assuming we do need to, the approach to solve it, i.e. this... > >> In such a case, it is not enough to stop at >> the first match but look at all of the matches in order to determine >> whether a head is stale. > > ... sounds sensible. Having said that, if we need to support such a configuration, I would not be surprised if there are many other corner case bugs coming from the same root cause---query_refspecs() does not allow us to see more than one destination. It would be prudent to squash them before we officially say we do support such a configuration. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 20:19 ` Junio C Hamano 2014-02-27 20:41 ` Junio C Hamano @ 2014-02-28 12:21 ` Carlos Martín Nieto 2014-02-28 18:04 ` Junio C Hamano 1 sibling, 1 reply; 11+ messages in thread From: Carlos Martín Nieto @ 2014-02-28 12:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 2014-02-27 at 12:19 -0800, Junio C Hamano wrote: > Carlos Martín Nieto <cmn@elego.de> writes: > > > From: Carlos Martín Nieto <cmn@dwim.me> > > > > We need to consider that a remote-tracking branch may match more than > > one rhs of a fetch refspec. > > Hmph, do we *need* to, really? > > Do you mean fetching one ref on the remote side and storing that in > multiple remote-tracking refs on our side? What benefit does such > an arrangement give the user? When we "git fetch $there $that_ref" No, I mean a different kind of overlap, where the right-hand side matches more refs than appear on the left side. In this particular case, we would have something like refs/heads/*:refs/remotes/origin/* refs/pull/*/head:refs/remotes/origin/pr/* as fetch refspecs. Going remote -> remote-tracking branch is not an issue, as each remote head only matches one refspec. However, we now have 'origin/master' and 'origin/pr/5' both of which match the 'refs/remotes/origin/*' pattern. The current behaviour is to stop at the first match, which would mark it as stale as there is no 'refs/heads/pr/5' branch in the remote. In lieu of "real" namespacing support for remotes, this seems like a reasonable way of coalescing the namespaces in the remote repo. I'll update the commit message with more exact explanation of what kind of overlap we're dealing with, as it seems it could do with help. Is there maybe a better word to describe this setup than "overlapping"? > to obtain that single ref, do we update both remote-tracking refs? > When the user asks "git log $that_ref@{upstream}", which one of two > or more remote-tracking refs should we consult? Should we report > an error if these remote-tracking refs that are supposed to track > the same remote ref not all match? Does "git push $there $that_ref" > to update that remote ref update all of these remote-tracking refs > on our side? Should it? > > My knee-jerk reaction is that it may not be worth supporting such an > arrangement as broken (we may even want to diagnose it as an error), > but assuming we do need to, the approach to solve it, i.e. this... > For this (other) situation, where you duplicate refs, the issue we're dealing with in these patches wouldn't arise. I have argued similarly against built-in support in libgit2 for this kind of shenanigans, but apparently there's people who use it, though their motivations remain a mystery to me. Luckily we can support *that* quite well by just going through the refspecs one by one and applying the rules (both in git and libgit2). cmn > > In such a case, it is not enough to stop at > > the first match but look at all of the matches in order to determine > > whether a head is stale. > > ... sounds sensible. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-28 12:21 ` Carlos Martín Nieto @ 2014-02-28 18:04 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2014-02-28 18:04 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git Carlos Martín Nieto <cmn@elego.de> writes: > ... However, we now > have 'origin/master' and 'origin/pr/5' both of which match the > 'refs/remotes/origin/*' pattern. The current behaviour is to stop at the > first match, which would mark it as stale as there is no > 'refs/heads/pr/5' branch in the remote. OK, but with a later pattern, we can find out that it came from pull/5 that was advertised by the remote. If we had origin/pr/1 when the remote no longer has pull/1, then we can say that is stale. Makes sense. Thanks for an explanation. I wonder how well --prune would work on a repository in pre 1.5 layout, where all branches were copied to local refs/heads/ hierarchy except for 'master' (which is renamed to 'origin'). Does it have a similar issue? Do we end up pruning refs/heads/origin away because we do not see it on the remote end, or we somehow already deal with it and not have to worry about it? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto 2014-02-27 10:21 ` Michael Haggerty 2014-02-27 20:19 ` Junio C Hamano @ 2014-03-24 19:24 ` Junio C Hamano 2 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2014-03-24 19:24 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git Carlos Martín Nieto <cmn@elego.de> writes: > From: Carlos Martín Nieto <cmn@dwim.me> > > We need to consider that a remote-tracking branch may match more than > one rhs of a fetch refspec. In such a case, it is not enough to stop at > the first match but look at all of the matches in order to determine > whether a head is stale. > > To this goal, introduce a variant of query_refspecs which returns all of > the matching refspecs and loop over those answers to check for > staleness. > > Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > --- > > There is an unfortunate duplication of code here, as > query_refspecs_multiple is mostly query_refspecs but we only care > about the other side of matching refspecs and disregard the 'force' > information which query_refspecs does want. > > I thought about putting both together via callbacks and having > query_refspecs stop at the first one, but I'm not sure that it would > make it easier to read or manage. Sorry for a belated review. I agree with your analysis of the root-cause of the symptom exposed by new tests in [1/2] and the proposed solution. > @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname, > const unsigned char *sha1, int flags, void *cb_data) > { > struct stale_heads_info *info = cb_data; > + struct string_list matches = STRING_LIST_INIT_DUP; > struct refspec query; > + int i, stale = 1; > memset(&query, 0, sizeof(struct refspec)); > query.dst = (char *)refname; > > - if (query_refspecs(info->refs, info->ref_count, &query)) > + query_refspecs_multiple(info->refs, info->ref_count, &query, &matches); > + if (matches.nr == 0) > return 0; /* No matches */ > > /* > * If we did find a suitable refspec and it's not a symref and > * it's not in the list of refs that currently exist in that > - * remote we consider it to be stale. > + * remote we consider it to be stale. In order to deal with > + * overlapping refspecs, we need to go over all of the > + * matching refs. > */ > - if (!((flags & REF_ISSYMREF) || > - string_list_has_string(info->ref_names, query.src))) { > + if (flags & REF_ISSYMREF) > + return 0; Who frees "matches"? At this point matches.nr != 0 so there must be something we need to free, no? > + for (i = 0; i < matches.nr; i++) { > + if (string_list_has_string(info->ref_names, matches.items[i].string)) { > + stale = 0; > + break; > + } > + } > + > + string_list_clear(&matches, 0); > + > + if (stale) { > struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail); > hashcpy(ref->new_sha1, sha1); > } > > - free(query.src); In the new code, query_refspecs_multiple() uses the result allocated by match_name_with_pattern() to the results list, taking it out of query.src without copying, so losing this free() is the right thing to do---"matches" must be cleared. And "string_list matches" is initialized as INIT_DUP, so we can rely on string_list_clear() to free these strings. > return 0; > } Regarding the seemingly duplicated logic in the new function, I wonder if the callers of non-duplicated variant may benefit if they notice there are multiple hits, even if they cannot use more than one in their context. That is, what would happen if we changed these callers to instead of calling query-refspecs call the "multi" variant, and if that call finds multiple matches, do something about it (e.g. warn if they use "the first hit" because they are not acting on later hits, possibly losing information)? Here is a minor clean-ups, both to fix style and plug leaks, that can be squashed to this patch. How does it look? Thanks. remote.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/remote.c b/remote.c index 26140c7..fde7b52 100644 --- a/remote.c +++ b/remote.c @@ -839,9 +839,8 @@ static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct if (!refspec->dst) continue; if (refspec->pattern) { - if (match_name_with_pattern(key, needle, value, result)) { + if (match_name_with_pattern(key, needle, value, result)) string_list_append_nodup(results, *result); - } } else if (!strcmp(needle, key)) { string_list_append(results, value); } @@ -1989,32 +1988,29 @@ static int get_stale_heads_cb(const char *refname, query_refspecs_multiple(info->refs, info->ref_count, &query, &matches); if (matches.nr == 0) - return 0; /* No matches */ + goto clean_exit; /* No matches */ /* * If we did find a suitable refspec and it's not a symref and * it's not in the list of refs that currently exist in that - * remote we consider it to be stale. In order to deal with + * remote, we consider it to be stale. In order to deal with * overlapping refspecs, we need to go over all of the * matching refs. */ if (flags & REF_ISSYMREF) - return 0; + goto clean_exit; - for (i = 0; i < matches.nr; i++) { - if (string_list_has_string(info->ref_names, matches.items[i].string)) { + for (i = 0; stale && i < matches.nr; i++) + if (string_list_has_string(info->ref_names, matches.items[i].string)) stale = 0; - break; - } - } - - string_list_clear(&matches, 0); if (stale) { struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail); hashcpy(ref->new_sha1, sha1); } +clean_exit: + string_list_clear(&matches, 0); return 0; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs 2014-02-27 9:00 [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Carlos Martín Nieto 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto @ 2014-02-27 20:18 ` Eric Sunshine 2014-02-27 20:19 ` Eric Sunshine 2 siblings, 0 replies; 11+ messages in thread From: Eric Sunshine @ 2014-02-27 20:18 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: Git List On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto <cmn@elego.de> wrote: > Subject: fetch: add a failing test for prunning with overlapping refspecs s/prunning/pruning/ > Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > --- > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 1f0f8e6..4949e3d 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' > git rev-parse origin/master > ' > > +test_expect_failure 'fetch --prune handles overlapping refspecs' ' > + cd "$D" && > + git update-ref refs/pull/42/head master && > + git clone . prune-overlapping && > + cd prune-overlapping && > + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > + > + git fetch --prune origin && > + git rev-parse origin/master && > + git rev-parse origin/pr/42 && > + > + git config --unset-all remote.origin.fetch Broken &&-chain. > + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && > + > + git fetch --prune origin && > + git rev-parse origin/master && > + git rev-parse origin/pr/42 > +' > + > test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' ' > cd "$D" && > git clone . prune-tags && > -- > 1.9.0.rc3.244.g3497008 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs 2014-02-27 9:00 [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Carlos Martín Nieto 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto 2014-02-27 20:18 ` [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Eric Sunshine @ 2014-02-27 20:19 ` Eric Sunshine 2 siblings, 0 replies; 11+ messages in thread From: Eric Sunshine @ 2014-02-27 20:19 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: Git List On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto <cmn@elego.de> wrote: > Subject: fetch: add a failing test for prunning with overlapping refspecs s/prunning/pruning/ > Signed-off-by: Carlos Martín Nieto <cmn@elego.de> > --- > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 1f0f8e6..4949e3d 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' > git rev-parse origin/master > ' > > +test_expect_failure 'fetch --prune handles overlapping refspecs' ' > + cd "$D" && > + git update-ref refs/pull/42/head master && > + git clone . prune-overlapping && > + cd prune-overlapping && > + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > + > + git fetch --prune origin && > + git rev-parse origin/master && > + git rev-parse origin/pr/42 && > + > + git config --unset-all remote.origin.fetch Broken &&-chain. > + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && > + > + git fetch --prune origin && > + git rev-parse origin/master && > + git rev-parse origin/pr/42 > +' > + > test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' ' > cd "$D" && > git clone . prune-tags && > -- > 1.9.0.rc3.244.g3497008 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-03-24 19:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-27 9:00 [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Carlos Martín Nieto 2014-02-27 9:00 ` [PATCH 2/2] fetch: handle overlaping refspecs on --prune Carlos Martín Nieto 2014-02-27 10:21 ` Michael Haggerty 2014-02-27 19:29 ` Carlos Martín Nieto 2014-02-27 20:19 ` Junio C Hamano 2014-02-27 20:41 ` Junio C Hamano 2014-02-28 12:21 ` Carlos Martín Nieto 2014-02-28 18:04 ` Junio C Hamano 2014-03-24 19:24 ` Junio C Hamano 2014-02-27 20:18 ` [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs Eric Sunshine 2014-02-27 20:19 ` Eric Sunshine
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.