{"thread":{"id":"29656","subject":"[RFC/PATCH 1/3] remote: use a local variable in match_push_refs()","startedAt":"2012-02-17T19:12:34Z","lastAt":"2012-02-22T23:54:37Z","messageCount":17,"participants":["Felipe Contreras","Jeff King","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"184897","messageId":"1329505957-24595-1-git-send-email-felipe.contreras@gmail.com","threadId":"29656","inReplyTo":null,"subject":"[RFC/PATCH 0/3] push: add 'prune' option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-17T19:12:34Z","receivedAt":"2012-02-17T19:12:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nAs mentioned in a previous thread[1], git is lacking some functionality to\nsynchronize completely with remote repositories.\n\nAs an example I put my use-case; I want to backup *all* my local branches to a\npersonal repository, and I want to remove branches that I have removed from my\nlocal repository. git push personal 'refs/heads/*' mostly does the job, but it\ndoesn't remove anything, and that's where 'prune' comes from.\n\nDo not confuse the remote branches with the upstream ones; you could have two\nrepositories where you want to synchronize branches to:\n% git push --prune --force backup1 'refs/heads/*'\n% git push --prune --force backup2 'refs/heads/*'\n\nI still think a 'git remote sync' would be tremendously useful, but it can\nactually use this --prune option.\n\nI know, documentation and testing are missing, but perhaps there will be\ncomments on the code itself.\n\nCheers.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/184990\n\nFelipe Contreras (3):\n  remote: use a local variable in match_push_refs()\n  remote: reorganize check_pattern_match()\n  push: add 'prune' option\n\n builtin/push.c |    2 +\n remote.c       |   91 +++++++++++++++++++++++++++++++++++--------------------\n remote.h       |    3 +-\n transport.c    |    2 +\n transport.h    |    1 +\n 5 files changed, 65 insertions(+), 34 deletions(-)\n\n-- \n1.7.9.1\n"},{"id":"184896","messageId":"1329505957-24595-2-git-send-email-felipe.contreras@gmail.com","threadId":"29656","inReplyTo":"1329505957-24595-1-git-send-email-felipe.contreras@gmail.com","subject":"[RFC/PATCH 1/3] remote: use a local variable in match_push_refs()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-17T19:12:35Z","receivedAt":"2012-02-17T19:12:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"So that we can reuse src later on.\n\nWill be useful in next patches.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n remote.c |   19 ++++++++++---------\n 1 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 73a3809..55d68d1 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1157,7 +1157,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \tint send_mirror = flags & MATCH_REFS_MIRROR;\n \tint errs;\n \tstatic const char *default_refspec[] = { \":\", NULL };\n-\tstruct ref **dst_tail = tail_ref(dst);\n+\tstruct ref *ref, **dst_tail = tail_ref(dst);\n \n \tif (!nr_refspec) {\n \t\tnr_refspec = 1;\n@@ -1167,14 +1167,14 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \terrs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);\n \n \t/* pick the remainder */\n-\tfor ( ; src; src = src->next) {\n+\tfor (ref = src; ref; ref = ref->next) {\n \t\tstruct ref *dst_peer;\n \t\tconst struct refspec *pat = NULL;\n \t\tchar *dst_name;\n-\t\tif (src->peer_ref)\n+\t\tif (ref->peer_ref)\n \t\t\tcontinue;\n \n-\t\tpat = check_pattern_match(rs, nr_refspec, src);\n+\t\tpat = check_pattern_match(rs, nr_refspec, ref);\n \t\tif (!pat)\n \t\t\tcontinue;\n \n@@ -1184,13 +1184,14 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \t\t\t * including refs outside refs/heads/ hierarchy, but\n \t\t\t * that does not make much sense these days.\n \t\t\t */\n-\t\t\tif (!send_mirror && prefixcmp(src->name, \"refs/heads/\"))\n+\t\t\tif (!send_mirror && prefixcmp(ref->name, \"refs/heads/\"))\n \t\t\t\tcontinue;\n-\t\t\tdst_name = xstrdup(src->name);\n+\t\t\tdst_name = xstrdup(ref->name);\n+\n \n \t\t} else {\n \t\t\tconst char *dst_side = pat->dst ? pat->dst : pat->src;\n-\t\t\tif (!match_name_with_pattern(pat->src, src->name,\n+\t\t\tif (!match_name_with_pattern(pat->src, ref->name,\n \t\t\t\t\t\t     dst_side, &dst_name))\n \t\t\t\tdie(\"Didn't think it matches any more\");\n \t\t}\n@@ -1211,9 +1212,9 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \n \t\t\t/* Create a new one and link it */\n \t\t\tdst_peer = make_linked_ref(dst_name, &dst_tail);\n-\t\t\thashcpy(dst_peer->new_sha1, src->new_sha1);\n+\t\t\thashcpy(dst_peer->new_sha1, ref->new_sha1);\n \t\t}\n-\t\tdst_peer->peer_ref = copy_ref(src);\n+\t\tdst_peer->peer_ref = copy_ref(ref);\n \t\tdst_peer->force = pat->force;\n \tfree_name:\n \t\tfree(dst_name);\n-- \n1.7.9.1\n"},{"id":"184898","messageId":"1329505957-24595-3-git-send-email-felipe.contreras@gmail.com","threadId":"29656","inReplyTo":"1329505957-24595-1-git-send-email-felipe.contreras@gmail.com","subject":"[RFC/PATCH 2/3] remote: reorganize check_pattern_match()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-17T19:12:36Z","receivedAt":"2012-02-17T19:12:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's a lot of code that can be consolidated there, and will be useful\nfor next patches.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n remote.c |   59 ++++++++++++++++++++++++++++++-----------------------------\n 1 files changed, 30 insertions(+), 29 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 55d68d1..019aafc 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n \treturn errs;\n }\n \n-static const struct refspec *check_pattern_match(const struct refspec *rs,\n-\t\t\t\t\t\t int rs_nr,\n-\t\t\t\t\t\t const struct ref *src)\n+static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n+\t\tint send_mirror, const struct refspec **ret_pat)\n {\n+\tconst struct refspec *pat;\n+\tchar *name;\n \tint i;\n \tint matching_refs = -1;\n \tfor (i = 0; i < rs_nr; i++) {\n@@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,\n-\t\t\t\t\t\t\t     NULL, NULL))\n-\t\t\treturn rs + i;\n+\t\tif (rs[i].pattern) {\n+\t\t\tconst char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;\n+\t\t\tif (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {\n+\t\t\t\tmatching_refs = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n-\tif (matching_refs != -1)\n-\t\treturn rs + matching_refs;\n-\telse\n+\tif (matching_refs == -1)\n \t\treturn NULL;\n+\n+\tpat = rs + matching_refs;\n+\tif (pat->matching) {\n+\t\t/*\n+\t\t * \"matching refs\"; traditionally we pushed everything\n+\t\t * including refs outside refs/heads/ hierarchy, but\n+\t\t * that does not make much sense these days.\n+\t\t */\n+\t\tif (!send_mirror && prefixcmp(ref->name, \"refs/heads/\"))\n+\t\t\treturn NULL;\n+\t\tname = xstrdup(ref->name);\n+\t}\n+\tif (ret_pat)\n+\t\t*ret_pat = pat;\n+\treturn name;\n }\n \n static struct ref **tail_ref(struct ref **head)\n@@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \t\tstruct ref *dst_peer;\n \t\tconst struct refspec *pat = NULL;\n \t\tchar *dst_name;\n+\n \t\tif (ref->peer_ref)\n \t\t\tcontinue;\n \n-\t\tpat = check_pattern_match(rs, nr_refspec, ref);\n-\t\tif (!pat)\n+\t\tdst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);\n+\t\tif (!dst_name)\n \t\t\tcontinue;\n \n-\t\tif (pat->matching) {\n-\t\t\t/*\n-\t\t\t * \"matching refs\"; traditionally we pushed everything\n-\t\t\t * including refs outside refs/heads/ hierarchy, but\n-\t\t\t * that does not make much sense these days.\n-\t\t\t */\n-\t\t\tif (!send_mirror && prefixcmp(ref->name, \"refs/heads/\"))\n-\t\t\t\tcontinue;\n-\t\t\tdst_name = xstrdup(ref->name);\n-\n-\n-\t\t} else {\n-\t\t\tconst char *dst_side = pat->dst ? pat->dst : pat->src;\n-\t\t\tif (!match_name_with_pattern(pat->src, ref->name,\n-\t\t\t\t\t\t     dst_side, &dst_name))\n-\t\t\t\tdie(\"Didn't think it matches any more\");\n-\t\t}\n \t\tdst_peer = find_ref_by_name(*dst, dst_name);\n \t\tif (dst_peer) {\n \t\t\tif (dst_peer->peer_ref)\n \t\t\t\t/* We're already sending something to this ref. */\n \t\t\t\tgoto free_name;\n-\n \t\t} else {\n \t\t\tif (pat->matching && !(send_all || send_mirror))\n \t\t\t\t/*\n-- \n1.7.9.1\n"},{"id":"184899","messageId":"1329505957-24595-4-git-send-email-felipe.contreras@gmail.com","threadId":"29656","inReplyTo":"1329505957-24595-1-git-send-email-felipe.contreras@gmail.com","subject":"[RFC/PATCH 3/3] push: add 'prune' option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-17T19:12:37Z","receivedAt":"2012-02-17T19:12:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This will allow us to remove refs from the remote that have been removed\nlocally.\n\nIt's useful to conveniently synchronize all the local branches to\ncertain remote.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/push.c |    2 ++\n remote.c       |   29 ++++++++++++++++++++++++++---\n remote.h       |    3 ++-\n transport.c    |    2 ++\n transport.h    |    1 +\n 5 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 35cce53..46b99b1 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('u', \"set-upstream\", &flags, \"set upstream for git pull/status\",\n \t\t\tTRANSPORT_PUSH_SET_UPSTREAM),\n \t\tOPT_BOOLEAN(0, \"progress\", &progress, \"force progress reporting\"),\n+\t\tOPT_BIT('p', \"prune\", &flags, \"prune locally removed refs\",\n+\t\t\tTRANSPORT_PUSH_PRUNE),\n \t\tOPT_END()\n \t};\n \ndiff --git a/remote.c b/remote.c\nindex 019aafc..0900bb5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n }\n \n static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n-\t\tint send_mirror, const struct refspec **ret_pat)\n+\t\tint send_mirror, int dir, const struct refspec **ret_pat)\n {\n \tconst struct refspec *pat;\n \tchar *name;\n@@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref\n \n \t\tif (rs[i].pattern) {\n \t\t\tconst char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;\n-\t\t\tif (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {\n+\t\t\tint match;\n+\t\t\tif (dir == 0)\n+\t\t\t\tmatch = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);\n+\t\t\telse\n+\t\t\t\tmatch = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);\n+\t\t\tif (match) {\n \t\t\t\tmatching_refs = i;\n \t\t\t\tbreak;\n \t\t\t}\n@@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \tstruct refspec *rs;\n \tint send_all = flags & MATCH_REFS_ALL;\n \tint send_mirror = flags & MATCH_REFS_MIRROR;\n+\tint send_prune = flags & MATCH_REFS_PRUNE;\n \tint errs;\n \tstatic const char *default_refspec[] = { \":\", NULL };\n \tstruct ref *ref, **dst_tail = tail_ref(dst);\n@@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \t\tif (ref->peer_ref)\n \t\t\tcontinue;\n \n-\t\tdst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);\n+\t\tdst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);\n \t\tif (!dst_name)\n \t\t\tcontinue;\n \n@@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \tfree_name:\n \t\tfree(dst_name);\n \t}\n+\tif (send_prune) {\n+\t\t/* check for missing refs on the remote */\n+\t\tfor (ref = *dst; ref; ref = ref->next) {\n+\t\t\tchar *src_name;\n+\n+\t\t\tif (ref->peer_ref)\n+\t\t\t\t/* We're already sending something to this ref. */\n+\t\t\t\tcontinue;\n+\n+\t\t\tsrc_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);\n+\t\t\tif (src_name) {\n+\t\t\t\tif (!find_ref_by_name(src, src_name))\n+\t\t\t\t\tref->peer_ref = try_explicit_object_name(\"\");\n+\t\t\t\tfree(src_name);\n+\t\t\t}\n+\t\t}\n+\t}\n \tif (errs)\n \t\treturn -1;\n \treturn 0;\ndiff --git a/remote.h b/remote.h\nindex b395598..341142c 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);\n enum match_refs_flags {\n \tMATCH_REFS_NONE\t\t= 0,\n \tMATCH_REFS_ALL \t\t= (1 << 0),\n-\tMATCH_REFS_MIRROR\t= (1 << 1)\n+\tMATCH_REFS_MIRROR\t= (1 << 1),\n+\tMATCH_REFS_PRUNE\t= (1 << 2),\n };\n \n /* Reporting of tracking info */\ndiff --git a/transport.c b/transport.c\nindex cac0c06..c20267c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,\n \t\t\tmatch_flags |= MATCH_REFS_ALL;\n \t\tif (flags & TRANSPORT_PUSH_MIRROR)\n \t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n+\t\tif (flags & TRANSPORT_PUSH_PRUNE)\n+\t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n \n \t\tif (match_push_refs(local_refs, &remote_refs,\n \t\t\t\t    refspec_nr, refspec, match_flags)) {\ndiff --git a/transport.h b/transport.h\nindex 059b330..5d30328 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -101,6 +101,7 @@ struct transport {\n #define TRANSPORT_PUSH_MIRROR 8\n #define TRANSPORT_PUSH_PORCELAIN 16\n #define TRANSPORT_PUSH_SET_UPSTREAM 32\n+#define TRANSPORT_PUSH_PRUNE 64\n #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n-- \n1.7.9.1\n"},{"id":"184906","messageId":"20120217222507.GA31830@sigill.intra.peff.net","threadId":"29656","inReplyTo":"1329505957-24595-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 3/3] push: add 'prune' option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-17T22:25:07Z","receivedAt":"2012-02-17T22:25:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2012 at 09:12:37PM +0200, Felipe Contreras wrote:\n\n> This will allow us to remove refs from the remote that have been removed\n> locally.\n> \n> It's useful to conveniently synchronize all the local branches to\n> certain remote.\n\nThanks for picking up this topic again.\n\nThere's one subtlety in the motivation above that you may find helpful\nwhen you end up writing the documentation: \"push --mirror\" already does\nthis sort of pruning, but it _also_ implies that we are pushing\n\"refs/*:refs/*\".  So this is really about giving access to the pruning\nhalf, but still being able to use custom refspecs. So the features\ntogether might end up being explained something like:\n\n  --prune::\n    ... prune things that no longer exist locally ...\n\n  --mirror::\n    ... turn on --prune, and also match all refs ...\n\nAt least that is my understanding of how the code is meant to work.\n\n>  builtin/push.c |    2 ++\n>  remote.c       |   29 ++++++++++++++++++++++++++---\n>  remote.h       |    3 ++-\n>  transport.c    |    2 ++\n>  transport.h    |    1 +\n>  5 files changed, 33 insertions(+), 4 deletions(-)\n\nI've just given a quick read to the patches so far, but I did notice\nthis:\n\n>  static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n> -\t\tint send_mirror, const struct refspec **ret_pat)\n> +\t\tint send_mirror, int dir, const struct refspec **ret_pat)\n\nThe \"dir\" flag looks like it is meant to be short for \"direction\". But\nthe callers only pass 0 or 1.  I'm not clear which direction is which.\nEither symbolic constants for directions, or perhaps giving it a more\nboolean name like \"match_to_dst\" might make it more clear.\n\nI'll try to take a closer look later tonight.\n\n-Peff\n"},{"id":"184909","messageId":"7v39a9rt0u.fsf@alter.siamese.dyndns.org","threadId":"29656","inReplyTo":"1329505957-24595-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 1/3] remote: use a local variable in match_push_refs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-17T22:31:45Z","receivedAt":"2012-02-17T22:31:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> So that we can reuse src later on.\n>\n> Will be useful in next patches.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n\nIf the 2/3 were much less complex, it might have made sense to squash this\ninto that, but I think it makes it easier to see what is happening if an\nobvious and safe no-op change like this is done as a separate patch like\nthis series did.\n\nLooking good so far.\n"},{"id":"184910","messageId":"7vvcn5qecl.fsf@alter.siamese.dyndns.org","threadId":"29656","inReplyTo":"1329505957-24595-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-17T22:34:02Z","receivedAt":"2012-02-17T22:34:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> There's a lot of code that can be consolidated there, and will be useful\n> for next patches.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  remote.c |   59 ++++++++++++++++++++++++++++++-----------------------------\n>  1 files changed, 30 insertions(+), 29 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 55d68d1..019aafc 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n>  \treturn errs;\n>  }\n>  \n> -static const struct refspec *check_pattern_match(const struct refspec *rs,\n> -\t\t\t\t\t\t int rs_nr,\n> -\t\t\t\t\t\t const struct ref *src)\n> +static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n> +\t\tint send_mirror, const struct refspec **ret_pat)\n>  {\n\nFor a change that not just adds parameters but removes an existing one, \nthis is way under-described with neither in-code comment nor log message.\n\nSo I'll have to think aloud in this review.  Take it as a chance to learn\nhow the thought process of other people with lessor intelligence than you\nhave might work, and how to help those slower than you are ;-)\n\n> +\tconst struct refspec *pat;\n> +\tchar *name;\n>  \tint i;\n>  \tint matching_refs = -1;\n>  \tfor (i = 0; i < rs_nr; i++) {\n> @@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> -\t\tif (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,\n> -\t\t\t\t\t\t\t     NULL, NULL))\n> -\t\t\treturn rs + i;\n> +\t\tif (rs[i].pattern) {\n> +\t\t\tconst char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;\n> +\t\t\tif (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {\n> +\t\t\t\tmatching_refs = i;\n> +\t\t\t\tbreak;\n\nWe used to discard what match_name_with_pattern() finds out by matching a\nwildcard refspec against the ref by passing two NULLs.  This updates the\ncode to capture what destination ref ref->name is mapped to, by using the\nsame logic as the original and only caller, i.e. 'foo' without destination\nmaps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.\n\nThis function is not used by fetching side of the codepath, so we do not\nhave to worry about its need to use different dst_side selection logic\n(i.e. 'foo' without destination maps to \"do not store anywhere other than\nFETCH_HEAD\").  Good.\n\n> +\t\t\t}\n> +\t\t}\n>  \t}\n> -...\n> +\tif (matching_refs == -1)\n>  \t\treturn NULL;\n> +\n> +\tpat = rs + matching_refs;\n> +\tif (pat->matching) {\n> +\t\t/*\n> +\t\t * \"matching refs\"; traditionally we pushed everything\n> +\t\t * including refs outside refs/heads/ hierarchy, but\n> +\t\t * that does not make much sense these days.\n> +\t\t */\n> +\t\tif (!send_mirror && prefixcmp(ref->name, \"refs/heads/\"))\n> +\t\t\treturn NULL;\n> +\t\tname = xstrdup(ref->name);\n> +\t}\n\nSo you are moving some code from what the sole caller of this function\ndoes after calling us, and that is where the new parameters come from.\nAnd by doing so, you do not have to run the same match_name_with_pattern()\nagain.  OK.\n\n> +\tif (ret_pat)\n> +\t\t*ret_pat = pat;\n> +\treturn name;\n>  }\n\nYou did not initialize name to anything at the beginning, but if the\nearlier match_name_with_pattern() didn't find anything, we could only come\nhere after matching_refs is set by the if (rs[i].matching) codepath; by\nthe time we come here, we would have xstrdup(ref->name) in name, so we\nwould never return a garbage pointer to the caller.  OK.\n\n>  static struct ref **tail_ref(struct ref **head)\n> @@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>  \t\tstruct ref *dst_peer;\n>  \t\tconst struct refspec *pat = NULL;\n>  \t\tchar *dst_name;\n> +\n>  \t\tif (ref->peer_ref)\n>  \t\t\tcontinue;\n>  \n> -\t\tpat = check_pattern_match(rs, nr_refspec, ref);\n> -\t\tif (!pat)\n> +\t\tdst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);\n> +\t\tif (!dst_name)\n>  \t\t\tcontinue;\n>  \n> -\t\tif (pat->matching) {\n> -\t\t\t/*\n> -\t\t\t * \"matching refs\"; traditionally we pushed everything\n> -\t\t\t * including refs outside refs/heads/ hierarchy, but\n> -\t\t\t * that does not make much sense these days.\n> -\t\t\t */\n> -\t\t\tif (!send_mirror && prefixcmp(ref->name, \"refs/heads/\"))\n> -\t\t\t\tcontinue;\n> -\t\t\tdst_name = xstrdup(ref->name);\n> -\n> -\n> -\t\t} else {\n> -\t\t\tconst char *dst_side = pat->dst ? pat->dst : pat->src;\n> -\t\t\tif (!match_name_with_pattern(pat->src, ref->name,\n> -\t\t\t\t\t\t     dst_side, &dst_name))\n> -\t\t\t\tdie(\"Didn't think it matches any more\");\n> -\t\t}\n>  \t\tdst_peer = find_ref_by_name(*dst, dst_name);\n>  \t\tif (dst_peer) {\n>  \t\t\tif (dst_peer->peer_ref)\n>  \t\t\t\t/* We're already sending something to this ref. */\n>  \t\t\t\tgoto free_name;\n> -\n>  \t\t} else {\n>  \t\t\tif (pat->matching && !(send_all || send_mirror))\n>  \t\t\t\t/*\n\nOK, it is easy to tell that the patch is trivially correct, once a reader\nfigures out that the patch is really about:\n\n\tMove code to check_pattern_match() from its sole caller to make it\n\tunnecessary to call match_name_with_pattern() twice.\n\nSaying so in the log message would have prepared the reader, instead of\nthe \"There's a lot of code that can be consolidated there.\" which does not\ngive hints on what to look for in the patch.\n\nAlso this changes the semantics (because it changed the meaning of its\nreturn value) of check_pattern_match() so much that it would deserve a\nrename, which I will address in my review of 3/3.\n\nOtherwise this step looks good.\n"},{"id":"184911","messageId":"7vobsxqebz.fsf@alter.siamese.dyndns.org","threadId":"29656","inReplyTo":"1329505957-24595-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 3/3] push: add 'prune' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-17T22:34:24Z","receivedAt":"2012-02-17T22:34:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> This will allow us to remove refs from the remote that have been removed\n> locally.\n\nCan you enhance this a bit more to summarize the gist of what the semantic\nof this new feature is, perhaps like this:\n\n\tAfter pushing refs, \"git push --prune\" will remove refs from the\n\tremote that existed before the push and would have been pushed\n\tfrom us if we had some local refs that would have matched the\n\trefspecs used.  For example,\n\n           $ git push --prune remote refs/heads/*:refs/remotes/repo1/*\n\n\twill push all local branches in our repository to refs with\n\tcorresponding names under refs/remotes/repo1/ at the remote, and\n\tremoves remote's refs in refs/remotes/repo1/ that no longer have\n        corresponding local branches in our repository.  The refs at the\n        remote outside refs/remotes/repo1/ are not affected.\n\nIn order to alley the worries raised in the previous discussion, something\nto the effect of the last sentence above is crucial to have, I would think. \n\n> It's useful to conveniently synchronize all the local branches to\n> certain remote.\n\nWhen an update to this patch comes with a documentation update to\nillustrate how to exercise this useful feature with an example, it will\nstart to make sense to write \"this is useful\" in the log message.  I know\nyou haven't gotten around the documentation part while the patch is marked\nas RFC, and that is OK.\n\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  builtin/push.c |    2 ++\n>  remote.c       |   29 ++++++++++++++++++++++++++---\n>  remote.h       |    3 ++-\n>  transport.c    |    2 ++\n>  transport.h    |    1 +\n>  5 files changed, 33 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 35cce53..46b99b1 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BIT('u', \"set-upstream\", &flags, \"set upstream for git pull/status\",\n>  \t\t\tTRANSPORT_PUSH_SET_UPSTREAM),\n>  \t\tOPT_BOOLEAN(0, \"progress\", &progress, \"force progress reporting\"),\n> +\t\tOPT_BIT('p', \"prune\", &flags, \"prune locally removed refs\",\n> +\t\t\tTRANSPORT_PUSH_PRUNE),\n\nPlease refrain from squatting on a short-and-sweet one letter option\nbefore this new feature proves to be popular and useful in a few cycles,\nespecially when there already is a long option that begins with 'p'.\n\n>  \t\tOPT_END()\n>  \t};\n>  \n> diff --git a/remote.c b/remote.c\n> index 019aafc..0900bb5 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n>  }\n>  \n>  static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n> -\t\tint send_mirror, const struct refspec **ret_pat)\n> +\t\tint send_mirror, int dir, const struct refspec **ret_pat)\n\nCan we name this a bit better?  I first thought \"Huh? directory?\", and had\nto scratch my head, wondering if it is an offset into refs/heads/* string\nor something....\n\n>  {\n>  \tconst struct refspec *pat;\n>  \tchar *name;\n> @@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref\n>  \n>  \t\tif (rs[i].pattern) {\n>  \t\t\tconst char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;\n> -\t\t\tif (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {\n> +\t\t\tint match;\n> +\t\t\tif (dir == 0)\n> +\t\t\t\tmatch = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);\n> +\t\t\telse\n> +\t\t\t\tmatch = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);\n\n....until the code told us that it is some sort of direction of the\nmatching.  A symbolic constant or two would be even better.\n\nOriginally this funcion was fed a list of refs in the source (i.e. on our\nend, as this is only used in 'push') and matched them against the source\nside of the refspec, rs[i].src, to see under what name the destination\nside will store it (i.e. give dst_side as value to find out the result in\n&name).  This patch adds a new caller, who feeds a list of refs in the\ndestination (i.e. on the remote end) to find out how they map to the names\non our end (i.e. source).  So \"direction\" is not necessarily incorrect; it\nis the direction this function maps the names (either src-to-dst for the\noriginal caller, or dst-to-src for the new caller).\n\nPerhaps \"enum map_direction { SRC_TO_DST, DST_TO_SRC }\" or something?\n\n> +\t\t\tif (match) {\n>  \t\t\t\tmatching_refs = i;\n>  \t\t\t\tbreak;\n>  \t\t\t}\n\nSo what is the updated semantics of this function?  Is it still\nappropriate to name it \"check_pattern_match()\"?\n\nIt seems that by now this does a lot more than just \"check if a pattern\nmatches\".  Since your patch 2/3, it is a function that finds out the\nrefname in the remote that the given one refspec would try to update, and\nwith this patch, it can also map in the reverse direction, given the list\nof remote refs, finding out which local ref a refspec would use to update\nthem.\n\nAt the same time, to reduce risk of future breakage, we probably should\nrename this function to make it clear that this function is to be only\nused by the push side.\n\nPerhaps rename this to \"map_push_refs()\" or something in the patch 2/3?\n\n> @@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>  \tstruct refspec *rs;\n>  \tint send_all = flags & MATCH_REFS_ALL;\n>  \tint send_mirror = flags & MATCH_REFS_MIRROR;\n> +\tint send_prune = flags & MATCH_REFS_PRUNE;\n>  \tint errs;\n>  \tstatic const char *default_refspec[] = { \":\", NULL };\n>  \tstruct ref *ref, **dst_tail = tail_ref(dst);\n> @@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>  \t\tif (ref->peer_ref)\n>  \t\t\tcontinue;\n>  \n> -\t\tdst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);\n> +\t\tdst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);\n>  \t\tif (!dst_name)\n>  \t\t\tcontinue;\n>  \n> @@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>  \tfree_name:\n>  \t\tfree(dst_name);\n>  \t}\n> +\tif (send_prune) {\n> +\t\t/* check for missing refs on the remote */\n> +\t\tfor (ref = *dst; ref; ref = ref->next) {\n> +\t\t\tchar *src_name;\n> +\n> +\t\t\tif (ref->peer_ref)\n> +\t\t\t\t/* We're already sending something to this ref. */\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tsrc_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);\n> +\t\t\tif (src_name) {\n> +\t\t\t\tif (!find_ref_by_name(src, src_name))\n> +\t\t\t\t\tref->peer_ref = try_explicit_object_name(\"\");\n\nYuck.  You do not want it to \"try\" as its name says.  You just want to\ntrigger its \"delete\" codepath.\n\nPlease extract the body of \"if (!*name) { ... }\" block out of that\nfunction into a separate helper function, i.e.\n\n\tstatic struct ref *deleted_ref(void)\n        {\n\t\tstruct ref *ref = alloc_ref(\"(delete)\");\n                hashclr(ref->new_sha1);\n                return ref;\n        }\n\nthen update try_explicit_...() to call it, and call the same helper here.\n\nThis is not for runtime efficiency; feeding a constant to a function that\nsays try_foo() or check_bar() that makes decision on the parameter only to\ntrigger a partial codepath hurts readability.\n\n> +\t\t\t\tfree(src_name);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n>  \tif (errs)\n>  \t\treturn -1;\n>  \treturn 0;\n> diff --git a/remote.h b/remote.h\n> index b395598..341142c 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);\n>  enum match_refs_flags {\n>  \tMATCH_REFS_NONE\t\t= 0,\n>  \tMATCH_REFS_ALL \t\t= (1 << 0),\n> -\tMATCH_REFS_MIRROR\t= (1 << 1)\n> +\tMATCH_REFS_MIRROR\t= (1 << 1),\n> +\tMATCH_REFS_PRUNE\t= (1 << 2),\n>  };\n>  \n>  /* Reporting of tracking info */\n> diff --git a/transport.c b/transport.c\n> index cac0c06..c20267c 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,\n>  \t\t\tmatch_flags |= MATCH_REFS_ALL;\n>  \t\tif (flags & TRANSPORT_PUSH_MIRROR)\n>  \t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n> +\t\tif (flags & TRANSPORT_PUSH_PRUNE)\n> +\t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n\nDoes it make sense to specify --prune when --mirror is in effect?  If so,\nhow would it behave differently from a vanilla --mirror?  If not, should\nit be detected as an error?\n\nI couldn't infer from the context shown in the patch, but how in general\ndoes this new feature interact with the codepath for --mirror?\n\n>  \t\tif (match_push_refs(local_refs, &remote_refs,\n>  \t\t\t\t    refspec_nr, refspec, match_flags)) {\n> diff --git a/transport.h b/transport.h\n> index 059b330..5d30328 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -101,6 +101,7 @@ struct transport {\n>  #define TRANSPORT_PUSH_MIRROR 8\n>  #define TRANSPORT_PUSH_PORCELAIN 16\n>  #define TRANSPORT_PUSH_SET_UPSTREAM 32\n> +#define TRANSPORT_PUSH_PRUNE 64\n>  #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n\nHrm...?\n\n>  #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n"},{"id":"185084","messageId":"CACsJy8ACoF=2dEUoFvT8bQDmUsjJA2_VWvTSJV6fakqyCLBCYg@mail.gmail.com","threadId":"29656","inReplyTo":"1329505957-24595-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 0/3] push: add 'prune' option","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-21T15:30:31Z","receivedAt":"2012-02-21T15:30:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n> As an example I put my use-case; I want to backup *all* my local branches to a\n> personal repository, and I want to remove branches that I have removed from my\n> local repository. git push personal 'refs/heads/*' mostly does the job, but it\n> doesn't remove anything, and that's where 'prune' comes from.\n\nYeah, may I have \"fetch --prune\" too, please? Looking at diffstat\ngives me a feeling that you only need to add maybe four lines to\nbuiltin/fetch.c and my dream would come true.\n-- \nDuy\n"},{"id":"185088","messageId":"20120221173548.GB32376@sigill.intra.peff.net","threadId":"29656","inReplyTo":"CACsJy8ACoF=2dEUoFvT8bQDmUsjJA2_VWvTSJV6fakqyCLBCYg@mail.gmail.com","subject":"Re: [RFC/PATCH 0/3] push: add 'prune' option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-21T17:35:48Z","receivedAt":"2012-02-21T17:35:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 21, 2012 at 10:30:31PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n> > As an example I put my use-case; I want to backup *all* my local branches to a\n> > personal repository, and I want to remove branches that I have removed from my\n> > local repository. git push personal 'refs/heads/*' mostly does the job, but it\n> > doesn't remove anything, and that's where 'prune' comes from.\n> \n> Yeah, may I have \"fetch --prune\" too, please? Looking at diffstat\n> gives me a feeling that you only need to add maybe four lines to\n> builtin/fetch.c and my dream would come true.\n\nHuh? Don't we already have \"fetch --prune\"?\n\n-Peff\n"},{"id":"185134","messageId":"CACsJy8C6K8UteZjH+mo88SeNWUf6yFyRRi_EQHa84ETNvv0iGA@mail.gmail.com","threadId":"29656","inReplyTo":"20120221173548.GB32376@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/3] push: add 'prune' option","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-22T01:45:33Z","receivedAt":"2012-02-22T01:45:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 22, 2012 at 12:35 AM, Jeff King <peff@peff.net> wrote:\n> Huh? Don't we already have \"fetch --prune\"?\n\nWe do indeed. I went to git-fetch.txt, searched for prune but missed\nmany \"include\" directives in there.\n-- \nDuy\n"},{"id":"185192","messageId":"CAMP44s2HUG_ocHBaVpcsZHWMf2Tww+=bVun5H9+S5EGkoiJHRQ@mail.gmail.com","threadId":"29656","inReplyTo":"7vvcn5qecl.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-22T20:15:41Z","receivedAt":"2012-02-22T20:15:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> There's a lot of code that can be consolidated there, and will be useful\n>> for next patches.\n>>\n>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> ---\n>>  remote.c |   59 ++++++++++++++++++++++++++++++-----------------------------\n>>  1 files changed, 30 insertions(+), 29 deletions(-)\n>>\n>> diff --git a/remote.c b/remote.c\n>> index 55d68d1..019aafc 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n>>       return errs;\n>>  }\n>>\n>> -static const struct refspec *check_pattern_match(const struct refspec *rs,\n>> -                                              int rs_nr,\n>> -                                              const struct ref *src)\n>> +static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n>> +             int send_mirror, const struct refspec **ret_pat)\n>>  {\n>\n> For a change that not just adds parameters but removes an existing one,\n> this is way under-described with neither in-code comment nor log message.\n\nBut it doesn't. src is renamed to ref.\n\n>> +     const struct refspec *pat;\n>> +     char *name;\n>>       int i;\n>>       int matching_refs = -1;\n>>       for (i = 0; i < rs_nr; i++) {\n>> @@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,\n>>                       continue;\n>>               }\n>>\n>> -             if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,\n>> -                                                          NULL, NULL))\n>> -                     return rs + i;\n>> +             if (rs[i].pattern) {\n>> +                     const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;\n>> +                     if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {\n>> +                             matching_refs = i;\n>> +                             break;\n>\n> We used to discard what match_name_with_pattern() finds out by matching a\n> wildcard refspec against the ref by passing two NULLs.  This updates the\n> code to capture what destination ref ref->name is mapped to, by using the\n> same logic as the original and only caller, i.e. 'foo' without destination\n> maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.\n>\n> This function is not used by fetching side of the codepath, so we do not\n> have to worry about its need to use different dst_side selection logic\n> (i.e. 'foo' without destination maps to \"do not store anywhere other than\n> FETCH_HEAD\").  Good.\n\nI actually didn't parse a lot of that.\n\n>> +                     }\n>> +             }\n>>       }\n>> -...\n>> +     if (matching_refs == -1)\n>>               return NULL;\n>> +\n>> +     pat = rs + matching_refs;\n>> +     if (pat->matching) {\n>> +             /*\n>> +              * \"matching refs\"; traditionally we pushed everything\n>> +              * including refs outside refs/heads/ hierarchy, but\n>> +              * that does not make much sense these days.\n>> +              */\n>> +             if (!send_mirror && prefixcmp(ref->name, \"refs/heads/\"))\n>> +                     return NULL;\n>> +             name = xstrdup(ref->name);\n>> +     }\n>\n> So you are moving some code from what the sole caller of this function\n> does after calling us, and that is where the new parameters come from.\n> And by doing so, you do not have to run the same match_name_with_pattern()\n> again.  OK.\n\nIndeed.\n\n-- \nFelipe Contreras\n"},{"id":"185199","messageId":"CAMP44s3+XCM1E_AtW1yGifmGoGSkFSpSTaFbbMffz+hmUzWahw@mail.gmail.com","threadId":"29656","inReplyTo":"7vobsxqebz.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 3/3] push: add 'prune' option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-22T20:43:05Z","receivedAt":"2012-02-22T20:43:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> This will allow us to remove refs from the remote that have been removed\n>> locally.\n>\n> Can you enhance this a bit more to summarize the gist of what the semantic\n> of this new feature is, perhaps like this:\n>\n>        After pushing refs, \"git push --prune\" will remove refs from the\n>        remote that existed before the push and would have been pushed\n>        from us if we had some local refs that would have matched the\n>        refspecs used.  For example,\n>\n>           $ git push --prune remote refs/heads/*:refs/remotes/repo1/*\n>\n>        will push all local branches in our repository to refs with\n>        corresponding names under refs/remotes/repo1/ at the remote, and\n>        removes remote's refs in refs/remotes/repo1/ that no longer have\n>        corresponding local branches in our repository.  The refs at the\n>        remote outside refs/remotes/repo1/ are not affected.\n>\n> In order to alley the worries raised in the previous discussion, something\n> to the effect of the last sentence above is crucial to have, I would think.\n\nOK.\n\n>> --- a/builtin/push.c\n>> +++ b/builtin/push.c\n>> @@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>>               OPT_BIT('u', \"set-upstream\", &flags, \"set upstream for git pull/status\",\n>>                       TRANSPORT_PUSH_SET_UPSTREAM),\n>>               OPT_BOOLEAN(0, \"progress\", &progress, \"force progress reporting\"),\n>> +             OPT_BIT('p', \"prune\", &flags, \"prune locally removed refs\",\n>> +                     TRANSPORT_PUSH_PRUNE),\n>\n> Please refrain from squatting on a short-and-sweet one letter option\n> before this new feature proves to be popular and useful in a few cycles,\n> especially when there already is a long option that begins with 'p'.\n\nOK.\n\n>>               OPT_END()\n>>       };\n>>\n>> diff --git a/remote.c b/remote.c\n>> index 019aafc..0900bb5 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n>>  }\n>>\n>>  static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,\n>> -             int send_mirror, const struct refspec **ret_pat)\n>> +             int send_mirror, int dir, const struct refspec **ret_pat)\n>\n> Can we name this a bit better?  I first thought \"Huh? directory?\", and had\n> to scratch my head, wondering if it is an offset into refs/heads/* string\n> or something....\n\nOK.\n\n>>  {\n>>       const struct refspec *pat;\n>>       char *name;\n>> @@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref\n>>\n>>               if (rs[i].pattern) {\n>>                       const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;\n>> -                     if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {\n>> +                     int match;\n>> +                     if (dir == 0)\n>> +                             match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);\n>> +                     else\n>> +                             match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);\n>\n> ....until the code told us that it is some sort of direction of the\n> matching.  A symbolic constant or two would be even better.\n>\n> Originally this funcion was fed a list of refs in the source (i.e. on our\n> end, as this is only used in 'push') and matched them against the source\n> side of the refspec, rs[i].src, to see under what name the destination\n> side will store it (i.e. give dst_side as value to find out the result in\n> &name).  This patch adds a new caller, who feeds a list of refs in the\n> destination (i.e. on the remote end) to find out how they map to the names\n> on our end (i.e. source).  So \"direction\" is not necessarily incorrect; it\n> is the direction this function maps the names (either src-to-dst for the\n> original caller, or dst-to-src for the new caller).\n>\n> Perhaps \"enum map_direction { SRC_TO_DST, DST_TO_SRC }\" or something?\n\nI think only FROM_SRC, FROM_DST is more than enough to figure it out.\n\n>> +                     if (match) {\n>>                               matching_refs = i;\n>>                               break;\n>>                       }\n>\n> So what is the updated semantics of this function?  Is it still\n> appropriate to name it \"check_pattern_match()\"?\n>\n> It seems that by now this does a lot more than just \"check if a pattern\n> matches\".  Since your patch 2/3, it is a function that finds out the\n> refname in the remote that the given one refspec would try to update, and\n> with this patch, it can also map in the reverse direction, given the list\n> of remote refs, finding out which local ref a refspec would use to update\n> them.\n>\n> At the same time, to reduce risk of future breakage, we probably should\n> rename this function to make it clear that this function is to be only\n> used by the push side.\n>\n> Perhaps rename this to \"map_push_refs()\" or something in the patch 2/3?\n\nI think get_ref_match() would be more appropriate because we are\nacting on a specific (singular) ref, and the primary thing we care\nabout is getting the peer name, based on the refspec match, which we\nmight want as a return value.\n\n>> @@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>>       struct refspec *rs;\n>>       int send_all = flags & MATCH_REFS_ALL;\n>>       int send_mirror = flags & MATCH_REFS_MIRROR;\n>> +     int send_prune = flags & MATCH_REFS_PRUNE;\n>>       int errs;\n>>       static const char *default_refspec[] = { \":\", NULL };\n>>       struct ref *ref, **dst_tail = tail_ref(dst);\n>> @@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>>               if (ref->peer_ref)\n>>                       continue;\n>>\n>> -             dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);\n>> +             dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);\n>>               if (!dst_name)\n>>                       continue;\n>>\n>> @@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,\n>>       free_name:\n>>               free(dst_name);\n>>       }\n>> +     if (send_prune) {\n>> +             /* check for missing refs on the remote */\n>> +             for (ref = *dst; ref; ref = ref->next) {\n>> +                     char *src_name;\n>> +\n>> +                     if (ref->peer_ref)\n>> +                             /* We're already sending something to this ref. */\n>> +                             continue;\n>> +\n>> +                     src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);\n>> +                     if (src_name) {\n>> +                             if (!find_ref_by_name(src, src_name))\n>> +                                     ref->peer_ref = try_explicit_object_name(\"\");\n>\n> Yuck.  You do not want it to \"try\" as its name says.  You just want to\n> trigger its \"delete\" codepath.\n>\n> Please extract the body of \"if (!*name) { ... }\" block out of that\n> function into a separate helper function, i.e.\n>\n>        static struct ref *deleted_ref(void)\n>        {\n>                struct ref *ref = alloc_ref(\"(delete)\");\n>                hashclr(ref->new_sha1);\n>                return ref;\n>        }\n>\n> then update try_explicit_...() to call it, and call the same helper here.\n>\n> This is not for runtime efficiency; feeding a constant to a function that\n> says try_foo() or check_bar() that makes decision on the parameter only to\n> trigger a partial codepath hurts readability.\n\nAll right.\n\n>> +                             free(src_name);\n>> +                     }\n>> +             }\n>> +     }\n>>       if (errs)\n>>               return -1;\n>>       return 0;\n>> diff --git a/remote.h b/remote.h\n>> index b395598..341142c 100644\n>> --- a/remote.h\n>> +++ b/remote.h\n>> @@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);\n>>  enum match_refs_flags {\n>>       MATCH_REFS_NONE         = 0,\n>>       MATCH_REFS_ALL          = (1 << 0),\n>> -     MATCH_REFS_MIRROR       = (1 << 1)\n>> +     MATCH_REFS_MIRROR       = (1 << 1),\n>> +     MATCH_REFS_PRUNE        = (1 << 2),\n>>  };\n>>\n>>  /* Reporting of tracking info */\n>> diff --git a/transport.c b/transport.c\n>> index cac0c06..c20267c 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,\n>>                       match_flags |= MATCH_REFS_ALL;\n>>               if (flags & TRANSPORT_PUSH_MIRROR)\n>>                       match_flags |= MATCH_REFS_MIRROR;\n>> +             if (flags & TRANSPORT_PUSH_PRUNE)\n>> +                     match_flags |= MATCH_REFS_PRUNE;\n>\n> Does it make sense to specify --prune when --mirror is in effect?  If so,\n> how would it behave differently from a vanilla --mirror?  If not, should\n> it be detected as an error?\n\nProbably doesn't make sense, should be an error.\n\n> I couldn't infer from the context shown in the patch, but how in general\n> does this new feature interact with the codepath for --mirror?\n>\n>>               if (match_push_refs(local_refs, &remote_refs,\n>>                                   refspec_nr, refspec, match_flags)) {\n>> diff --git a/transport.h b/transport.h\n>> index 059b330..5d30328 100644\n>> --- a/transport.h\n>> +++ b/transport.h\n>> @@ -101,6 +101,7 @@ struct transport {\n>>  #define TRANSPORT_PUSH_MIRROR 8\n>>  #define TRANSPORT_PUSH_PORCELAIN 16\n>>  #define TRANSPORT_PUSH_SET_UPSTREAM 32\n>> +#define TRANSPORT_PUSH_PRUNE 64\n>>  #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n>\n> Hrm...?\n\nProbably some rebase mistake =/\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"185203","messageId":"7v8vjua8p9.fsf@alter.siamese.dyndns.org","threadId":"29656","inReplyTo":"CAMP44s3+XCM1E_AtW1yGifmGoGSkFSpSTaFbbMffz+hmUzWahw@mail.gmail.com","subject":"Re: [RFC/PATCH 3/3] push: add 'prune' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T20:56:18Z","receivedAt":"2012-02-22T20:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Perhaps \"enum map_direction { SRC_TO_DST, DST_TO_SRC }\" or something?\n>\n> I think only FROM_SRC, FROM_DST is more than enough to figure it out.\n\nYeah, as you can tell from my \"or something\", as long as the name makes\nthe direction clear, I don't care too much about the exact spelling.\n\n>> Perhaps rename this to \"map_push_refs()\" or something in the patch 2/3?\n>\n> I think get_ref_match() would be more appropriate because we are\n> acting on a specific (singular) ref, and the primary thing we care\n> about is getting the peer name, based on the refspec match, which we\n> might want as a return value.\n\nAgain, as long as the name makes it clear this is meant only for \"push\"\nand never be used for \"fetch\", I am fine with it.  One way to make it sure\nis to have a substring \"_push\" somewhere in that name.\n\n> Probably some rebase mistake =/\n\nThanks; I was wondering if there is something subtle unexplained going on.\n"},{"id":"185205","messageId":"7v4nuia8fk.fsf@alter.siamese.dyndns.org","threadId":"29656","inReplyTo":"CAMP44s2HUG_ocHBaVpcsZHWMf2Tww+=bVun5H9+S5EGkoiJHRQ@mail.gmail.com","subject":"Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T21:02:07Z","receivedAt":"2012-02-22T21:02:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> For a change that not just adds parameters but removes an existing one,\n>> this is way under-described with neither in-code comment nor log message.\n>\n> But it doesn't. src is renamed to ref.\n\nThat is exactly why I mentioned that this is a \"chance\" in the part of my\nmessage you did not quote ;-).\n\nIt was a chance for you to learn how the thought process of others may\nwork or not work well, depending on how well the log message at the top\nprepares their mind before they start to read the patch, by looking at how\none reader (me) tried to figure your patch out.\n\nAnything that I said in the message you are responding to that tempts you\nto say \"No you are reading wrong\" is an indication that the patch did not\ndo a good job to help the reader understand what you wanted to do.\n"},{"id":"185223","messageId":"CAMP44s0DnVfd_Ged5rnhM4BF1jEtm4jzyq9sJAoEtQ-0CM5_aA@mail.gmail.com","threadId":"29656","inReplyTo":"CAMP44s3+XCM1E_AtW1yGifmGoGSkFSpSTaFbbMffz+hmUzWahw@mail.gmail.com","subject":"Re: [RFC/PATCH 3/3] push: add 'prune' option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-22T23:06:41Z","receivedAt":"2012-02-22T23:06:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Feb 22, 2012 at 10:43 PM, Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n> On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Does it make sense to specify --prune when --mirror is in effect?  If so,\n>> how would it behave differently from a vanilla --mirror?  If not, should\n>> it be detected as an error?\n>\n> Probably doesn't make sense, should be an error.\n\nActually, after writing the patch for the documentation I realized\nthis would be difficult to describe: you can use --all --prune, but\nnot --mirror --prune, and the documentation currently has '[--all |\n--mirror | --tags]'. So I decided to make it orthogonal, you can use\n--prune with --all, --tags, *and* --mirror. Of course, using --prune\n--mirror is the same as --mirror.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"185226","messageId":"7vaa4a8lvm.fsf@alter.siamese.dyndns.org","threadId":"29656","inReplyTo":"CAMP44s0DnVfd_Ged5rnhM4BF1jEtm4jzyq9sJAoEtQ-0CM5_aA@mail.gmail.com","subject":"Re: [RFC/PATCH 3/3] push: add 'prune' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T23:54:37Z","receivedAt":"2012-02-22T23:54:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> ... Of course, using --prune\n> --mirror is the same as --mirror.\n\nSounds good.  Thanks.\n"}]}