This is in reply to a brief conversation I had with jnareb on #git
about the blogpost "5 things git could learn from hg", or something
like that (can't find it right now).
Comments most welcome.
Sverre Rabbelier (2):
add a --delete option to git push
test that git push --delete deletes the remote ref
Documentation/git-push.txt | 8 +++++++-
builtin-push.c | 10 +++++++++-
t/t5516-fetch-push.sh | 6 ++++++
3 files changed, 22 insertions(+), 2 deletions(-)
Those new to git have a hard time learning how to delete a remote
ref. This makes it easier for them to find out how by providing a
flag to git push.
Signed-off-by: Sverre Rabbelier <redacted>
---
Currently `git push --delete master:master` results in a somewhat
cryptic error message. It seems unlikely however, that those new
to git would use the 'old:new' notation, so I haven't bothered
guarding against it and settled for documenting it.
Documentation/git-push.txt | 8 +++++++-
builtin-push.c | 10 +++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
@@ -137,6 +137,12 @@ useful if you write an alias or script around 'git-push'. --verbose:: Run verbosely.+-d::+--delete::+ Delete the specified refs. Prefixes all refs with ':' to tell the+ push machinery to delete the specified ref. As such, the refs+ that are to be deleted should not contain a ':' specifier.+ include::urls-remotes.txt[] OUTPUT
Signed-off-by: Sverre Rabbelier <redacted>
---
Not a lot of tests yet, but I suspect reviewers will ask for more
and/or new features will pop up that need testing, hence a
seperate patch with tests.
t/t5516-fetch-push.sh | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:47:14
On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
Those new to git have a hard time learning how to delete a remote
ref. This makes it easier for them to find out how by providing a
flag to git push.
Should this also impact refs read from the config? E.g., if I do "git
push --delete" will it try to impact matching refs (which is almost
certainly a mistake), or refs I have in my remote.$X.push (which is
probably also a mistake)?
From reading your patch, it looks like it just touches the command-line.
I think that's the right thing to do, but I think it makes sense to
think half a second to make sure.
And with the way you have it, "git push --delete" will silently ignore
the --delete and push configured refspecs. Probably it should say
"--delete is useless without refspecs on the command line".
Currently `git push --delete master:master` results in a somewhat
cryptic error message. It seems unlikely however, that those new
to git would use the 'old:new' notation, so I haven't bothered
guarding against it and settled for documenting it.
It seems like it would be simple enough to just check whether the
refspec contains a colon; if so, silently leave it alone. That could
also protect configured refspecs, as mentioned above, but I wouldn't
rule out somebody have a single-name refspec in their config (in fact, I
think "remote.$X.push = HEAD" is reasonable -- should that delete the
HEAD on "git push --delete"?).
+--delete::
+ Delete the specified refs. Prefixes all refs with ':' to tell the
+ push machinery to delete the specified ref. As such, the refs
+ that are to be deleted should not contain a ':' specifier.
+
This impacts _all_ refspecs. Remember that we can have multiple refspecs
on the command-line. So I can "move" a ref remotely with:
git push :old-name old-name:new-name
but I can't do:
git push --delete old-name old-name:new-name
Ignoring colon-less refspecs would make that work, but would not allow
the similar:
# branch has already been renamed to new-name locally
git push --delete old-name new-name
So maybe it would make more sense for it to be "--delete <ref>" and
impact only a single ref. The simple case of "git push --delete foo"
would remain unchanged.
The counter-argument is that "--delete" does not necessarily need to be
as powerful as the ":ref" syntax, but I don't see the downside in making
it so.
-Peff
Heya,
On Thu, Aug 13, 2009 at 22:21, Jeff King[off-list ref] wrote:
On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
From reading your patch, it looks like it just touches the command-line.
I think that's the right thing to do, but I think it makes sense to
think half a second to make sure.
Indeed, the reason I sent out this RFC was to gather more opinions :).
And with the way you have it, "git push --delete" will silently ignore
the --delete and push configured refspecs. Probably it should say
"--delete is useless without refspecs on the command line".
This makes sense, I will fix that.
quoted
Currently `git push --delete master:master` results in a somewhat
cryptic error message. It seems unlikely however, that those new
to git would use the 'old:new' notation, so I haven't bothered
guarding against it and settled for documenting it.
It seems like it would be simple enough to just check whether the
refspec contains a colon; if so, silently leave it alone. That could
also protect configured refspecs, as mentioned above, but I wouldn't
rule out somebody have a single-name refspec in their config (in fact, I
think "remote.$X.push = HEAD" is reasonable -- should that delete the
HEAD on "git push --delete"?).
I don't think we should touch any configured refspecs, think about how
often one would use that vs. the inconvenience of doing so
unintentionally.
quoted
+--delete::
+ Delete the specified refs. Prefixes all refs with ':' to tell the
+ push machinery to delete the specified ref. As such, the refs
+ that are to be deleted should not contain a ':' specifier.
+
This impacts _all_ refspecs. Remember that we can have multiple refspecs
on the command-line. So I can "move" a ref remotely with:
Correct, hence the plural 'refs'.
git push :old-name old-name:new-name
but I can't do:
git push --delete old-name old-name:new-name
I don't think that's the use case for this option, it is mostly for
new users who do not know about the colon notation; now you do raise a
valid point that we might want to add a 'git push --rename old new' at
some point, but I think that's beyond the scope of this patch.
So maybe it would make more sense for it to be "--delete <ref>" and
impact only a single ref. The simple case of "git push --delete foo"
would remain unchanged.
I thought about that, but I decided that it was both intuitive and
convenient to be able to delete multiple refs this way.
The counter-argument is that "--delete" does not necessarily need to be
as powerful as the ":ref" syntax, but I don't see the downside in making
it so.
I do, it's easy to make mistakes when it's more powerful, and I think
less intuitive. I think we want this to be as intuitive as possible.
I'm not very opinionated over any of this, if you have strong feelings
yourself please let me know and I'll change the patch.
--
Cheers,
Sverre Rabbelier
From: Jeff King <hidden> Date: 2016-06-15 22:47:14
On Thu, Aug 13, 2009 at 11:24:05PM -0700, Sverre Rabbelier wrote:
quoted
It seems like it would be simple enough to just check whether the
refspec contains a colon; if so, silently leave it alone. That could
also protect configured refspecs, as mentioned above, but I wouldn't
rule out somebody have a single-name refspec in their config (in fact, I
think "remote.$X.push = HEAD" is reasonable -- should that delete the
HEAD on "git push --delete"?).
I don't think we should touch any configured refspecs, think about how
often one would use that vs. the inconvenience of doing so
unintentionally.
I think you are right. My previous message was sort of thinking out
loud, but I think on the whole, the annoyance caused by accidental
deletion is not worth it. :)
quoted
So maybe it would make more sense for it to be "--delete <ref>" and
impact only a single ref. The simple case of "git push --delete foo"
would remain unchanged.
I thought about that, but I decided that it was both intuitive and
convenient to be able to delete multiple refs this way.
[...]
I do, it's easy to make mistakes when it's more powerful, and I think
less intuitive. I think we want this to be as intuitive as possible.
I guess I find what you are doing _more_ complex, because you are really
introducing a whole new mode to push, which is "I am deleting some
stuff". As opposed to some syntactic sugar to replace the confusing
":ref" syntax, which is what I thought the goal was.
On the other hand, "--delete <ref>" introduces its own syntactic
problems. Is it an option, in which case you end up doing:
git push --delete master origin
which is a bit backwards from the usual syntax. Or is it part of the
refspec list, in which case:
1. We have just disallowed a refspec called "--delete" (though to be
fair, you have to be a little insane to use that anyway, and you
can always call it refs/heads/--delete)).
2. Now we don't simply have a list, one refspec per element, which
makes things syntactically a little more complex.
Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
a reasonable way of adding just the syntactic sugar. I.e.:
git push origin --delete=master
Of course, maybe the goal of a "delete mode" is useful to people. I
can't think of a time when I would have used it, but then I also tend to
think ":<ref>" is elegant and obvious. ;)
I dunno. I don't feel too strongly about it; mainly I was just surprised
because I would have done it the other way. :)
-Peff
Heya,
On Thu, Aug 13, 2009 at 23:33, Jeff King[off-list ref] wrote:
On Thu, Aug 13, 2009 at 11:24:05PM -0700, Sverre Rabbelier wrote:
quoted
I don't think we should touch any configured refspecs, think about how
often one would use that vs. the inconvenience of doing so
unintentionally.
I think you are right. My previous message was sort of thinking out
loud, but I think on the whole, the annoyance caused by accidental
deletion is not worth it. :)
Thinking out loud is good :).
I guess I find what you are doing _more_ complex, because you are really
introducing a whole new mode to push, which is "I am deleting some
stuff". As opposed to some syntactic sugar to replace the confusing
":ref" syntax, which is what I thought the goal was.
Yes, replacing the confusing ":ref" syntax was the goal, but I think
the current solution does that as well.
On the other hand, "--delete <ref>" introduces its own syntactic
problems. [...]
It does indeed, and I don't think that's the way to go.
Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
a reasonable way of adding just the syntactic sugar. [...]
That would work too I guess, although it would be technically more difficult.
Of course, maybe the goal of a "delete mode" is useful to people. I
can't think of a time when I would have used it, but then I also tend to
think ":<ref>" is elegant and obvious. ;)
I don't think it's that confusing either, but it's hard to stumble
upon, yes? When you're looking at the man page for git push it is
easier to deduct that '--delete' is what you need, than ':master'.
--
Cheers,
Sverre Rabbelier
From: Jeff King <hidden> Date: 2016-06-15 22:47:14
On Thu, Aug 13, 2009 at 11:40:44PM -0700, Sverre Rabbelier wrote:
quoted
On the other hand, "--delete <ref>" introduces its own syntactic
problems. [...]
It does indeed, and I don't think that's the way to go.
Hmm. Actually, looking at the code, we _already_ have a funny
two-element syntax:
git push origin tag v1.6.1
which, AFAICT, is totally useless, as you can just push v1.6.1 directly.
I assume it's historical. I still think it's probably not a good idea to
introduce a similar "delete foo".
quoted
Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
a reasonable way of adding just the syntactic sugar. [...]
That would work too I guess, although it would be technically more difficult.
which is even shorter than your patch, not needing a separate option
parser.
That being said, currently parseopt will complain about that, even after
the "remote" field; we would need to pass it
PARSE_OPT_STOP_AT_NON_OPTION.
I don't think it's that confusing either, but it's hard to stumble
upon, yes? When you're looking at the man page for git push it is
easier to deduct that '--delete' is what you need, than ':master'.
I think you mean "deduce", but yes, I think we have seen people complain
about the syntax in the past. I'm not against fixing it; I just want to
make sure what we introduce doesn't make any new confusion.
-Peff
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:14
On Fri, 14 August 2009, Sverre Rabbelier wrote:
This is in reply to a brief conversation I had with jnareb on #git
about the blogpost "5 things git could learn from hg", or something
like that (can't find it right now).
Thanks.
It was "Five Features from Mercurial That Would Make Git Suck Less"
http://nubyonrails.com/articles/five-features-from-mercurial-that-would-make-git-suck-less
Those five features were:
1. "<scm> init <directory>" (done)
2. "hg commit --close-branch" vs slightly cryptic
"git push origin :refs/heads/feature-tweak", which can be written
simply as "git push origin :feature-tweak" I think.
(that is what this patch series is about)
3. Numeric local references, e.g. 18:a432bc and "hg checkout 18"...
but more realistic example would be "hg checkout 6324" :-P
4. sensible defaults: meaning of revert, staging area (i.e. commit -a)
5. "hg serve" (gitweb and a kind of git-daemon equivalent)
See the alleged blog for details (I call it 'alleged blog' because real
blog has comments which work, and are shown soon after posting them).
--
Jakub Narebski
Poland