From: Felipe Contreras Date: Wed, 22 Feb 2012 20:43:05 GMT Subject: Re: [RFC/PATCH 3/3] push: add 'prune' option Message-ID: In-Reply-To: <7vobsxqebz.fsf@alter.siamese.dyndns.org> On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano wrote: > Felipe Contreras writes: > >> This will allow us to remove refs from the remote that have been removed >> locally. > > Can you enhance this a bit more to summarize the gist of what the semantic > of this new feature is, perhaps like this: > >        After pushing refs, "git push --prune" will remove refs from the >        remote that existed before the push and would have been pushed >        from us if we had some local refs that would have matched the >        refspecs used.  For example, > >           $ git push --prune remote refs/heads/*:refs/remotes/repo1/* > >        will push all local branches in our repository to refs with >        corresponding names under refs/remotes/repo1/ at the remote, and >        removes remote's refs in refs/remotes/repo1/ that no longer have >        corresponding local branches in our repository.  The refs at the >        remote outside refs/remotes/repo1/ are not affected. > > In order to alley the worries raised in the previous discussion, something > to the effect of the last sentence above is crucial to have, I would think. OK. >> --- a/builtin/push.c >> +++ b/builtin/push.c >> @@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix) >>               OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status", >>                       TRANSPORT_PUSH_SET_UPSTREAM), >>               OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"), >> +             OPT_BIT('p', "prune", &flags, "prune locally removed refs", >> +                     TRANSPORT_PUSH_PRUNE), > > Please refrain from squatting on a short-and-sweet one letter option > before this new feature proves to be popular and useful in a few cycles, > especially when there already is a long option that begins with 'p'. OK. >>               OPT_END() >>       }; >> >> diff --git a/remote.c b/remote.c >> index 019aafc..0900bb5 100644 >> --- a/remote.c >> +++ b/remote.c >> @@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst, >>  } >> >>  static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref, >> -             int send_mirror, const struct refspec **ret_pat) >> +             int send_mirror, int dir, const struct refspec **ret_pat) > > Can we name this a bit better?  I first thought "Huh? directory?", and had > to scratch my head, wondering if it is an offset into refs/heads/* string > or something.... OK. >>  { >>       const struct refspec *pat; >>       char *name; >> @@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref >> >>               if (rs[i].pattern) { >>                       const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src; >> -                     if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) { >> +                     int match; >> +                     if (dir == 0) >> +                             match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name); >> +                     else >> +                             match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name); > > ....until the code told us that it is some sort of direction of the > matching.  A symbolic constant or two would be even better. > > Originally this funcion was fed a list of refs in the source (i.e. on our > end, as this is only used in 'push') and matched them against the source > side of the refspec, rs[i].src, to see under what name the destination > side will store it (i.e. give dst_side as value to find out the result in > &name).  This patch adds a new caller, who feeds a list of refs in the > destination (i.e. on the remote end) to find out how they map to the names > on our end (i.e. source).  So "direction" is not necessarily incorrect; it > is the direction this function maps the names (either src-to-dst for the > original caller, or dst-to-src for the new caller). > > Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something? I think only FROM_SRC, FROM_DST is more than enough to figure it out. >> +                     if (match) { >>                               matching_refs = i; >>                               break; >>                       } > > So what is the updated semantics of this function?  Is it still > appropriate to name it "check_pattern_match()"? > > It seems that by now this does a lot more than just "check if a pattern > matches".  Since your patch 2/3, it is a function that finds out the > refname in the remote that the given one refspec would try to update, and > with this patch, it can also map in the reverse direction, given the list > of remote refs, finding out which local ref a refspec would use to update > them. > > At the same time, to reduce risk of future breakage, we probably should > rename this function to make it clear that this function is to be only > used by the push side. > > Perhaps rename this to "map_push_refs()" or something in the patch 2/3? I think get_ref_match() would be more appropriate because we are acting on a specific (singular) ref, and the primary thing we care about is getting the peer name, based on the refspec match, which we might want as a return value. >> @@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst, >>       struct refspec *rs; >>       int send_all = flags & MATCH_REFS_ALL; >>       int send_mirror = flags & MATCH_REFS_MIRROR; >> +     int send_prune = flags & MATCH_REFS_PRUNE; >>       int errs; >>       static const char *default_refspec[] = { ":", NULL }; >>       struct ref *ref, **dst_tail = tail_ref(dst); >> @@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst, >>               if (ref->peer_ref) >>                       continue; >> >> -             dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat); >> +             dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat); >>               if (!dst_name) >>                       continue; >> >> @@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst, >>       free_name: >>               free(dst_name); >>       } >> +     if (send_prune) { >> +             /* check for missing refs on the remote */ >> +             for (ref = *dst; ref; ref = ref->next) { >> +                     char *src_name; >> + >> +                     if (ref->peer_ref) >> +                             /* We're already sending something to this ref. */ >> +                             continue; >> + >> +                     src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL); >> +                     if (src_name) { >> +                             if (!find_ref_by_name(src, src_name)) >> +                                     ref->peer_ref = try_explicit_object_name(""); > > Yuck.  You do not want it to "try" as its name says.  You just want to > trigger its "delete" codepath. > > Please extract the body of "if (!*name) { ... }" block out of that > function into a separate helper function, i.e. > >        static struct ref *deleted_ref(void) >        { >                struct ref *ref = alloc_ref("(delete)"); >                hashclr(ref->new_sha1); >                return ref; >        } > > then update try_explicit_...() to call it, and call the same helper here. > > This is not for runtime efficiency; feeding a constant to a function that > says try_foo() or check_bar() that makes decision on the parameter only to > trigger a partial codepath hurts readability. All right. >> +                             free(src_name); >> +                     } >> +             } >> +     } >>       if (errs) >>               return -1; >>       return 0; >> diff --git a/remote.h b/remote.h >> index b395598..341142c 100644 >> --- a/remote.h >> +++ b/remote.h >> @@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *); >>  enum match_refs_flags { >>       MATCH_REFS_NONE         = 0, >>       MATCH_REFS_ALL          = (1 << 0), >> -     MATCH_REFS_MIRROR       = (1 << 1) >> +     MATCH_REFS_MIRROR       = (1 << 1), >> +     MATCH_REFS_PRUNE        = (1 << 2), >>  }; >> >>  /* Reporting of tracking info */ >> diff --git a/transport.c b/transport.c >> index cac0c06..c20267c 100644 >> --- a/transport.c >> +++ b/transport.c >> @@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport, >>                       match_flags |= MATCH_REFS_ALL; >>               if (flags & TRANSPORT_PUSH_MIRROR) >>                       match_flags |= MATCH_REFS_MIRROR; >> +             if (flags & TRANSPORT_PUSH_PRUNE) >> +                     match_flags |= MATCH_REFS_PRUNE; > > Does it make sense to specify --prune when --mirror is in effect?  If so, > how would it behave differently from a vanilla --mirror?  If not, should > it be detected as an error? Probably doesn't make sense, should be an error. > I couldn't infer from the context shown in the patch, but how in general > does this new feature interact with the codepath for --mirror? > >>               if (match_push_refs(local_refs, &remote_refs, >>                                   refspec_nr, refspec, match_flags)) { >> diff --git a/transport.h b/transport.h >> index 059b330..5d30328 100644 >> --- a/transport.h >> +++ b/transport.h >> @@ -101,6 +101,7 @@ struct transport { >>  #define TRANSPORT_PUSH_MIRROR 8 >>  #define TRANSPORT_PUSH_PORCELAIN 16 >>  #define TRANSPORT_PUSH_SET_UPSTREAM 32 >> +#define TRANSPORT_PUSH_PRUNE 64 >>  #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64 > > Hrm...? Probably some rebase mistake =/ Cheers. -- Felipe Contreras