From: Junio C Hamano <gitster@pobox•com>
To: Elena Petrashen <elena.petrashen@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'
Date: Fri, 18 Mar 2016 09:10:37 -0700 [thread overview]
Message-ID: <xmqqvb4jrcle.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1458305231-2333-1-git-send-email-elena.petrashen@gmail.com> (Elena Petrashen's message of "Fri, 18 Mar 2016 15:47:11 +0300")
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;
next prev parent reply other threads:[~2016-03-18 16:10 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-18 12:47 [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}' Elena Petrashen
2016-03-18 16:10 ` Junio C Hamano [this message]
2016-03-18 17:02 ` Mike Hommey
2016-03-18 18:15 ` Eric Sunshine
2016-03-18 18:13 ` Eric Sunshine
2016-03-21 15:12 ` elena petrashen
2016-03-21 16:03 ` Eric Sunshine
2016-03-21 16:07 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvb4jrcle.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=elena.petrashen@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox