builtin-push: add --delete as syntactic sugar for :foo

RFC PATCH In review Jan Krüger 4 participants 11 messages Jun 15, 2016 Documentation

Share & tools

Jan Krüger

author

2016-06-15 22:47 UTC #0 lore raw

RFC PATCH 1/1 builtin-push: add --delete as syntactic sugar for :foo

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 <[email protected]>

---
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(-)

builtin-push.c +15 -0

index 8631c06..4ae9166 100644
@@ -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/*");

t/t5516-fetch-push.sh +6 -0

index 6889a53..aa1450a 100755
@@ -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 &&

Signature · 1 line

1.6.5.2.155.gbb47.dirty

Junio C Hamano

2016-06-15 22:47 UTC #1 lore raw

Quoted 7 lines · 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.

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.

Jan Krüger

author

2016-06-15 22:47 UTC #2 lore raw

RFC PATCH v2 1/1 builtin-push: add --delete as syntactic sugar for :foo

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 <[email protected]>

---
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(-)

builtin-push.c +16 -0

index 8631c06..e40bfe8 100644
@@ -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/*");

t/t5516-fetch-push.sh +6 -0

index 6889a53..aa1450a 100755
@@ -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 &&

Signature · 1 line

1.6.5.2.155.gbb47.dirty

Nanako Shiraishi

2016-06-15 22:47 UTC #3 lore raw

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.

Junio C Hamano

2016-06-15 22:47 UTC #4 lore raw

Quoted 6 lines · 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.

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.

Jan Krüger

author

2016-06-15 22:47 UTC #5 lore raw

PATCH v3 1/1 builtin-push: add --delete as syntactic sugar for :foo

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 <[email protected]>

---

Quoted 6 lines · Junio C Hamano

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(-)

Documentation/git-push.txt +4 -0

index 52c0538..e3eb1e8 100644
@@ -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

builtin-push.c +23 -3

index dcfb53f..f7661d2 100644
@@ -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/*");

t/t5516-fetch-push.sh +16 -0

index 516127b..a17666c 100755
@@ -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 &&

Signature · 1 line

1.6.6.60.gc2ff1

Junio C Hamano

2016-06-15 22:47 UTC #6 lore raw

Quoted 6 lines · 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

Quoted 6 lines

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 <[email protected]>
---

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.

Jan Krüger

author

2016-06-15 22:47 UTC #7 lore raw

PATCH v4 1/1 builtin-push: add --delete as syntactic sugar for :foo

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 <[email protected]>

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

Quoted 3 lines · Junio C Hamano

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.

Documentation/git-push.txt +4 -0

index 52c0538..e3eb1e8 100644
@@ -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

builtin-push.c +23 -3

index dcfb53f..f7661d2 100644
@@ -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/*");

t/t5516-fetch-push.sh +26 -0

index 516127b..0f04b2e 100755
@@ -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 &&

Signature · 1 line

1.6.6.60.gc2ff1

Sverre Rabbelier

2016-06-15 22:47 UTC #8 lore raw

In reply to Jan Krüger

Heya,

Quoted 5 lines · Jan Krüger

Quoted 1 line
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.

Signature · 3 lines

Cheers,

Sverre Rabbelier

Junio C Hamano

2016-06-15 22:47 UTC #9 lore raw

In reply to Junio C Hamano

Quoted 4 lines · Junio C Hamano

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.

Sverre Rabbelier

2016-06-15 22:47 UTC #10 lore raw

In reply to Junio C Hamano

Heya,

Quoted 8 lines · Junio C Hamano

Quoted 2 lines · Jan Krüger
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

Signature · 3 lines

Cheers,

Sverre Rabbelier