[PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Elena Petrashen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1170 bytes --]

Signed-off-by: Elena Petrashen <elena.petrashen@gmail.com>
---

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

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Junio C Hamano]

Elena Petrashen <elena.petrashen@gmail.com> writes:

> Signed-off-by: Elena Petrashen <elena.petrashen@gmail.com>
> ---
>
> 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;

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Mike Hommey]

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

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Eric Sunshine]

On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano <gitster@pobox.com> 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.

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Eric Sunshine]

On Fri, Mar 18, 2016 at 1:02 PM, Mike Hommey <mh@glandium.org> 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'.

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[elena petrashen]

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 <sunshine@sunshineco.com> wrote:
>
> On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano <gitster@pobox.com> 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.

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Eric Sunshine]

On Mon, Mar 21, 2016 at 11:12 AM, elena petrashen
<elena.petrashen@gmail.com> 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.

Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'

[Junio C Hamano]

elena petrashen <elena.petrashen@gmail.com> 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.