Re: [PATCH v2] branch -D: allow - as abbreviation of '@{-1}'
On Tue, Mar 22, 2016 at 8:07 PM, Junio C Hamano wrote:
> Elena Petrashen writes:
>
>> +static int expand_dash_shortcut(const char **argv, int dash_position)
>> +{
>> + if (!strcmp(argv[dash_position], "-")){
>> + argv[dash_position] = "@{-1}";
>> + return 1;
>> + }
>> + return 0;
>> +}
>> int i;
>> int ret = 0;
>> + int dash_shortcut = 0;
>> int remote_branch = 0;
>> struct strbuf bname = STRBUF_INIT;
>>
>> @@ -213,7 +223,8 @@ static int delete_branches(int argc, const char **argv,
>> int force, int kinds,
>> for (i = 0; i < argc; i++, strbuf_release(&bname)) {
>> const char *target;
>> int flags = 0;
>> -
>> + if (expand_dash_shortcut (argv, i))
>> + dash_shortcut = 1;
>> strbuf_branchname(&bname, argv[i]);
>
> I think this code special cases "-" too much. Have you considered
> doing this without "dash_shortcut" variable? With that variable,
> your code says "there is no previous" when the user says "-", but
> isn't that message also appropriate when she says "@{-1}" on the
> command line? Furthermore, wouldn't the same apply to the case in
> which she said "@{-4}"?
>
> I suspect that you can check that condition immediately after
> calling expand-dash-shortcut and then strbuf-branchname, in other
> words, right here. And if there is not enough branch switches, you
> can say something like "you gave me @{-4} but you haven't made that
> many branch switches" and continue the loop.
>
> I _think_ strbuf_branchname() leaves "@{-}" when you do not have
> enough branch switches in the reflog, so perhaps
>
> strbuf_branchname(&bname, (!strcmp(argv[i], "-") ? "@{-1}" :
> argv[i]));
> if (starts_with(bname.buf, "@{-")) {
> ... say "you do not have enough branch switches" here.
> ... when adjusting the message to end-user input,
> ... you can look at argv[i] to notice that the original
> ... input was "-".
> error(...);
> continue;
> }
>
> or something?
>
> That way, there is no change necessary below this line, i.e. from
> here...
>
>> if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf))
>> {
>> error(_("Cannot delete the branch '%s' "
>> @@ -231,9 +242,12 @@ static int delete_branches(int argc, const char **argv,
>> int force, int kinds,
>> | RESOLVE_REF_ALLOW_BAD_NAME,
>> sha1, &flags);
>> if (!target) {
>> - error(remote_branch
>> - ? _("remote-tracking branch '%s' not found.")
>> - : _("branch '%s' not found."), bname.buf);
>> + error(dash_shortcut
>> + ? _("There is no previous branch that could be"
>> + " referred to at the moment.")
>> + : remote_branch
>> + ? _("remote-tracking branch '%s' not
>> found.")
>> + : _("branch '%s' not found."),
>> bname.buf);
>> ret = 1;
>> continue;
>> }
>> @@ -262,6 +276,10 @@ static int delete_branches(int argc, const char **argv,
>> int force, int kinds,
>> (flags & REF_ISBROKEN) ? "broken"
>> : (flags & REF_ISSYMREF) ? target
>> : find_unique_abbrev(sha1, DEFAULT_ABBREV));
Right, thank you for the idea and the detailed explanation! I will try
to implement the "not enough switches" message for the v3 version
of the patch the, along with the necessary style corrections that
Matthieu pointed out above.
>
> ... to here.
>
>
>> + if (dash_shortcut == 1)
>> +printf( _("\nIf that happened by mistake, you
>> may want to restore"
>> + " it with:\n\ngit branch %s %s\n"), bname.buf,
>> + find_unique_abbrev(sha1, DEFAULT_ABBREV));
>
> This change can be justified only if we believe that people who say
>
> $ git branch -D -
>
> by mistake are much less clueful than those who say
>
> $ git branch -D @{-1}
> $ git branch -D a-misspelled-branch-name
>
> by mistake and need extra help recovering. Is there an evidence to
> support such an assumption?
I'd think it's a little bit more likely to be the "I thought the
previous branch is "foo" but turns out it's "bar" which I didn't
mean to delete" case for -/@{-1}. case, then just misspelling.
The idea of the warning message was brought up because of
this I think? If we allow deleting via - or even @{-1}, which is
currently possible, it might make sense to additionally enable
the user to recover if she deleted the wron
Re: [PATCH v2] branch -D: allow - as abbreviation of '@{-1}'
On Tue, Mar 22, 2016 at 1:12 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>> I _think_ strbuf_branchname() leaves "@{-}" when you do not have
>> enough branch switches in the reflog, so perhaps ...
>
> This is a tangent, but the value returned from strbuf_branchname()
> is not well documented, and nobody looks at it. It might not be a
> bad idea to change the return value from the function in such a way
> that it can signal "I saw @{-} syntax but there weren't enough
> branch switches" and also "I saw branch@{upstream} syntax but that
> branch does not have an upstream configured for it" to its callers.
>
> That way, the suggested "perhaps like this..." patch does not have
> to do an ugly hack: if (starts_with(bname.buf, "@{-")).
In case it's not clear, when Junio says "this is a tangent", he is not
saying that he expects you to make this change as part of your
microproject. Rather, he's just putting it out there as "low hanging
fruit" if someone wants a small project to tackle (and indeed, it
might make a reasonable microproject itself).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] branch -D: allow - as abbreviation of '@{-1}'
Junio C Hamano writes:
> I _think_ strbuf_branchname() leaves "@{-}" when you do not have
> enough branch switches in the reflog, so perhaps ...
This is a tangent, but the value returned from strbuf_branchname()
is not well documented, and nobody looks at it. It might not be a
bad idea to change the return value from the function in such a way
that it can signal "I saw @{-} syntax but there weren't enough
branch switches" and also "I saw branch@{upstream} syntax but that
branch does not have an upstream configured for it" to its callers.
That way, the suggested "perhaps like this..." patch does not have
to do an ugly hack: if (starts_with(bname.buf, "@{-")).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] branch -D: allow - as abbreviation of '@{-1}'
Elena Petrashen writes:
> +static int expand_dash_shortcut(const char **argv, int dash_position)
> +{
> + if (!strcmp(argv[dash_position], "-")){
> + argv[dash_position] = "@{-1}";
> + return 1;
> + }
> + return 0;
> +}
> int i;
> int ret = 0;
> + int dash_shortcut = 0;
> int remote_branch = 0;
> struct strbuf bname = STRBUF_INIT;
>
> @@ -213,7 +223,8 @@ static int delete_branches(int argc, const char **argv,
> int force, int kinds,
> for (i = 0; i < argc; i++, strbuf_release(&bname)) {
> const char *target;
> int flags = 0;
> -
> + if (expand_dash_shortcut (argv, i))
> + dash_shortcut = 1;
> strbuf_branchname(&bname, argv[i]);
I think this code special cases "-" too much. Have you considered
doing this without "dash_shortcut" variable? With that variable,
your code says "there is no previous" when the user says "-", but
isn't that message also appropriate when she says "@{-1}" on the
command line? Furthermore, wouldn't the same apply to the case in
which she said "@{-4}"?
I suspect that you can check that condition immediately after
calling expand-dash-shortcut and then strbuf-branchname, in other
words, right here. And if there is not enough branch switches, you
can say something like "you gave me @{-4} but you haven't made that
many branch switches" and continue the loop.
I _think_ strbuf_branchname() leaves "@{-}" when you do not have
enough branch switches in the reflog, so perhaps
strbuf_branchname(&bname, (!strcmp(argv[i], "-") ? "@{-1}" : argv[i]));
if (starts_with(bname.buf, "@{-")) {
... say "you do not have enough branch switches" here.
... when adjusting the message to end-user input,
... you can look at argv[i] to notice that the original
... input was "-".
error(...);
continue;
}
or something?
That way, there is no change necessary below this line, i.e. from
here...
> if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> error(_("Cannot delete the branch '%s' "
> @@ -231,9 +242,12 @@ static int delete_branches(int argc, const char **argv,
> int force, int kinds,
> | RESOLVE_REF_ALLOW_BAD_NAME,
> sha1, &flags);
> if (!target) {
> - error(remote_branch
> - ? _("remote-tracking branch '%s' not found.")
> - : _("branch '%s' not found."), bname.buf);
> + error(dash_shortcut
> + ? _("There is no previous branch that could be"
> + " referred to at the moment.")
> + : remote_branch
> + ? _("remote-tracking branch '%s' not
> found.")
> + : _("branch '%s' not found."),
> bname.buf);
> ret = 1;
> continue;
> }
> @@ -262,6 +276,10 @@ static int delete_branches(int argc, const char **argv,
> int force, int kinds,
> (flags & REF_ISBROKEN) ? "broken"
> : (flags & REF_ISSYMREF) ? target
> : find_unique_abbrev(sha1, DEFAULT_ABBREV));
... to here.
> + if (dash_shortcut == 1)
> +printf( _("\nIf that happened by mistake, you
> may want to restore"
> + " it with:\n\ngit branch %s %s\n"), bname.buf,
> + find_unique_abbrev(sha1, DEFAULT_ABBREV));
This change can be justified only if we believe that people who say
$ git branch -D -
by mistake are much less clueful than those who say
$ git branch -D @{-1}
$ git branch -D a-misspelled-branch-name
by mistake and need extra help recovering. Is there an evidence to
support such an assumption?
I would actually understand it if this were more like
if (advice_mistaken_branch_deletion)
printf(_("If you deleted the branch by mistake, you can..."));
so that everybody who ran "git branch -D" on a (wrong) branch by
mistake can get the extra help.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] branch -D: allow - as abbreviation of '@{-1}'
Hi, and thanks for the quick reroll.
A small advice: don't hesitate to reply to reviewers in the thread for
v1. As a reviewer, I appreciate a quick reply like "OK, will do in v2",
or even "I disagree, I think my version is better because: ..." (I'm not
_always_ right ;-) ).
Elena Petrashen writes:
> Signed-off-by: Elena Petrashen
> ---
> This micro-patch is meant to allow “-“ as a short-hand for
> “@{-1} for branch -D (Cf. $gmane/230828):
>
> * based on the discussion on the previous version of the patch,
> added the advice on how to restore the deleted branch using
> git branch deleted_name sha1 - to ensure safety and
> newbie-friendliness
>
> * git branch (-d | -D) is not supposed to accept any other
> arguments except for branch name so it makes sense to replace
> the argv[i] with @{-1}. We will not lose the opportunity to
> use it for something different for other git branch uses if
> we will decide it’s required.
As much as possible, try to distinguish new things in this version and
general remarks on your patch. Here, the first point is new and could
appear below "Changes since v1". The second is not new, I think you can
either consider that it is a followup to previous discussion (as Eric
interpreted it in v1) and remove the comment from v2, or consider it as
a justification of why your patch is sensible and include it in the
commit message (what I suggested in v1, but Eric more or less convinced
me that I was wrong).
> * if there’s no previous branch in the repository yet, a
> specific warning message is given
As I said in v1, I think this is a separate topic and should be moved to
a separate patch. As I said above, you are free to disagree (and it's
not terribly important), but avoid leaving discussions open.
One point in favor of splitting the patch is that it's a good exercice
for a newcommer ;-).
> + if (!strcmp(argv[dash_position], "-")){
Style: space between ) and {.
> @@ -213,7 +223,8 @@ static int delete_branches(int argc, const char **argv,
> int force, int kinds,
> for (i = 0; i < argc; i++, strbuf_release(&bname)) {
> const char *target;
> int flags = 0;
> -
> + if (expand_dash_shortcut (argv, i))
> + dash_shortcut = 1;
Nit: the blank line removal is not needed.
> + if (dash_shortcut == 1)
> +printf( _("\nIf that happened by mistake, you
> may want to restore"
Style: no space after (.
> + " it with:\n\ngit branch %s %s\n"), bname.buf,
> + find_unique_abbrev(sha1, DEFAULT_ABBREV));
I think the message is overly long (4 '\n', and I think "If that
happened by mistake" is not needed, just "to restore the branch,
run ..." would be enough).
A rule that is usually applied for messages with Git:
* If the message is triggered by a user-error, then it's OK to have a
long and possibly scary message: advanced users are not supposed to
see it.
* If the message is seen by normal users in daily usage, then
- we try to keep it as short as possible.
- if it's not short enough, we allow disabling it with an advice.*
configuration variable.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
