threads / patch / 13751

patch, 3 partsMake "git-remote rm" delete refs acccording to fetch specs

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

## tl;dr

7 messages between Jun 1, 2008 and Jun 2, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Shawn O. Pearce· Jun 1, 2008, 03:59 UTC · lore

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(-)
Show changes to builtin-remote.c +6 −5
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
Shawn O. Pearce· Jun 1, 2008, 04:28 UTC · re: Shawn O. Pearce · lore

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

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(-)
Show changes to builtin-remote.c +54 −14
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
Shawn O. Pearce· Jun 1, 2008, 04:40 UTC · re: Shawn O. Pearce · lore

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

"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<--
Show changes to t/t5505-remote.sh +12 −0
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.
Junio C Hamano· Jun 1, 2008, 05:29 UTC · re: Shawn O. Pearce · lore

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

"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.

Shawn O. Pearce· Jun 1, 2008, 06:30 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> "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.
Show 6 quoted lines
> 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.
Junio C Hamano· Jun 1, 2008, 08:28 UTC · re: Shawn O. Pearce · lore

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

"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.

Jon Loeliger· Jun 2, 2008, 17:10 UTC · re: Shawn O. Pearce · lore

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

On Sun, 2008-06-01 at 02:30 -0400, Shawn O. Pearce wrote:
Show 40 quoted lines
> 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.

Show 8 quoted lines
> 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

← back to recent threads