Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
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. -Peff
Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
On Thu, May 17, 2018 at 11:48:33AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > If there are ample branches, the warning message might be
> > hidden out of screen but we shouldn't rely on that, I
> > suppose.
>
> Also if we have lots of branches, depending on your pager settings you
> won't see this at all, I have:
We redirect stderr to the pager, as well, exactly to catch this sort of
case. But because git-branch does not kick in the pager until later
(because it only wants to do it for list-mode), that happens _after_
we've emitted the message.
So one fix would be to teach deprecated_reflog_option_cb() to just set a
flag rather than printing the warning, and then issue the warning.
Something like this:
diff --git a/builtin/branch.c b/builtin/branch.c
index 452742fecf..f51b89e962 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;
@@ -576,8 +577,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;
}
@@ -703,6 +703,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"));
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.
-Peff
Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
On Thu, May 17 2018, Kaartic Sivaraam wrote: > Hi Ævar, > > On Thursday 17 May 2018 03:18 PM, Ævar Arnfjörð Bjarmason wrote: >> I've ended up with that $LESS setting via hackery over the years, so >> maybe I'm doing something retarded, minimal test case: >> >> PAGER=less LESS="--no-init --quit-if-one-screen" git branch -l >> >> ... >> >> So I think this is probably OK for most users, if the have very few >> branches they'll see it, and then if they use default pager settings >> they'll see the stderr output once they quit the pager. I don't know how >> common my (mis)configuration is. >> > I'm not sure this is ok, because I still see the stderr output with your > minimal test case even when I have enough branches to not fit in one > screen. The stderr output is of course above the pager output (after I > quit the pager) and gets hidden out-of display as I stated before. I > also get weird 'ESC[m' characters with you minimal test case. I'm not > sure what I'm missing. I don't get it anywhere, above or below. > What version of 'less' do you use? Is any other configuration that you > didn't mention affecting what your observation? Both "less 481 (GNU regular expressions)" and version 487. I upgraded and tried both. This is on Debian stable (and newer version from testing). It may be something in my system config, but I tried sudo-ing to a user that only has stock Debian config & none of my custom .bashrc etc, and I get the same thing.
Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
Hi Ævar, On Thursday 17 May 2018 03:18 PM, Ævar Arnfjörð Bjarmason wrote: > I've ended up with that $LESS setting via hackery over the years, so > maybe I'm doing something retarded, minimal test case: > > PAGER=less LESS="--no-init --quit-if-one-screen" git branch -l > > ... > > So I think this is probably OK for most users, if the have very few > branches they'll see it, and then if they use default pager settings > they'll see the stderr output once they quit the pager. I don't know how > common my (mis)configuration is. > I'm not sure this is ok, because I still see the stderr output with your minimal test case even when I have enough branches to not fit in one screen. The stderr output is of course above the pager output (after I quit the pager) and gets hidden out-of display as I stated before. I also get weird 'ESC[m' characters with you minimal test case. I'm not sure what I'm missing. What version of 'less' do you use? Is any other configuration that you didn't mention affecting what your observation? -- 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: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
On Thu, May 17 2018, Kaartic Sivaraam wrote: > On Thursday 17 May 2018 11:31 AM, Junio C Hamano wrote: >> * jk/branch-l-0-deprecation (2018-03-26) 3 commits >> >> ... >> >> The "-l" option in "git branch -l" is an unfortunate short-hand for >> "--create-reflog", but many users, both old and new, somehow expect >> it to be something else, perhaps "--list". This step deprecates >> the short-hand and warns about the future removal of the it when it >> is used. >> >> Will cook in 'next'. >> Perhaps merge to 'master' immediately after 2.18 release? > > I still have a slight feeling that we shouldn't list the branches for > "git branch -l" during the deprecation period. If feel this because > > i) It would avoid confusions for the users during the > deprecation period > > ii) The warning message seems to add to the confusion: > > $ git branch -l > warning: the '-l' alias for '--create-reflog' is deprecated; > warning: it will be removed in a future version of Git > * master > ... > > > If there are ample branches, the warning message might be > hidden out of screen but we shouldn't rely on that, I > suppose. Also if we have lots of branches, depending on your pager settings you won't see this at all, I have: PAGER=less LESS="--IGNORE-CASE --LONG-PROMPT --QUIET --chop-long-lines --RAW-CONTROL-CHARS --no-init --quit-if-one-screen" I've ended up with that $LESS setting via hackery over the years, so maybe I'm doing something retarded, minimal test case: PAGER=less LESS="--no-init --quit-if-one-screen" git branch -l So with those two options I don't get the stderr output at all, but if I remove either one of those options once I quit the pager I get the output at the end. So I think this is probably OK for most users, if the have very few branches they'll see it, and then if they use default pager settings they'll see the stderr output once they quit the pager. I don't know how common my (mis)configuration is.
