Re: [RFC/PATCH 3/3] push: add 'prune' option

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> 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