Elena Petrashen <elena.petrashen@xxxxxxxxx> writes:
> Signed-off-by: Elena Petrashen <elena.petrashen@xxxxxxxxx>
> ---
>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html