Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-06-02 Thread Jeff King 
On Sat, Jun 02, 2018 at 06:46:31AM +0200, Duy Nguyen wrote:

> > if (used_deprecated_reflog_option) {
> > -   warning("the '-l' alias for '--create-reflog' is 
> > deprecated;");
> > -   warning("it will be removed in a future version of Git");
> > +   if (list) {
> > +   warning("the '-l' option is an alias for 
> > '--create-reflog' and");
> > +   warning("has no effect in list mode. This option 
> > will soon be");
> > +   warning("removed and you should omit it (or use 
> > '--list' instead).");
> > +   } else {
> > +   warning("the '-l' alias for '--create-reflog' is 
> > deprecated;");
> > +   warning("it will be removed in a future version of 
> > Git");
> > +   }
> 
> This is already in 'next', but could you do a follow up patch to mark
> these strings for translation? While at there, concatenating them into
> full sentences would also help translators.

Already being discussed elsewhere on the thread; we're hoping for a
warning() that will auto-prefix each one.

That said, I think I'm going to re-roll this in the direction discussed
elsewhere in the thread (skipping the warning for list-mode).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-06-01 Thread Duy Nguyen 
On Fri, May 25, 2018 at 4:40 AM, Jeff King  wrote:
> -- >8 --
> Subject: [PATCH] branch: customize "-l" warning in list mode
>
> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
>
> Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.
>
> Let's make it clear that:
>
>   1. No, "-l" didn't do what they thought here.
>
>   2. It's going away, and what they should do instead.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/branch.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
> setup_auto_pager("branch", 1);
>
> if (used_deprecated_reflog_option) {
> -   warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> -   warning("it will be removed in a future version of Git");
> +   if (list) {
> +   warning("the '-l' option is an alias for 
> '--create-reflog' and");
> +   warning("has no effect in list mode. This option will 
> soon be");
> +   warning("removed and you should omit it (or use 
> '--list' instead).");
> +   } else {
> +   warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> +   warning("it will be removed in a future version of 
> Git");
> +   }

This is already in 'next', but could you do a follow up patch to mark
these strings for translation? While at there, concatenating them into
full sentences would also help translators.

> }
>
> if (delete) {
> --
> 2.17.0.1391.g6fdbf40724
>



-- 
Duy

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-31 Thread Junio C Hamano 
Jeff King  writes:

> So I think you're proposing:
>
>   - step 0: warn about "-l" when it actually gets used, and otherwise
> continue ignoring
>
>   - step 1: turn "-l" into "--list"
>
>   - step 2: there is no step 2
>
> ... So I
> guess the right rule is to warn when we are not in list-mode, and
> otherwise quietly accept it.
>
> That does mean that anybody who misses the deprecation warning may be
> surprised when "branch -l foo" starts listing instead of creating a
> branch with a reflog (whereas in the current 3-step plan, we have a
> period in the middle where that's a hard error). That may be OK, though,
> and is a natural consequence of getting to the end step sooner (even
> with a 3-step plan, anybody who skips the versions in the middle _could_
> be surprised).

Thanks for a concise and readably summary ;-)

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-30 Thread Kaartic.Sivaraam 

On Wednesday 30 May 2018 08:22 AM, Junio C Hamano wrote:

Jeff King  writes:


Right, what I meant by "gentler" is that we continue to perform the same
behavior as the old version, alongside the warning. It's arguable here
because running "git branch -l" has _always_ been wrong. It's just wrong
in a way that happens to do what the user wants. ;)
...

Anyways, if you think it mustn't turn into an error now and only in the
next stage, a suggestion follows in another thread.


I don't think "mustn't", but I have a slight preference for what I
posted, as I think it is a little friendlier during the transition (at
the risk of somebody missing the warning, but then step 2 turns it into
a hard error anyway, so they'll certainly find out then).


Well, we could keep treating '-l' given in contexts where we have
silently ignored the option and did "list" instead as before during
the transition, until the very end where it becomes an explicit
"list" command, no?  Then there is no need to even warn against '-l'
that is ignored because we are listing in the earliest step.  The
only usage that requires a warning then becomes '-l' used for its
original meaning to create a reflog, right?  That sounds gentler to
me.



That sounds interesting. I guess that would avoid the confusion I was 
speaking of from the beginning of this thread as the warning message 
would not be shown at all for "git branch -l". Of course, it would then 
confuse people who discover that "git branch -l" lists and "git branch 
-l $prefix" creates a branch with name "$prefix" (if it's valid) instead 
of listing branch names with prefix "$prefix". So, it might be worth 
considering.


BTW, I suspect this would make the deprecation of "-l" a little unsual 
as the no. of people who see the deprecation warning would be less as 
the warning is shown only for "git branch -l $branch" and I also suspect 
the no. of users of that command would be very less as previously 
pointed by someone elsewhere.


--
Sivaraam

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-30 Thread Jeff King 
On Wed, May 30, 2018 at 11:52:19AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Right, what I meant by "gentler" is that we continue to perform the same
> > behavior as the old version, alongside the warning. It's arguable here
> > because running "git branch -l" has _always_ been wrong. It's just wrong
> > in a way that happens to do what the user wants. ;)
> > ...
> >> Anyways, if you think it mustn't turn into an error now and only in the
> >> next stage, a suggestion follows in another thread.
> >
> > I don't think "mustn't", but I have a slight preference for what I
> > posted, as I think it is a little friendlier during the transition (at
> > the risk of somebody missing the warning, but then step 2 turns it into
> > a hard error anyway, so they'll certainly find out then).
> 
> Well, we could keep treating '-l' given in contexts where we have
> silently ignored the option and did "list" instead as before during
> the transition, until the very end where it becomes an explicit
> "list" command, no?  Then there is no need to even warn against '-l'
> that is ignored because we are listing in the earliest step.  The
> only usage that requires a warning then becomes '-l' used for its
> original meaning to create a reflog, right?  That sounds gentler to
> me.

So I think you're proposing:

  - step 0: warn about "-l" when it actually gets used, and otherwise
continue ignoring

  - step 1: turn "-l" into "--list"

  - step 2: there is no step 2

I can't think of any reason that would work, and it lets us reclaim it
for "--list" sooner. I guess "when it gets used" is maybe not the right
criterion. We'd warn on:

  git branch -l foo

But we wouldn't on:

  git branch -d -l foo

That's clearly nonsense, but we're probably better off complaining. So I
guess the right rule is to warn when we are not in list-mode, and
otherwise quietly accept it.

That does mean that anybody who misses the deprecation warning may be
surprised when "branch -l foo" starts listing instead of creating a
branch with a reflog (whereas in the current 3-step plan, we have a
period in the middle where that's a hard error). That may be OK, though,
and is a natural consequence of getting to the end step sooner (even
with a 3-step plan, anybody who skips the versions in the middle _could_
be surprised).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-30 Thread Jeff King 
On Wed, May 30, 2018 at 11:48:32AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> -  if (list) {
> >> -  warning("the '-l' option is an alias for 
> >> '--create-reflog' and");
> >> -  warning("has no effect in list mode. This option will 
> >> soon be");
> >> -  warning("removed and you should omit it (or use 
> >> '--list' instead).");
> >> -  } else {
> >> -  warning("the '-l' alias for '--create-reflog' is 
> >> deprecated;");
> >> -  warning("it will be removed in a future version of 
> >> Git");
> >> -  }
> >> -  }
> >> -
> >
> > Oh, and did we want to mark these for translation on the step 0 branch?
> > Obviously that would impact this hunk.
> 
> I was hoping that we can settle the "multi-line message translation
> that can potentially result in different number of lines" issue
> before we have to mark the above for translation ;-)

Yeah, right after saying that I realized it would create horrible
translation-lego. I agree we should punt for now.

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-29 Thread Junio C Hamano 
Jeff King  writes:

> Right, what I meant by "gentler" is that we continue to perform the same
> behavior as the old version, alongside the warning. It's arguable here
> because running "git branch -l" has _always_ been wrong. It's just wrong
> in a way that happens to do what the user wants. ;)
> ...
>> Anyways, if you think it mustn't turn into an error now and only in the
>> next stage, a suggestion follows in another thread.
>
> I don't think "mustn't", but I have a slight preference for what I
> posted, as I think it is a little friendlier during the transition (at
> the risk of somebody missing the warning, but then step 2 turns it into
> a hard error anyway, so they'll certainly find out then).

Well, we could keep treating '-l' given in contexts where we have
silently ignored the option and did "list" instead as before during
the transition, until the very end where it becomes an explicit
"list" command, no?  Then there is no need to even warn against '-l'
that is ignored because we are listing in the earliest step.  The
only usage that requires a warning then becomes '-l' used for its
original meaning to create a reflog, right?  That sounds gentler to
me.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-29 Thread Junio C Hamano 
Jeff King  writes:

>> -if (list) {
>> -warning("the '-l' option is an alias for 
>> '--create-reflog' and");
>> -warning("has no effect in list mode. This option will 
>> soon be");
>> -warning("removed and you should omit it (or use 
>> '--list' instead).");
>> -} else {
>> -warning("the '-l' alias for '--create-reflog' is 
>> deprecated;");
>> -warning("it will be removed in a future version of 
>> Git");
>> -}
>> -}
>> -
>
> Oh, and did we want to mark these for translation on the step 0 branch?
> Obviously that would impact this hunk.

I was hoping that we can settle the "multi-line message translation
that can potentially result in different number of lines" issue
before we have to mark the above for translation ;-)

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-29 Thread Jeff King 
On Tue, May 29, 2018 at 05:20:29PM -0400, Jeff King wrote:

> Thanks. There's one bit missing here, because it did not cause a textual
> conflict during the rebase (but it's now dead code). Patch below (to be
> squashed to the tip of jk/branch-l-1-removal).
> [...]
> - if (used_deprecated_reflog_option) {
> - if (list) {
> - warning("the '-l' option is an alias for 
> '--create-reflog' and");
> - warning("has no effect in list mode. This option will 
> soon be");
> - warning("removed and you should omit it (or use 
> '--list' instead).");
> - } else {
> - warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> - warning("it will be removed in a future version of 
> Git");
> - }
> - }
> -

Oh, and did we want to mark these for translation on the step 0 branch?
Obviously that would impact this hunk.

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-29 Thread Jeff King 
On Sat, May 26, 2018 at 11:32:35AM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Yup, thanks for being extra explicit.  I do imagine there are quite
> > a few of us who would be puzzled without this update (but with the
> > previous one to unhide it from behind the pager).
> 
> With these two patches queued on top of jk/branch-l-0-deprecation,
> the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
> to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
> a synonym to 'branch --list' needs rebasing.  Both are trivial and
> hopefully I did them correctly.
> -- >8 --
> From: Jeff King 
> Date: Mon, 26 Mar 2018 03:29:22 -0400
> Subject: [PATCH] branch: drop deprecated "-l" option
> 
> We marked the "-l" option as deprecated back in  here>. Now that sufficient time has passed, let's follow
> through and get rid of it.

Thanks. There's one bit missing here, because it did not cause a textual
conflict during the rebase (but it's now dead code). Patch below (to be
squashed to the tip of jk/branch-l-1-removal).

We may also want to fill in . I think it's afc968e579
(branch: deprecate "-l" option, 2018-03-26), which should be stable at
this point (it's already merged to 'next').

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..f7cd333587 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -34,7 +34,6 @@ static const char * const builtin_branch_usage[] = {
NULL
 };
 
-static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 
@@ -686,17 +685,6 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (list)
setup_auto_pager("branch", 1);
 
-   if (used_deprecated_reflog_option) {
-   if (list) {
-   warning("the '-l' option is an alias for 
'--create-reflog' and");
-   warning("has no effect in list mode. This option will 
soon be");
-   warning("removed and you should omit it (or use 
'--list' instead).");
-   } else {
-   warning("the '-l' alias for '--create-reflog' is 
deprecated;");
-   warning("it will be removed in a future version of 
Git");
-   }
-   }
-
if (delete) {
if (!argc)
die(_("branch name required"));

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-29 Thread Jeff King 
On Sun, May 27, 2018 at 12:15:40AM +0530, Kaartic Sivaraam wrote:

> > Hmm, actually, I suppose the true value of the warning is to help people
> > doing "git branch -l foo", and it would still work there. The "more
> > extreme" from your suggested patch would only affect "branch -l".
> > 
> > Still, I think I prefer the gentler version that we get by keeping it as
> > a warning even in the latter case.
> >
> 
> I never wanted to suppress the warning message in the latter case. I
> just wanted to avoid listing the branches. Actually the patch I sent in
> one the previous threads[1] that avoids listing the branches has the
> following behaviour,
> 
> $ git branch -l
> warning: the '-l' alias for '--create-reflog' is deprecated;
> warning: it will be removed in a future version of Git
> usage: git branch [] [-r | -a] [--merged | --no-merged]
>or: git branch [] [-l] [-f]  []
>or: git branch [] [-r] (-d | -D) ...
>or: git branch [] (-m | -M) [] 
>or: git branch [] (-c | -C) [] 
>or: git branch [] [-r | -a] [--points-at]
>or: git branch [] [-r | -a] [--format]
> 
> 
> So, the warning message isn't lost. It just prevents the listing of
> branches.

Right, what I meant by "gentler" is that we continue to perform the same
behavior as the old version, alongside the warning. It's arguable here
because running "git branch -l" has _always_ been wrong. It's just wrong
in a way that happens to do what the user wants. ;)

> Wait, maybe I'm misunderstanding what you mean by "warning". You're
> probably meaning something related to the way Git exits in both cases.
> With your patch "git branch -l" prints a warning, lists the branches and
> has an exit status of 0. With my patch it prints the warning, the usage
> specifications with exit status 128. In that case, I still don't think
> it's bad to turn "git branch -l" into an error now as it's been
> incorrect for a long time now and it's not wrong if we correct it now.
> 
> Anyways, if you think it mustn't turn into an error now and only in the
> next stage, a suggestion follows in another thread.

I don't think "mustn't", but I have a slight preference for what I
posted, as I think it is a little friendlier during the transition (at
the risk of somebody missing the warning, but then step 2 turns it into
a hard error anyway, so they'll certainly find out then).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-26 Thread Kaartic Sivaraam 
On Friday 25 May 2018 08:10 AM, Jeff King wrote:
> Subject: [PATCH] branch: customize "-l" warning in list mode
> 
> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
> > Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> 
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.
> 

So, this patch is to improve the user experience of people who use "git
branch -l" for listing and not for the people who forget to give a new
branch name argument for "-l". In that case, this makes sense.

BTW, I hope people don't start wondering why "git branch -d" doesn't
trigger list mode ;-)


> + warning("the '-l' option is an alias for 
> '--create-reflog' and");
> + warning("has no effect in list mode. This option will 
> soon be");
> + warning("removed and you should omit it (or use 
> '--list' instead).");

I suppose s/alias/deprecated alias/ makes the point that '-l' will be
removed more stronger.


-- 
Sivaraam

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-26 Thread Kaartic Sivaraam 
On Friday 25 May 2018 01:01 AM, Jeff King wrote:
> On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote:
> 
> Hmm, actually, I suppose the true value of the warning is to help people
> doing "git branch -l foo", and it would still work there. The "more
> extreme" from your suggested patch would only affect "branch -l".
> 
> Still, I think I prefer the gentler version that we get by keeping it as
> a warning even in the latter case.
>

I never wanted to suppress the warning message in the latter case. I
just wanted to avoid listing the branches. Actually the patch I sent in
one the previous threads[1] that avoids listing the branches has the
following behaviour,

$ git branch -l
warning: the '-l' alias for '--create-reflog' is deprecated;
warning: it will be removed in a future version of Git
usage: git branch [] [-r | -a] [--merged | --no-merged]
   or: git branch [] [-l] [-f]  []
   or: git branch [] [-r] (-d | -D) ...
   or: git branch [] (-m | -M) [] 
   or: git branch [] (-c | -C) [] 
   or: git branch [] [-r | -a] [--points-at]
   or: git branch [] [-r | -a] [--format]


So, the warning message isn't lost. It just prevents the listing of
branches.

Wait, maybe I'm misunderstanding what you mean by "warning". You're
probably meaning something related to the way Git exits in both cases.
With your patch "git branch -l" prints a warning, lists the branches and
has an exit status of 0. With my patch it prints the warning, the usage
specifications with exit status 128. In that case, I still don't think
it's bad to turn "git branch -l" into an error now as it's been
incorrect for a long time now and it's not wrong if we correct it now.

Anyways, if you think it mustn't turn into an error now and only in the
next stage, a suggestion follows in another thread.


[1]: https://public-inbox.org/git/1527174618.10589.4.ca...@gmail.com/


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - John Sonmez


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-25 Thread Junio C Hamano 
Jeff King  writes:

>> By the way, this is one of these times when I feel that we should
>> have a better multi-line message support in die/error/warning/info
>> functions.  Ideally, I should be able to write
>> 
>>  warning(_("the '-l' option is an alias for '--create-reflog' and\n"
>>"has no effect in list mode, This option will soon be\n"
>>"removed and you should omit it (or use '--list' instead)."));
>> 
>> and warning() would:
>> 
>>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>>message;
>> 
>>  - chomp that into lines at '\n' boundary; and
>> 
>>  - give each of these lines with _("warning: ") prefixed.
>> 
>> That way, translators can choose to make the resulting message to
>> different number of lines from the original easily.
>
> Yep, I totally agree. In past discussions I was thinking mostly of
> the pain of writing these multi-line messages. But I imagine it is
> absolute hell for translators, and we should fix it for that reason.
>
> (Also, I guess this message probably ought to be marked for
> translation).

Needless to tell you, I worked backwards when noticing that these
three warning() lines are not marked for translation ;-).

But of course, fixing this in a naïve way will involve memory
allocation during execution of die(), which may well be due to OOM,
which is why we knew the need but haven't found a good solution to
it.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-25 Thread Junio C Hamano 
Junio C Hamano  writes:

> With these two patches queued on top of jk/branch-l-0-deprecation,
> the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
> to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
> a synonym to 'branch --list' needs rebasing.  Both are trivial and
> hopefully I did them correctly.
>
> -- >8 --
> From: Jeff King 
> Date: Mon, 26 Mar 2018 03:29:22 -0400
> Subject: [PATCH] branch: drop deprecated "-l" option

And this is the final "reincarnation" step.

-- >8 --
From: Jeff King 
Date: Mon, 26 Mar 2018 03:29:48 -0400
Subject: [PATCH] branch: make "-l" a synonym for "--list"

The other "mode" options of git-branch have short-option
aliases that are easy to type (e.g., "-d" and "-m"). Let's
give "--list" the same treatment.

This also makes it consistent with the similar "git tag -l"
option.

We didn't do this originally because "--create-reflog" was
squatting on the "-l" option. Now that sufficient time has
passed with that alias removed, we can finally repurpose it.

Signed-off-by: Jeff King 
Reviewed-by: Eric Sunshine 
Reviewed-by: Jacob Keller 
[jc: adjusted the new tests added earlier]
Signed-off-by: Junio C Hamano 
---
 builtin/branch.c  | 2 +-
 t/t3200-branch.sh | 8 +---
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..1d06f5c547 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -612,7 +612,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('M', NULL, , N_("move/rename a branch, even if 
target exists"), 2),
OPT_BIT('c', "copy", , N_("copy a branch and its reflog"), 
1),
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
-   OPT_BOOL(0, "list", , N_("list branch names")),
+   OPT_BOOL('l', "list", , N_("list branch names")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index eca75d3ca1..022d6a41c8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -45,12 +45,6 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
 '
 
-test_expect_success 'git branch -l no longer is --create-reflog' '
-   test_when_finished "git branch -D new-branch-with-reflog || :" &&
-   test_must_fail git branch -l new-branch-with-reflog &&
-   test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
-'
-
 cat >expect < 1117150200 +
branch: Created from master
 EOF
@@ -294,7 +288,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
-test_expect_failure 'git branch -l eventually is --list' '
+test_expect_success 'git branch -l is --list' '
git branch --list >expect &&
git branch -l >actual &&
test_cmp expect actual
-- 
2.17.0-775-ge144d126d7

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-25 Thread Junio C Hamano 
Junio C Hamano  writes:

> Yup, thanks for being extra explicit.  I do imagine there are quite
> a few of us who would be puzzled without this update (but with the
> previous one to unhide it from behind the pager).

With these two patches queued on top of jk/branch-l-0-deprecation,
the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
a synonym to 'branch --list' needs rebasing.  Both are trivial and
hopefully I did them correctly.

-- >8 --
From: Jeff King 
Date: Mon, 26 Mar 2018 03:29:22 -0400
Subject: [PATCH] branch: drop deprecated "-l" option

We marked the "-l" option as deprecated back in . Now that sufficient time has passed, let's follow
through and get rid of it.

Signed-off-by: Jeff King 
Reviewed-by: Eric Sunshine 
Reviewed-by: Jacob Keller 
[jc: added a few tests]
Signed-off-by: Junio C Hamano 
---
 builtin/branch.c  | 14 --
 t/t3200-branch.sh | 12 
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b0b33dab94..01b35b3c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -571,14 +571,6 @@ static int edit_branch_description(const char *branch_name)
return 0;
 }
 
-static int deprecated_reflog_option_cb(const struct option *opt,
-  const char *arg, int unset)
-{
-   used_deprecated_reflog_option = 1;
-   *(int *)opt->value = !unset;
-   return 0;
-}
-
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -622,12 +614,6 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
OPT_BOOL(0, "list", , N_("list branch names")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
-   {
-   OPTION_CALLBACK, 'l', NULL, , NULL,
-   N_("deprecated synonym for --create-reflog"),
-   PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
-   deprecated_reflog_option_cb
-   },
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
OPT__FORCE(, N_("force creation, move/rename, deletion"), 
PARSE_OPT_NOCOMPLETE),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index da97b8a62b..eca75d3ca1 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -45,6 +45,12 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
 '
 
+test_expect_success 'git branch -l no longer is --create-reflog' '
+   test_when_finished "git branch -D new-branch-with-reflog || :" &&
+   test_must_fail git branch -l new-branch-with-reflog &&
+   test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
+'
+
 cat >expect < 1117150200 +
branch: Created from master
 EOF
@@ -288,6 +294,12 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
+test_expect_failure 'git branch -l eventually is --list' '
+   git branch --list >expect &&
+   git branch -l >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'git branch --column' '
COLUMNS=81 git branch --column=column >actual &&
cat >expected <<\EOF &&
-- 
2.17.0-775-ge144d126d7

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-25 Thread Jeff King 
On Fri, May 25, 2018 at 06:14:16PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> >> -  warning("the '-l' alias for '--create-reflog' is deprecated;");
> >> -  warning("it will be removed in a future version of Git");
> >> +  if (list) {
> >> +  warning("the '-l' option is an alias for 
> >> '--create-reflog' and");
> >> +  warning("has no effect in list mode. This option will 
> >> soon be");
> >> +  warning("removed and you should omit it (or use 
> >> '--list' instead).");
> >> +  } else {
> >> +  warning("the '-l' alias for '--create-reflog' is 
> >> deprecated;");
> >> +  warning("it will be removed in a future version of 
> >> Git");
> >> +  }
> 
> By the way, this is one of these times when I feel that we should
> have a better multi-line message support in die/error/warning/info
> functions.  Ideally, I should be able to write
> 
>   warning(_("the '-l' option is an alias for '--create-reflog' and\n"
> "has no effect in list mode, This option will soon be\n"
> "removed and you should omit it (or use '--list' instead)."));
> 
> and warning() would:
> 
>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>message;
> 
>  - chomp that into lines at '\n' boundary; and
> 
>  - give each of these lines with _("warning: ") prefixed.
> 
> That way, translators can choose to make the resulting message to
> different number of lines from the original easily.

Yep, I totally agree. In past discussions I was thinking mostly of
the pain of writing these multi-line messages. But I imagine it is
absolute hell for translators, and we should fix it for that reason.

(Also, I guess this message probably ought to be marked for
translation).

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-25 Thread Junio C Hamano 
Junio C Hamano  writes:

>> -warning("the '-l' alias for '--create-reflog' is deprecated;");
>> -warning("it will be removed in a future version of Git");
>> +if (list) {
>> +warning("the '-l' option is an alias for 
>> '--create-reflog' and");
>> +warning("has no effect in list mode. This option will 
>> soon be");
>> +warning("removed and you should omit it (or use 
>> '--list' instead).");
>> +} else {
>> +warning("the '-l' alias for '--create-reflog' is 
>> deprecated;");
>> +warning("it will be removed in a future version of 
>> Git");
>> +}

By the way, this is one of these times when I feel that we should
have a better multi-line message support in die/error/warning/info
functions.  Ideally, I should be able to write

warning(_("the '-l' option is an alias for '--create-reflog' and\n"
  "has no effect in list mode, This option will soon be\n"
  "removed and you should omit it (or use '--list' instead)."));

and warning() would:

 - do the sprintf formatting thing as necessary to prepare a long multi-line
   message;

 - chomp that into lines at '\n' boundary; and

 - give each of these lines with _("warning: ") prefixed.

That way, translators can choose to make the resulting message to
different number of lines from the original easily.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-25 Thread Junio C Hamano 
Jeff King  writes:

> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
>
> Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.

Yup, thanks for being extra explicit.  I do imagine there are quite
a few of us who would be puzzled without this update (but with the
previous one to unhide it from behind the pager).

> Let's make it clear that:
>
>   1. No, "-l" didn't do what they thought here.
>
>   2. It's going away, and what they should do instead.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/branch.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   setup_auto_pager("branch", 1);
>  
>   if (used_deprecated_reflog_option) {
> - warning("the '-l' alias for '--create-reflog' is deprecated;");
> - warning("it will be removed in a future version of Git");
> + if (list) {
> + warning("the '-l' option is an alias for 
> '--create-reflog' and");
> + warning("has no effect in list mode. This option will 
> soon be");
> + warning("removed and you should omit it (or use 
> '--list' instead).");
> + } else {
> + warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> + warning("it will be removed in a future version of 
> Git");
> + }
>   }
>  
>   if (delete) {

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-24 Thread Jeff King 
On Fri, May 25, 2018 at 10:55:45AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Hmm, actually, I suppose the true value of the warning is to help people
> > doing "git branch -l foo", and it would still work there. The "more
> > extreme" from your suggested patch would only affect "branch -l".
> 
> > Still, I think I prefer the gentler version that we get by keeping it as
> > a warning even in the latter case.
> 
> "git branch -l newbranch [forkpoint]" that warns "We won't be doing
> reflog creation with -l" is good, but "git branch -l" that warns "We
> won't be doing reflog creation with -l" sounds like a pure noise, as
> the user would say "Irrelevant, I am not doing that anyway--I am
> listing".
> 
> The warning to prepare users for the next step jk/branch-l-1-removal
> should say "we won't be accepting '-l' as a silent and unadvertised
> synonym soon. Spell it as --list" when "git branch -l" is given, I
> would think.

I hoped that reminding them that "-l is a synonym for --create-reflog"
would serve as a gentle reminder that they're Doing It Wrong. I guess we
could be more explicit, though.

It is not "we won't be accepting -l as a synonym" though. It was never a
synonym, it's just that it didn't happen to do anything in list mode.

> > @@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char 
> > *prefix)
> > if (list)
> > setup_auto_pager("branch", 1);
> >  
> > +   if (used_deprecated_reflog_option) {
> > +   warning("the '-l' alias for '--create-reflog' is deprecated;");
> > +   warning("it will be removed in a future version of Git");
> > +   }
> 
> So from that point of view, we may need a separate message to warn
> users who _do_ want listing with '-l' before jk/branch-l-1-removal
> removes it?  
> 
> The jk/branch-l-2-resurrection topic later repurposes '-l' for
> '--list' but until that happens 'git branch -l' will error not, no?

Yes, after step 1 it will error out. Again, I hoped the existing message
would prepare people. But maybe we should do this on top of what I
posted earlier?

-- >8 --
Subject: [PATCH] branch: customize "-l" warning in list mode

People mistakenly use "git branch -l", thinking that it
triggers list mode. It doesn't, but the lack of non-option
arguments in that command does (and the "-l" becomes a
silent noop).

Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
we've warned that "-l" is going away. But the warning text
is primarily aimed at people who _meant_ to use "-l", as in
"git branch -l foo". People who mistakenly said "git branch
-l" may be left puzzled.

Let's make it clear that:

  1. No, "-l" didn't do what they thought here.

  2. It's going away, and what they should do instead.

Signed-off-by: Jeff King 
---
 builtin/branch.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55bfacd843..b0b33dab94 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
setup_auto_pager("branch", 1);
 
if (used_deprecated_reflog_option) {
-   warning("the '-l' alias for '--create-reflog' is deprecated;");
-   warning("it will be removed in a future version of Git");
+   if (list) {
+   warning("the '-l' option is an alias for 
'--create-reflog' and");
+   warning("has no effect in list mode. This option will 
soon be");
+   warning("removed and you should omit it (or use 
'--list' instead).");
+   } else {
+   warning("the '-l' alias for '--create-reflog' is 
deprecated;");
+   warning("it will be removed in a future version of 
Git");
+   }
}
 
if (delete) {
-- 
2.17.0.1391.g6fdbf40724

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-24 Thread Junio C Hamano 
Jeff King  writes:

> Hmm, actually, I suppose the true value of the warning is to help people
> doing "git branch -l foo", and it would still work there. The "more
> extreme" from your suggested patch would only affect "branch -l".

> Still, I think I prefer the gentler version that we get by keeping it as
> a warning even in the latter case.

"git branch -l newbranch [forkpoint]" that warns "We won't be doing
reflog creation with -l" is good, but "git branch -l" that warns "We
won't be doing reflog creation with -l" sounds like a pure noise, as
the user would say "Irrelevant, I am not doing that anyway--I am
listing".

The warning to prepare users for the next step jk/branch-l-1-removal
should say "we won't be accepting '-l' as a silent and unadvertised
synonym soon. Spell it as --list" when "git branch -l" is given, I
would think.

> @@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   if (list)
>   setup_auto_pager("branch", 1);
>  
> + if (used_deprecated_reflog_option) {
> + warning("the '-l' alias for '--create-reflog' is deprecated;");
> + warning("it will be removed in a future version of Git");
> + }

So from that point of view, we may need a separate message to warn
users who _do_ want listing with '-l' before jk/branch-l-1-removal
removes it?  

The jk/branch-l-2-resurrection topic later repurposes '-l' for
'--list' but until that happens 'git branch -l' will error not, no?

[PATCH] branch: issue "-l" deprecation warning after pager starts

2018-05-24 Thread Jeff King 
On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote:

> On Thu, May 24, 2018 at 08:40:18PM +0530, Kaartic Sivaraam wrote:
> 
> > > On the other hand, I'm not sure this is that big a deal. The point of
> > > the deprecation warning is to catch people who are actually trying to
> > > use "-l" as "--create-reflog", and that case does not page. The people
> > > doing "git branch -l" are actually getting what they want eventually,
> > > which is to turn it into "--list". In the interim step where it becomes
> > > an unknown option, they'll get a hard error. 
> > 
> > I just thought we wouldn't want to surprise/confuse users who try to
> > use "git branch -l" with the warning message about "create reflog"
> > along-side the list of branches. That would just add to the confusion.
> > So, I thought we should error out when users do "git branch -l"
> > instead. Something like the following should help us prevent "git
> > branch -l" from listing branch names and might also prevent the
> > confusion.
> 
> Yeah, I think that's just a more extreme version of the current plan (it
> turns it immediately into a hard error instead of warning for a while).
> If we just make the warning easier to see in the paged case, I think
> that makes the current plan fine.
> 
> I'll wrap up the patch I sent earlier.

Hmm, actually, I suppose the true value of the warning is to help people
doing "git branch -l foo", and it would still work there. The "more
extreme" from your suggested patch would only affect "branch -l".

Still, I think I prefer the gentler version that we get by keeping it as
a warning even in the latter case.

Here's the patch. It goes on top of jk/branch-l-0-deprecation (and will
naturally conflict with the removal branch, but the resolution should be
obvious).

-- >8 --
Subject: [PATCH] branch: issue "-l" deprecation warning after pager starts

If you run "git branch -l", we issue a deprecation warning
that "-l" is probably not doing what you expect. But we do
so while parsing the options, which is _before_ we start the
pager. Depending on your pager settings, this means that the
warning may get totally overwritten by the pager.

Instead, let's delay the message until after we would have
started the pager. If we do page, then it will end up inside
the pager (since we redirect stderr). And if not (including
the case when you really did mean for "-l" to work as
"--create-reflog"), then it will still get shown on stderr.

Signed-off-by: Jeff King <p...@peff.net>
---
 builtin/branch.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e50a5a1680..55bfacd843 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -34,6 +34,7 @@ static const char * const builtin_branch_usage[] = {
NULL
 };
 
+static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 
@@ -573,8 +574,7 @@ static int edit_branch_description(const char *branch_name)
 static int deprecated_reflog_option_cb(const struct option *opt,
   const char *arg, int unset)
 {
-   warning("the '-l' alias for '--create-reflog' is deprecated;");
-   warning("it will be removed in a future version of Git");
+   used_deprecated_reflog_option = 1;
*(int *)opt->value = !unset;
return 0;
 }
@@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (list)
setup_auto_pager("branch", 1);
 
+   if (used_deprecated_reflog_option) {
+   warning("the '-l' alias for '--create-reflog' is deprecated;");
+   warning("it will be removed in a future version of Git");
+   }
+
if (delete) {
if (!argc)
die(_("branch name required"));
-- 
2.17.0.1391.g6fdbf40724