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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2016, 16:10 UTC
Message-ID: <xmqqvb4jrcle.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458305231-2333-1-git-send-email-elena.petrashen@gmail.com>

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

Show 10 quoted lines

> 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.

Show 20 quoted lines

>  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;