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.
> It's useful to conveniently synchronize all the local branches to
> certain remote.
When an update to this patch comes with a documentation update to
illustrate how to exercise this useful feature with an example, it will
start to make sense to write "this is useful" in the log message. I know
you haven't gotten around the documentation part while the patch is marked
as RFC, and that is OK.
> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
> ---
> builtin/push.c | 2 ++
> remote.c | 29 ++++++++++++++++++++++++++---
> remote.h | 3 ++-
> transport.c | 2 ++
> transport.h | 1 +
> 5 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 35cce53..46b99b1 100644
> --- 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'.
> 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....
> {
> 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?
> + 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?
> @@ -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.
> + 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?
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...?
> #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
--
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