[PATCH RFC] builtin-push: add --delete as syntactic sugar for :foo

[PATCH RFC] builtin-push: add --delete as syntactic sugar for :foo

[Jan Krüger]

Refspecs without a source side have been reported as confusing by many.
As an alternative, this adds support for commands like:

    git push origin --delete somebranch

Specifically, --delete will prepend a colon to all colon-less refspecs
given on the command line.

Signed-off-by: Jan Krüger <jk@jk.gs>
---
Since I consider this extension pure syntactic sugar, it doesn't change
the underlying transport code. As such it's a relatively non-invasive
change.

One might imagine a different implementation that supports combining
--delete with --all and/or --tags, but perhaps it's better if people
are forced to do that kind of thing manually.

 builtin-push.c        |   15 +++++++++++++++
 t/t5516-fetch-push.sh |    6 ++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 8631c06..4ae9166 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -15,6 +15,7 @@ static const char * const push_usage[] = {
 };
 
 static int thin;
+static int deleterefs;
 static const char *receivepack;
 
 static const char **refspec;
@@ -44,6 +45,14 @@ static void set_refspecs(const char **refs, int nr)
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
+		if (deleterefs && !strchr(ref, ':')) {
+			char *delref;
+			int len = strlen(refs[i] + 1);
+			delref = xmalloc(len);
+			strcpy(delref, ":");
+			strcat(delref, refs[i]);
+			ref = delref;
+		}
 		add_refspec(ref);
 	}
 }
@@ -181,6 +190,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "all", &flags, "push all refs", TRANSPORT_PUSH_ALL),
 		OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
+		OPT_BOOLEAN( 0, "delete", &deleterefs, "delete refs"),
 		OPT_BOOLEAN( 0 , "tags", &tags, "push tags (can't be used with --all or --mirror)"),
 		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
@@ -193,6 +203,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
+		die("--delete is incompatible with --all, --mirror and --tags");
+	if (deleterefs && argc < 2)
+		die("--delete doesn't make sense without any refs");
+
 	if (tags)
 		add_refspec("refs/tags/*");
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6889a53..aa1450a 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -546,6 +546,12 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'allow deleting a ref using --delete' '
+	mk_test heads/master &&
+	git push testrepo --delete master &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
+'
+
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test heads/master
 	(cd testrepo &&
-- 
1.6.5.2.155.gbb47.dirty

Re: [PATCH RFC] builtin-push: add --delete as syntactic sugar for :foo

[Junio C Hamano]

Jan Krüger <jk@jk.gs> writes:

> Refspecs without a source side have been reported as confusing by many.
> As an alternative, this adds support for commands like:
>
>     git push origin --delete somebranch
>
> Specifically, --delete will prepend a colon to all colon-less refspecs
> given on the command line.

Will it barf and error out if there is any colon-ful one?  I think it
should.  I was about to write "I guess it could be argued both ways", but
after thinking about it for 5 seconds I do not see a sane way to explain a
command line "push origin --delete one two:three".

I agree with you that it wouldn't make sense to mix this with --all and
friends.

Re: [PATCH RFC] builtin-push: add --delete as syntactic sugar for :foo

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Will it barf and error out if there is any colon-ful one?  I think it
> should.  I was about to write "I guess it could be argued both ways", but
> after thinking about it for 5 seconds I do not see a sane way to explain a
> command line "push origin --delete one two:three".

Actually the one I had in mind was

	git push origin --delete one two:three four

If it were

	git push origin --tags --delete=one --delete=four two:three

it would be perfectly understandable, though.

No, I am not saying that we should make --delete take a parameter, allow
multiple of them, and make them compatible with --tags.  At least not yet.

Re: [PATCH RFC] builtin-push: add --delete as syntactic sugar for :foo

[Sverre Rabbelier]

Heya,

On Mon, Nov 9, 2009 at 17:59, Junio C Hamano <gitster@pobox.com> wrote:
> Jan Krüger <jk@jk.gs> writes:
>> Specifically, --delete will prepend a colon to all colon-less refspecs
>> given on the command line.
>
> Will it barf and error out if there is any colon-ful one?  I think it
> should.  I was about to write "I guess it could be argued both ways", but
> after thinking about it for 5 seconds I do not see a sane way to explain a
> command line "push origin --delete one two:three".

Did we not have this discussion not 3 months ago and decided it was a bad idea?

  http://thread.gmane.org/gmane.comp.version-control.git/125894

-- 
Cheers,

Sverre Rabbelier

[PATCH RFC v2] builtin-push: add --delete as syntactic sugar for :foo

[Jan Krüger]

Refspecs without a source side have been reported as confusing by many.
As an alternative, this adds support for commands like:

    git push origin --delete somebranch

Specifically, --delete will prepend a colon to all colon-less refspecs
given on the command line, and will refuse to accept refspecs with
colons to prevent undue confusion.

Signed-off-by: Jan Krüger <jk@jk.gs>
---
All good points, and I actually remembered to catch the colon-refspec
case a few minutes after sending the patch, but didn't have time to fix
it up. (Sorry for not considering the old thread; I had gotten
unsubscribed from the list back then).

I think with this change it becomes much saner. And no, I'm not
proposing to add --rename and --copy, too. ;)

The new error message is not completely technically correct since it's
still a refspec we're taking, we just force it to be without a colon.
On the other hand, this error message will hopefully make much more
sense to people who don't know all the background, and it's not too
horribly wrong either.

(Sverre added to Cc list from subthread)

 builtin-push.c        |   16 ++++++++++++++++
 t/t5516-fetch-push.sh |    6 ++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 8631c06..e40bfe8 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -15,6 +15,7 @@ static const char * const push_usage[] = {
 };
 
 static int thin;
+static int deleterefs;
 static const char *receivepack;
 
 static const char **refspec;
@@ -44,6 +45,15 @@ static void set_refspecs(const char **refs, int nr)
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
+		if (deleterefs && !strchr(ref, ':')) {
+			char *delref;
+			int len = strlen(refs[i] + 1);
+			delref = xmalloc(len);
+			strcpy(delref, ":");
+			strcat(delref, refs[i]);
+			ref = delref;
+		} else if (deleterefs)
+			die("--delete only accepts plain target ref names");
 		add_refspec(ref);
 	}
 }
@@ -181,6 +191,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "all", &flags, "push all refs", TRANSPORT_PUSH_ALL),
 		OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
+		OPT_BOOLEAN( 0, "delete", &deleterefs, "delete refs"),
 		OPT_BOOLEAN( 0 , "tags", &tags, "push tags (can't be used with --all or --mirror)"),
 		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
@@ -193,6 +204,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
+		die("--delete is incompatible with --all, --mirror and --tags");
+	if (deleterefs && argc < 2)
+		die("--delete doesn't make sense without any refs");
+
 	if (tags)
 		add_refspec("refs/tags/*");
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6889a53..aa1450a 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -546,6 +546,12 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'allow deleting a ref using --delete' '
+	mk_test heads/master &&
+	git push testrepo --delete master &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
+'
+
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test heads/master
 	(cd testrepo &&
-- 
1.6.5.2.155.gbb47.dirty

Re: [PATCH RFC v2] builtin-push: add --delete as syntactic sugar for :foo

[Nanako Shiraishi]

Junio, could you tell us what happened to this thread?

The patch implements "git push repo --delete branch" and rejects
--delete used with other options like --all and --tags, as
suggested in the initial review.  I think it makes sense, but
nothing happens after that.

Re: [PATCH RFC v2] builtin-push: add --delete as syntactic sugar for :foo

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Junio, could you tell us what happened to this thread?
>
> The patch implements "git push repo --delete branch" and rejects
> --delete used with other options like --all and --tags, as suggested in
> the initial review.  I think it makes sense, but nothing happens after
> that.

Sverre cited an old discussion and the discussion stalled.  I just re-read
the thread, and think the "this is a wrong idea" objection was primarily
about allowing --delete with non-delete kinds of refspecs, so in that
sense Jan's patch is a perfected form of the Sverre's patch from that old
discussion.

As a summary of the lesson learned and concensus from the old discussion,
I agree with this from Sverre:

  http://article.gmane.org/gmane.comp.version-control.git/125901

namely, (1) barf and abort if src:dst is given; (2) touch only refs given
from the command line, "push there --delete" without any refspec is an
error; (3) be careful about "git push there tag v1.0.0" form.

So if Jan or Sverre want to resurrect the topic, I am all for it.

[PATCH v3] builtin-push: add --delete as syntactic sugar for :foo

[Jan Krüger]

Refspecs without a source side have been reported as confusing by many.
As an alternative, this adds support for commands like:

    git push origin --delete somebranch
    git push origin --delete tag sometag

Specifically, --delete will prepend a colon to all colon-less refspecs
given on the command line, and will refuse to accept refspecs with
colons to prevent undue confusion.

Signed-off-by: Jan Krüger <jk@jk.gs>
---

Junio C Hamano <gitster@pobox.com> wrote:

> namely, (1) barf and abort if src:dst is given; (2) touch only refs
> given from the command line, "push there --delete" without any
> refspec is an error; (3) be careful about "git push there tag v1.0.0"
> form.
> 
> So if Jan or Sverre want to resurrect the topic, I am all for it.

Alrighty. I assume by (3) you meant that it should be possible to use
something like "push there --delete tag v1.0.0". Version 3 of the patch
adds this, and it also includes updated tests and (brief) documentation.

 Documentation/git-push.txt |    4 ++++
 builtin-push.c             |   26 +++++++++++++++++++++++---
 t/t5516-fetch-push.sh      |   16 ++++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 52c0538..e3eb1e8 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -91,6 +91,10 @@ nor in any Push line of the corresponding remotes file---see below).
 	will be tab-separated and sent to stdout instead of stderr.  The full
 	symbolic names of the refs will be given.
 
+--delete::
+	All listed refs are deleted from the remote repository. This is
+	the same as prefixing all refs with a colon.
+
 --tags::
 	All refs under `$GIT_DIR/refs/tags` are pushed, in
 	addition to refspecs explicitly listed on the command
diff --git a/builtin-push.c b/builtin-push.c
index dcfb53f..f7661d2 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -15,6 +15,7 @@ static const char * const push_usage[] = {
 };
 
 static int thin;
+static int deleterefs;
 static const char *receivepack;
 
 static const char **refspec;
@@ -39,11 +40,24 @@ static void set_refspecs(const char **refs, int nr)
 			if (nr <= ++i)
 				die("tag shorthand without <tag>");
 			len = strlen(refs[i]) + 11;
-			tag = xmalloc(len);
-			strcpy(tag, "refs/tags/");
+			if (deleterefs) {
+				tag = xmalloc(len+1);
+				strcpy(tag, ":refs/tags/");
+			} else {
+				tag = xmalloc(len);
+				strcpy(tag, "refs/tags/");
+			}
 			strcat(tag, refs[i]);
 			ref = tag;
-		}
+		} else if (deleterefs && !strchr(ref, ':')) {
+			char *delref;
+			int len = strlen(ref)+1;
+			delref = xmalloc(len);
+			strcpy(delref, ":");
+			strcat(delref, ref);
+			ref = delref;
+		} else if (deleterefs)
+			die("--delete only accepts plain target ref names");
 		add_refspec(ref);
 	}
 }
@@ -196,6 +210,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "all", &flags, "push all refs", TRANSPORT_PUSH_ALL),
 		OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
+		OPT_BOOLEAN( 0, "delete", &deleterefs, "delete refs"),
 		OPT_BOOLEAN( 0 , "tags", &tags, "push tags (can't be used with --all or --mirror)"),
 		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
@@ -209,6 +224,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
+		die("--delete is incompatible with --all, --mirror and --tags");
+	if (deleterefs && argc < 2)
+		die("--delete doesn't make sense without any refs");
+
 	if (tags)
 		add_refspec("refs/tags/*");
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 516127b..a17666c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -547,6 +547,22 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'allow deleting a ref using --delete' '
+	mk_test heads/master &&
+	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
+	git push testrepo --delete master &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
+'
+
+test_expect_success 'allow deleting a tag using --delete' '
+	mk_test heads/master &&
+	git tag -a -m dummy_message deltag heads/master &&
+	git push testrepo --tags &&
+	(cd testrepo && git rev-parse --verify -q refs/tags/deltag) &&
+	git push testrepo --delete tag deltag &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/tags/deltag)
+'
+
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test heads/master
 	(cd testrepo &&
-- 
1.6.6.60.gc2ff1

Re: [PATCH v3] builtin-push: add --delete as syntactic sugar for :foo

[Sverre Rabbelier]

Heya,

On Wed, Dec 30, 2009 at 03:52, Jan Krüger <jk@jk.gs> wrote:
>> So if Jan or Sverre want to resurrect the topic, I am all for it.
>
> Alrighty. I assume by (3) you meant that it should be possible to use
> something like "push there --delete tag v1.0.0". Version 3 of the patch
> adds this, and it also includes updated tests and (brief) documentation.

Thanks for picking this up.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v3] builtin-push: add --delete as syntactic sugar for :foo

[Junio C Hamano]

Jan Krüger <jk@jk.gs> writes:

> Refspecs without a source side have been reported as confusing by many.
> As an alternative, this adds support for commands like:
>
>     git push origin --delete somebranch
>     git push origin --delete tag sometag
>
> Specifically, --delete will prepend a colon to all colon-less refspecs
> given on the command line, and will refuse to accept refspecs with
> colons to prevent undue confusion.
>
> Signed-off-by: Jan Krüger <jk@jk.gs>
> ---

Thanks.  From a cursory read, the patch looks good.  We would however want
to have a test that has test_must_fail to protect the error codepath from
getting broken in the future.

[PATCH v4] builtin-push: add --delete as syntactic sugar for :foo

[Jan Krüger]

Refspecs without a source side have been reported as confusing by many.
As an alternative, this adds support for commands like:

    git push origin --delete somebranch
    git push origin --delete tag sometag

Specifically, --delete will prepend a colon to all colon-less refspecs
given on the command line, and will refuse to accept refspecs with
colons to prevent undue confusion.

Signed-off-by: Jan Krüger <jk@jk.gs>
---
 Documentation/git-push.txt |    4 ++++
 builtin-push.c             |   26 +++++++++++++++++++++++---
 t/t5516-fetch-push.sh      |   26 ++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 3 deletions(-)

Junio C Hamano <gitster@pobox.com> wrote:

> Thanks.  From a cursory read, the patch looks good.  We would however
> want to have a test that has test_must_fail to protect the error
> codepath from getting broken in the future.

Here you are.

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 52c0538..e3eb1e8 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -91,6 +91,10 @@ nor in any Push line of the corresponding remotes file---see below).
 	will be tab-separated and sent to stdout instead of stderr.  The full
 	symbolic names of the refs will be given.
 
+--delete::
+	All listed refs are deleted from the remote repository. This is
+	the same as prefixing all refs with a colon.
+
 --tags::
 	All refs under `$GIT_DIR/refs/tags` are pushed, in
 	addition to refspecs explicitly listed on the command
diff --git a/builtin-push.c b/builtin-push.c
index dcfb53f..f7661d2 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -15,6 +15,7 @@ static const char * const push_usage[] = {
 };
 
 static int thin;
+static int deleterefs;
 static const char *receivepack;
 
 static const char **refspec;
@@ -39,11 +40,24 @@ static void set_refspecs(const char **refs, int nr)
 			if (nr <= ++i)
 				die("tag shorthand without <tag>");
 			len = strlen(refs[i]) + 11;
-			tag = xmalloc(len);
-			strcpy(tag, "refs/tags/");
+			if (deleterefs) {
+				tag = xmalloc(len+1);
+				strcpy(tag, ":refs/tags/");
+			} else {
+				tag = xmalloc(len);
+				strcpy(tag, "refs/tags/");
+			}
 			strcat(tag, refs[i]);
 			ref = tag;
-		}
+		} else if (deleterefs && !strchr(ref, ':')) {
+			char *delref;
+			int len = strlen(ref)+1;
+			delref = xmalloc(len);
+			strcpy(delref, ":");
+			strcat(delref, ref);
+			ref = delref;
+		} else if (deleterefs)
+			die("--delete only accepts plain target ref names");
 		add_refspec(ref);
 	}
 }
@@ -196,6 +210,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "all", &flags, "push all refs", TRANSPORT_PUSH_ALL),
 		OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
+		OPT_BOOLEAN( 0, "delete", &deleterefs, "delete refs"),
 		OPT_BOOLEAN( 0 , "tags", &tags, "push tags (can't be used with --all or --mirror)"),
 		OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
@@ -209,6 +224,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
+		die("--delete is incompatible with --all, --mirror and --tags");
+	if (deleterefs && argc < 2)
+		die("--delete doesn't make sense without any refs");
+
 	if (tags)
 		add_refspec("refs/tags/*");
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 516127b..0f04b2e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -547,6 +547,32 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'allow deleting a ref using --delete' '
+	mk_test heads/master &&
+	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
+	git push testrepo --delete master &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
+'
+
+test_expect_success 'allow deleting a tag using --delete' '
+	mk_test heads/master &&
+	git tag -a -m dummy_message deltag heads/master &&
+	git push testrepo --tags &&
+	(cd testrepo && git rev-parse --verify -q refs/tags/deltag) &&
+	git push testrepo --delete tag deltag &&
+	(cd testrepo && test_must_fail git rev-parse --verify refs/tags/deltag)
+'
+
+test_expect_success 'push --delete without args aborts' '
+	mk_test heads/master &&
+	test_must_fail git push testrepo --delete
+'
+
+test_expect_success 'push --delete refuses src:dest refspecs' '
+	mk_test heads/master &&
+	test_must_fail git push testrepo --delete master:foo
+'
+
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test heads/master
 	(cd testrepo &&
-- 
1.6.6.60.gc2ff1