# [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' 8 messages from 2016-03-18 to 2016-03-21. Participants: Elena Petrashen, Junio C Hamano, Mike Hommey, Eric Sunshine, elena petrashen. Thread: https://gitlist.dev/t/41738 ## Elena Petrashen, 2016-03-18 12:47 Subject: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: <1458305231-2333-1-git-send-email-elena.petrashen@gmail.com> URL: https://gitlist.dev/e/1458305231-2333-1-git-send-email-elena.petrashen%40gmail.com ``` Signed-off-by: Elena Petrashen --- Hi everyone, As my first Outreachy submission micropoject I’ve chosen to try to approach “Allow “-“ as a short-hand for “@{-1}” in more places.” (http://git.github.io/SoC-2016-Microprojects/ (Cf. $gmane/230828)) My goal was to teach git branch to accept - shortcut and interpret it as “previous working branch”, i.e $git branch -D - Really looking forward to hear what do you think, so please let me know if something is done incorrectly, etc. Thank you, Elena builtin/branch.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/builtin/branch.c b/builtin/branch.c index 7b45b6b..9d0f8a7 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -675,6 +675,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, builtin_branch_usage, 0); + int i; + for (i = 0; i < argc; i++) { + if (!strcmp(argv[i], "-")) { + argv[i] = "@{-1}"; + } + } + if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0) list = 1; -- 1.9.1 ``` ## Junio C Hamano, 2016-03-18 16:10 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: URL: https://gitlist.dev/e/xmqqvb4jrcle.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1458305231-2333-1-git-send-email-elena.petrashen@gmail.com> ``` Elena Petrashen writes: > Signed-off-by: Elena Petrashen > --- > > Hi everyone, > > As my first Outreachy submission micropoject I've chosen to try to approach "Allow '-' as a short-hand for '@{-1} in more places." (http://git.github.io/SoC-2016-Microprojects/ (Cf. $gmane/230828)) > My goal was to teach git branch to accept - shortcut and interpret it as "previous working branch", i.e $git branch -D - > Really looking forward to hear what do you think, so please let me know if something is done incorrectly, etc. > Thank you, > Elena Thanks for your interests. Here are a few quick ones on the formality: * The message was sent with "Content-Type: text/plain; charset=y", which you probably did not intend to. Perhaps use git-send-email from a more recent version of Git to avoid such a mistake? * It is customary to avoid using pretty-quotes ("“, etc.) around here when you are merely quoting things (I couldn't see them due to the above "charset=y" issue, so I replaced them all in the above quote). * Your lines are overly long (see the above I quoted). On the submitted patch itself, in decreasing order of importance: * The approach you took turns every "-" that appears as a command line argument into "@{-1}", but it does so without even checking where "-" appears on the command line is meant to take a branch name. This closes the door to later add an option that takes "-" as an argument that means something different (e.g. one common use of "-" is to mean "the standard input" when a filename is expected). * There is no explanation and justification in the proposed log message why you took a particular approach. Why is that a good approach? What are the possible downsides? What were the alternatives (if any), and why is the approach chosen is better than them? * We forbid declaration-after-statement in our codebase. * When you do not yet have the "branch I was previously on", I imagine that your implementation would give you this: $ git branch -d - error: branch '@{-1}' not found. $ git branch new - fatal: Not a valid object name: '@{-1}'. even though the user did not type '@{-1}'. It probably is OK if the user understands "-" is merely a short-hand for "@{-1}", but if you limited your "turn '-' to '@{-1}'" to places on the command line that are meant to always take a branch name (i.e. immediately after "branch -d" in the first example), you may be able to give a lot more meaningful error message (e.g. when doing "-" to "@{-1}" conversion, notice that you do not yet have 'previous branch' and say so, for example). * You do not need the brace pairs around the body of these new "for" or "if" statements. > builtin/branch.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 7b45b6b..9d0f8a7 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -675,6 +675,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > argc = parse_options(argc, argv, prefix, options, builtin_branch_usage, > 0); > > + int i; > + for (i = 0; i < argc; i++) { > + if (!strcmp(argv[i], "-")) { > + argv[i] = "@{-1}"; > + } > + } > + > if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0) > list = 1; ``` ## Mike Hommey, 2016-03-18 17:02 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: <20160318170255.GA9846@glandium.org> URL: https://gitlist.dev/e/20160318170255.GA9846%40glandium.org In-Reply-To: ``` On Fri, Mar 18, 2016 at 09:10:37AM -0700, Junio C Hamano wrote: > * We forbid declaration-after-statement in our codebase. By the way, why not enforce it with -Werror=declaration-after-statement? If people patching git get an error when they do it, they won't submit a patch with it, and you won't have to tell them about it :) Mike ``` ## Eric Sunshine, 2016-03-18 18:13 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQYYPmUtOoTGDzzQJC8n%2B9UoVEskmnZ19E%3DrroyU5%2BPkg%40mail.gmail.com In-Reply-To: ``` On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano wrote: > On the submitted patch itself, in decreasing order of importance: > > * The approach you took turns every "-" that appears as a command > line argument into "@{-1}", but it does so without even checking > where "-" appears on the command line is meant to take a branch > name. This closes the door to later add an option that takes "-" > as an argument that means something different (e.g. one common > use of "-" is to mean "the standard input" when a filename is > expected). > > * There is no explanation and justification in the proposed log > message why you took a particular approach. Why is that a good > approach? What are the possible downsides? What were the > alternatives (if any), and why is the approach chosen is better > than them? > > * We forbid declaration-after-statement in our codebase. > > * When you do not yet have the "branch I was previously on", I > imagine that your implementation would give you this: > [...] > > * You do not need the brace pairs around the body of these new > "for" or "if" statements. Also for consideration: You'd probably also want to add a test or two or three to verify that "-" works as intended, and a documentation update may be warranted. ``` ## Eric Sunshine, 2016-03-18 18:15 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ7pf2S_bB6tgbe2BazLNv9JpN%3D35XYGopYViNr3YqgmQ%40mail.gmail.com In-Reply-To: <20160318170255.GA9846@glandium.org> ``` On Fri, Mar 18, 2016 at 1:02 PM, Mike Hommey wrote: > On Fri, Mar 18, 2016 at 09:10:37AM -0700, Junio C Hamano wrote: >> * We forbid declaration-after-statement in our codebase. > > By the way, why not enforce it with -Werror=declaration-after-statement? > If people patching git get an error when they do it, they won't submit > a patch with it, and you won't have to tell them about it :) See 658df95 (add DEVELOPER makefile knob to check for acknowledged warnings, 2016-02-25), which is already in 'master'. ``` ## elena petrashen, 2016-03-21 15:12 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: URL: https://gitlist.dev/e/CAJPOeMd-zeThDhx-OWW_6LwG9A%2BU3UU-oKPOZGcycR-xQRPcgg%40mail.gmail.com In-Reply-To: ``` Hi Eric, Junio, thank you very much for your feedback! I honestly apologize I got so many things wrong. I'll try to minimize the scope a little bit for the next attempt to make sure my approach is good first and then expand: i.e will only teach git branch to delete "-" & give out an error message in case there is no previous branch to make sure I got the basics correct. Elena On Fri, Mar 18, 2016 at 9:13 PM, Eric Sunshine wrote: > > On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano wrote: > > On the submitted patch itself, in decreasing order of importance: > > > > * The approach you took turns every "-" that appears as a command > > line argument into "@{-1}", but it does so without even checking > > where "-" appears on the command line is meant to take a branch > > name. This closes the door to later add an option that takes "-" > > as an argument that means something different (e.g. one common > > use of "-" is to mean "the standard input" when a filename is > > expected). > > > > * There is no explanation and justification in the proposed log > > message why you took a particular approach. Why is that a good > > approach? What are the possible downsides? What were the > > alternatives (if any), and why is the approach chosen is better > > than them? > > > > * We forbid declaration-after-statement in our codebase. > > > > * When you do not yet have the "branch I was previously on", I > > imagine that your implementation would give you this: > > [...] > > > > * You do not need the brace pairs around the body of these new > > "for" or "if" statements. > > Also for consideration: You'd probably also want to add a test or two > or three to verify that "-" works as intended, and a documentation > update may be warranted. ``` ## Eric Sunshine, 2016-03-21 16:03 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR0dvKUOVwd%3DMt%3D4mzEDVE4NVanPJTdztzEQajzm71%3Dpw%40mail.gmail.com In-Reply-To: ``` On Mon, Mar 21, 2016 at 11:12 AM, elena petrashen wrote: > Hi Eric, Junio, > > thank you very much for your feedback! I honestly apologize I > got so many things wrong. No apologies necessary. We understand that it takes time for newcomers to the project to absorb the codebase and become familiar with Git development. And, aside from gauging technical ability, an important aspect of these microprojects is to expose the candidate to the Git review process and see how the candidate absorbs and responds to review comments. ``` ## Junio C Hamano, 2016-03-21 16:07 Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Message-ID: URL: https://gitlist.dev/e/xmqqy49bn7av.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` elena petrashen writes: > Hi Eric, Junio, > > thank you very much for your feedback! I honestly apologize I > got so many things wrong. Nothing to apologize. You are not expected to know all the local conventions from the beginning, and we appreciate your willingness to adjust ;-) > I'll try to minimize the scope a little bit for the next attempt to > make sure my approach is good first and then expand: > i.e will only teach git branch to delete "-" & give out an > error message in case there is no previous branch > to make sure I got the basics correct. Sounds good. ```