On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html