[PATCH 3/3] Make "git-remote rm" delete refs acccording to fetch specs

[PATCH 3/3] Make "git-remote rm" delete refs acccording to fetch specs

[Shawn O. Pearce]

A remote may be configured to fetch into tracking branches that
don't match its name.  A user may have created a remote by hand
that will fetch to a different tracking branch namespace:

  [remote "alt"]
    url = git://repo.or.cz/alt-git.git
    fetch = refs/heads/*:refs/remotes/origin/*

When deleting remote alt we should clean up the refs whose names
start with "refs/remotes/origin/", even though the remote itself
was named alt by the user.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 I'm not sure this is the right thing to do here.  If another
 remote is still using these tracking refs (such as the example
 in my prior patch of this series) we may not want to remove these
 when we remove the remote.

 builtin-remote.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index e5cfc88..6dbc3d4 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -268,7 +268,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 }
 
 struct branches_for_remote {
-	const char *prefix;
+	struct remote *remote;
 	struct path_list *branches;
 };
 
@@ -276,8 +276,11 @@ static int add_branch_for_removal(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct branches_for_remote *branches = cb_data;
+	struct refspec refspec;
 
-	if (!prefixcmp(refname, branches->prefix)) {
+	memset(&refspec, 0, sizeof(refspec));
+	refspec.dst = (char *)refname;
+	if (!remote_find_tracking(branches->remote, &refspec)) {
 		struct path_list_item *item;
 
 		/* make sure that symrefs are deleted */
@@ -352,9 +355,7 @@ static int rm(int argc, const char **argv)
 	 * the branches one by one, since for_each_ref() relies on cached
 	 * refs, which are invalidated when deleting a branch.
 	 */
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "refs/remotes/%s/", remote->name);
-	cb_data.prefix = buf.buf;
+	cb_data.remote = remote;
 	i = for_each_ref(add_branch_for_removal, &cb_data);
 	strbuf_release(&buf);
 
-- 
1.5.6.rc0.158.g7c7a1

[PATCH 3/3 v2] Make "git-remote rm" delete refs acccording to fetch specs

[Shawn O. Pearce]

A remote may be configured to fetch into tracking branches that
don't match its name.  A user may have created a remote by hand
that will fetch to a different tracking branch namespace:

  [remote "alt"]
    url = git://repo.or.cz/alt-git.git
    fetch = refs/heads/*:refs/remotes/origin/*

When deleting remote alt we should clean up the refs whose names
start with "refs/remotes/origin/", even though the remote itself
was named alt by the user.

To avoid deleting refs used by another remote we only clear refs
that are unique to this remote.  This prevents `git prune rm alt`
from removing the refs used by say origin if alt was just using a
different URL for the same repository.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This is a longer, but better version of this patch.  Instead of
 blindly deleting the refs we remove them only if this is the last
 remote that would write to the local tracking ref.

 builtin-remote.c |   68 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index e5cfc88..c49f00f 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -267,27 +267,65 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 	return 0;
 }
 
+struct known_remote {
+	struct known_remote *next;
+	struct remote *remote;
+};
+
+struct known_remotes {
+	struct remote *to_delete;
+	struct known_remote *list;
+};
+
+static int add_known_remote(struct remote *remote, void *cb_data)
+{
+	struct known_remotes *all = cb_data;
+	struct known_remote *r;
+
+	if (!strcmp(all->to_delete->name, remote->name))
+		return 0;
+
+	r = xmalloc(sizeof(*r));
+	r->remote = remote;
+	r->next = all->list;
+	all->list = r;
+	return 0;
+}
+
 struct branches_for_remote {
-	const char *prefix;
+	struct remote *remote;
 	struct path_list *branches;
+	struct known_remotes *keep;
 };
 
 static int add_branch_for_removal(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct branches_for_remote *branches = cb_data;
+	struct refspec refspec;
+	struct path_list_item *item;
+	struct known_remote *kr;
 
-	if (!prefixcmp(refname, branches->prefix)) {
-		struct path_list_item *item;
+	memset(&refspec, 0, sizeof(refspec));
+	refspec.dst = (char *)refname;
+	if (remote_find_tracking(branches->remote, &refspec))
+		return 0;
+
+	/* don't delete a branch if another remote also uses it */
+	for (kr = branches->keep->list; kr; kr = kr->next) {
+		memset(&refspec, 0, sizeof(refspec));
+		refspec.dst = (char *)refname;
+		if (!remote_find_tracking(kr->remote, &refspec))
+			return 0;
+	}
 
-		/* make sure that symrefs are deleted */
-		if (flags & REF_ISSYMREF)
-			return unlink(git_path(refname));
+	/* make sure that symrefs are deleted */
+	if (flags & REF_ISSYMREF)
+		return unlink(git_path(refname));
 
-		item = path_list_append(refname, branches->branches);
-		item->util = xmalloc(20);
-		hashcpy(item->util, sha1);
-	}
+	item = path_list_append(refname, branches->branches);
+	item->util = xmalloc(20);
+	hashcpy(item->util, sha1);
 
 	return 0;
 }
@@ -313,8 +351,9 @@ static int rm(int argc, const char **argv)
 	};
 	struct remote *remote;
 	struct strbuf buf;
+	struct known_remotes known_remotes = { NULL, NULL };
 	struct path_list branches = { NULL, 0, 0, 1 };
-	struct branches_for_remote cb_data = { NULL, &branches };
+	struct branches_for_remote cb_data = { NULL, &branches, &known_remotes };
 	int i;
 
 	if (argc != 2)
@@ -324,6 +363,9 @@ static int rm(int argc, const char **argv)
 	if (!remote)
 		die("No such remote: %s", argv[1]);
 
+	known_remotes.to_delete = remote;
+	for_each_remote(add_known_remote, &known_remotes);
+
 	strbuf_init(&buf, 0);
 	strbuf_addf(&buf, "remote.%s", remote->name);
 	if (git_config_rename_section(buf.buf, NULL) < 1)
@@ -352,9 +394,7 @@ static int rm(int argc, const char **argv)
 	 * the branches one by one, since for_each_ref() relies on cached
 	 * refs, which are invalidated when deleting a branch.
 	 */
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "refs/remotes/%s/", remote->name);
-	cb_data.prefix = buf.buf;
+	cb_data.remote = remote;
 	i = for_each_ref(add_branch_for_removal, &cb_data);
 	strbuf_release(&buf);
 
-- 
1.5.6.rc0.158.g7c7a1

Re: [PATCH 3/3 v2] Make "git-remote rm" delete refs acccording to fetch specs

[Shawn O. Pearce]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> To avoid deleting refs used by another remote we only clear refs
> that are unique to this remote.  This prevents `git prune rm alt`
> from removing the refs used by say origin if alt was just using a
> different URL for the same repository.

Hmm.  You probably should squash this test case into that patch.

--8<--
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0d7ed1f..96e461b 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -182,6 +182,18 @@ test_expect_success 'add alt && prune' '
 	 git rev-parse --verify refs/remotes/origin/side2)
 '
 
+test_expect_success 'rm alt' '
+	(cd alttst &&
+	 git remote rm alt &&
+	 tokens_match z z$(git config remote.alt.url) &&
+	 tokens_match z z$(git config remote.alt.fetch) &&
+	 git rev-parse --verify refs/remotes/origin/side2 &&
+	 git remote rm origin &&
+	 tokens_match z z$(git config remote.alt.url) &&
+	 tokens_match z z$(git config remote.alt.fetch) &&
+	 ! git rev-parse --verify refs/remotes/origin/side2)
+'
+
 cat > one/expect << EOF
   apis/master
   apis/side

-- 
Shawn.

Re: [PATCH 3/3 v2] Make "git-remote rm" delete refs acccording to fetch specs

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

>  This is a longer, but better version of this patch.  Instead of
>  blindly deleting the refs we remove them only if this is the last
>  remote that would write to the local tracking ref.

If this is a better version than the previous one, then probably "git
remote prune" patch to unconditionally remove ones that do not exist in
one of the remotes that fetch into the tracking namespace also needs to be
rethought, doesn't it?  Another remote may still have it but you obviously
do not know that until you check.

Admittedly, next fetch from the other remote may or may not resurrect them
and either way it is not a big deal.

I think this is exactly the same issue as this improvement in [3/3] deals
with.  If "git remote rm" of one remote removed the shared tracked refs,
next fetch from the other remote would resurrect them if the other remote
still exists.  It may probably feel better to be extra careful like this
improved patch, but I doubt it would matter in practice.  After all,
people who creates such a configuration would know what they are doing.

Re: [PATCH 3/3 v2] Make "git-remote rm" delete refs acccording to fetch specs

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> >  This is a longer, but better version of this patch.  Instead of
> >  blindly deleting the refs we remove them only if this is the last
> >  remote that would write to the local tracking ref.
> 
> If this is a better version than the previous one, then probably "git
> remote prune" patch to unconditionally remove ones that do not exist in
> one of the remotes that fetch into the tracking namespace also needs to be
> rethought, doesn't it?

Nope.  I've thought about this already.  ;-)
 
> Admittedly, next fetch from the other remote may or may not resurrect them
> and either way it is not a big deal.

Correct.

> I think this is exactly the same issue as this improvement in [3/3] deals
> with.  If "git remote rm" of one remote removed the shared tracked refs,
> next fetch from the other remote would resurrect them if the other remote
> still exists.  It may probably feel better to be extra careful like this
> improved patch, but I doubt it would matter in practice.  After all,
> people who creates such a configuration would know what they are doing.

I don't think it matters in practice.

If the user has configured two different remotes with different URLs
fetching to the same set of tracking branches, well, they get what
they get when their prune and fetch start fighting over a tracking
branch existing or disappearing.

Today I found these misfeatures in git-remote because I sort of do
what I showed in my second patch.  I have two remotes which fetch to
the same tracking branches, as they fetch from the same repository,
just over two different protocols.  There never should be a time
where I can see a difference between them, unless its just a race
condition where someone else created or deleted a branch between
my two sequential git-fetch/git-remote calls.

I think the behavior in 2/2 and 3/3v2 is the best we can do, short of
contacting all other remotes which go to the same tracking branch.
But that may not be possible.  One reason for using two different
remote configurations to the same tracking branches is to make
the URL differ.  Think fetching against repo.or.cz using git://
and also http://, where http:// is necessary when you are stuck
behind a @#U*!@(#*!@#*!@(@!! proxy server.  We cannot contact both
remotes to verify the branch really is safe to prune.

-- 
Shawn.

Re: [PATCH 3/3 v2] Make "git-remote rm" delete refs acccording to fetch specs

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> I think the behavior in 2/2 and 3/3v2 is the best we can do,...

Oh, I think you already know I agree with that.  I was just pointing out
that the extra niceness would not have mattered that much in practice, but
if the code is already written and is not crappy, why not ;-)

Not blindly assuming that the remote nickname matches the actualy used
namespace under refs/remotes _is_ a good bugfix regardless of the issue of
overlapping configuration. 

Re: [PATCH 3/3 v2] Make "git-remote rm" delete refs acccording to fetch specs

[Jon Loeliger]

On Sun, 2008-06-01 at 02:30 -0400, Shawn O. Pearce wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > "Shawn O. Pearce" <spearce@spearce.org> writes:
> > 
> > >  This is a longer, but better version of this patch.  Instead of
> > >  blindly deleting the refs we remove them only if this is the last
> > >  remote that would write to the local tracking ref.
> > 
> > If this is a better version than the previous one, then probably "git
> > remote prune" patch to unconditionally remove ones that do not exist in
> > one of the remotes that fetch into the tracking namespace also needs to be
> > rethought, doesn't it?
> 
> Nope.  I've thought about this already.  ;-)
>  
> > Admittedly, next fetch from the other remote may or may not resurrect them
> > and either way it is not a big deal.
> 
> Correct.
> 
> > I think this is exactly the same issue as this improvement in [3/3] deals
> > with.  If "git remote rm" of one remote removed the shared tracked refs,
> > next fetch from the other remote would resurrect them if the other remote
> > still exists.  It may probably feel better to be extra careful like this
> > improved patch, but I doubt it would matter in practice.  After all,
> > people who creates such a configuration would know what they are doing.
> 
> I don't think it matters in practice.
> 
> If the user has configured two different remotes with different URLs
> fetching to the same set of tracking branches, well, they get what
> they get when their prune and fetch start fighting over a tracking
> branch existing or disappearing.
> 
> Today I found these misfeatures in git-remote because I sort of do
> what I showed in my second patch.  I have two remotes which fetch to
> the same tracking branches, as they fetch from the same repository,
> just over two different protocols.  There never should be a time
> where I can see a difference between them, unless its just a race
> condition where someone else created or deleted a branch between
> my two sequential git-fetch/git-remote calls.

This all seems sort of fragile to me.  Like maybe there
is a different name-space concept itching to get out here.
Like a [refspec "foo"] node in the config file or something
that clearly states the expected namespace for an remote.

> I think the behavior in 2/2 and 3/3v2 is the best we can do, short of
> contacting all other remotes which go to the same tracking branch.
> But that may not be possible.  One reason for using two different
> remote configurations to the same tracking branches is to make
> the URL differ.  Think fetching against repo.or.cz using git://
> and also http://, where http:// is necessary when you are stuck
> behind a @#U*!@(#*!@#*!@(@!! proxy server.  We cannot contact both
> remotes to verify the branch really is safe to prune.

Didn't we settle on an alternate URL mechanism that covered
this case already?  Or was that a different case/

CRS,
jdl