{"thread":{"id":"41738","subject":"[PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","startedAt":"2016-03-18T12:47:11Z","lastAt":"2016-03-21T16:07:36Z","messageCount":8,"participants":["Elena Petrashen","Junio C Hamano","Mike Hommey","Eric Sunshine","elena petrashen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281143","messageId":"1458305231-2333-1-git-send-email-elena.petrashen@gmail.com","threadId":"41738","inReplyTo":null,"subject":"[PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Elena Petrashen","fromEmail":"elena.petrashen@gmail.com","sentAt":"2016-03-18T12:47:11Z","receivedAt":"2016-03-18T12:47:11Z","isPatch":true,"sender":{"key":"elena.petrashen@gmail.com","avatar":"https://gravatar.com/avatar/a2fd27bdf84eb2b7fc7b034a358aa6777fe86d5a9ac9339f122aea2b6b7689d4?d=mp&s=160"},"body":"Signed-off-by: Elena Petrashen \n---\n\nHi everyone,\n\nAs 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))\nMy goal was to teach git branch to accept - shortcut and interpret it as “previous working branch”, i.e $git branch -D -\nReally looking forward to hear what do you think, so please let me know if something is done incorrectly, etc.\nThank you,\nElena\n\n builtin/branch.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6b..9d0f8a7 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -675,6 +675,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n \t\t\t 0);\n \n+\tint i;\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"-\")) {\n+\t\t\targv[i] = \"@{-1}\";\n+\t\t}\n+\t}\n+\n \tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-- \n1.9.1\n"},{"id":"281155","messageId":"xmqqvb4jrcle.fsf@gitster.mtv.corp.google.com","threadId":"41738","inReplyTo":"1458305231-2333-1-git-send-email-elena.petrashen@gmail.com","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-18T16:10:37Z","receivedAt":"2016-03-18T16:10:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elena Petrashen writes:\n\n> Signed-off-by: Elena Petrashen \n> ---\n>\n> Hi everyone,\n>\n> 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))\n> My goal was to teach git branch to accept - shortcut and interpret it as \"previous working branch\", i.e $git branch -D -\n> Really looking forward to hear what do you think, so please let me know if something is done incorrectly, etc.\n> Thank you,\n> Elena\n\nThanks for your interests.\n\nHere are a few quick ones on the formality:\n\n * The message was sent with \"Content-Type: text/plain; charset=y\",\n which you probably did not intend to. Perhaps use git-send-email\n from a more recent version of Git to avoid such a mistake?\n\n * It is customary to avoid using pretty-quotes (\"“, etc.) around\n here when you are merely quoting things (I couldn't see them due\n to the above \"charset=y\" issue, so I replaced them all in the\n above quote).\n\n * Your lines are overly long (see the above I quoted).\n\nOn the submitted patch itself, in decreasing order of importance:\n\n * The approach you took turns every \"-\" that appears as a command\n line argument into \"@{-1}\", but it does so without even checking\n where \"-\" appears on the command line is meant to take a branch\n name. This closes the door to later add an option that takes \"-\"\n as an argument that means something different (e.g. one common\n use of \"-\" is to mean \"the standard input\" when a filename is\n expected).\n\n * There is no explanation and justification in the proposed log\n message why you took a particular approach. Why is that a good\n approach? What are the possible downsides? What were the\n alternatives (if any), and why is the approach chosen is better\n than them?\n\n * We forbid declaration-after-statement in our codebase.\n\n * When you do not yet have the \"branch I was previously on\", I\n imagine that your implementation would give you this:\n\n $ git branch -d -\n error: branch '@{-1}' not found.\n $ git branch new -\n fatal: Not a valid object name: '@{-1}'.\n\n even though the user did not type '@{-1}'. It probably is OK if\n the user understands \"-\" is merely a short-hand for \"@{-1}\", but\n if you limited your \"turn '-' to '@{-1}'\" to places on the\n command line that are meant to always take a branch name\n (i.e. immediately after \"branch -d\" in the first example), you\n may be able to give a lot more meaningful error message\n (e.g. when doing \"-\" to \"@{-1}\" conversion, notice that you do\n not yet have 'previous branch' and say so, for example).\n\n * You do not need the brace pairs around the body of these new\n \"for\" or \"if\" statements.\n\n> builtin/branch.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 7b45b6b..9d0f8a7 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -675,6 +675,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n> \t\t\t 0);\n> \n> +\tint i;\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"-\")) {\n> +\t\t\targv[i] = \"@{-1}\";\n> +\t\t}\n> +\t}\n> +\n> \tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n> \t\tlist = 1;\n"},{"id":"281161","messageId":"20160318170255.GA9846@glandium.org","threadId":"41738","inReplyTo":"xmqqvb4jrcle.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-03-18T17:02:55Z","receivedAt":"2016-03-18T17:02:55Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Mar 18, 2016 at 09:10:37AM -0700, Junio C Hamano wrote:\n> * We forbid declaration-after-statement in our codebase.\n\nBy the way, why not enforce it with -Werror=declaration-after-statement?\nIf people patching git get an error when they do it, they won't submit\na patch with it, and you won't have to tell them about it :)\n\nMike\n"},{"id":"281163","messageId":"CAPig+cQYYPmUtOoTGDzzQJC8n+9UoVEskmnZ19E=rroyU5+Pkg@mail.gmail.com","threadId":"41738","inReplyTo":"xmqqvb4jrcle.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-18T18:13:24Z","receivedAt":"2016-03-18T18:13:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano wrote:\n> On the submitted patch itself, in decreasing order of importance:\n>\n> * The approach you took turns every \"-\" that appears as a command\n> line argument into \"@{-1}\", but it does so without even checking\n> where \"-\" appears on the command line is meant to take a branch\n> name. This closes the door to later add an option that takes \"-\"\n> as an argument that means something different (e.g. one common\n> use of \"-\" is to mean \"the standard input\" when a filename is\n> expected).\n>\n> * There is no explanation and justification in the proposed log\n> message why you took a particular approach. Why is that a good\n> approach? What are the possible downsides? What were the\n> alternatives (if any), and why is the approach chosen is better\n> than them?\n>\n> * We forbid declaration-after-statement in our codebase.\n>\n> * When you do not yet have the \"branch I was previously on\", I\n> imagine that your implementation would give you this:\n> [...]\n>\n> * You do not need the brace pairs around the body of these new\n> \"for\" or \"if\" statements.\n\nAlso for consideration: You'd probably also want to add a test or two\nor three to verify that \"-\" works as intended, and a documentation\nupdate may be warranted.\n"},{"id":"281164","messageId":"CAPig+cQ7pf2S_bB6tgbe2BazLNv9JpN=35XYGopYViNr3YqgmQ@mail.gmail.com","threadId":"41738","inReplyTo":"20160318170255.GA9846@glandium.org","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-18T18:15:45Z","receivedAt":"2016-03-18T18:15:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 18, 2016 at 1:02 PM, Mike Hommey wrote:\n> On Fri, Mar 18, 2016 at 09:10:37AM -0700, Junio C Hamano wrote:\n>> * We forbid declaration-after-statement in our codebase.\n>\n> By the way, why not enforce it with -Werror=declaration-after-statement?\n> If people patching git get an error when they do it, they won't submit\n> a patch with it, and you won't have to tell them about it :)\n\nSee 658df95 (add DEVELOPER makefile knob to check for acknowledged\nwarnings, 2016-02-25), which is already in 'master'.\n"},{"id":"281343","messageId":"CAJPOeMd-zeThDhx-OWW_6LwG9A+U3UU-oKPOZGcycR-xQRPcgg@mail.gmail.com","threadId":"41738","inReplyTo":"CAPig+cQYYPmUtOoTGDzzQJC8n+9UoVEskmnZ19E=rroyU5+Pkg@mail.gmail.com","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"elena petrashen","fromEmail":"elena.petrashen@gmail.com","sentAt":"2016-03-21T15:12:36Z","receivedAt":"2016-03-21T15:12:36Z","isPatch":true,"sender":{"key":"elena.petrashen@gmail.com","avatar":"https://gravatar.com/avatar/a2fd27bdf84eb2b7fc7b034a358aa6777fe86d5a9ac9339f122aea2b6b7689d4?d=mp&s=160"},"body":"Hi Eric, Junio,\n\nthank you very much for your feedback! I honestly apologize I\ngot so many things wrong.\n\nI'll try to minimize the scope a little bit for the next attempt to\nmake sure my approach is good first and then expand:\ni.e will only teach git branch to delete \"-\" & give out an\nerror message in case there is no previous branch\nto make sure I got the basics correct.\n\nElena\n\nOn Fri, Mar 18, 2016 at 9:13 PM, Eric Sunshine wrote:\n>\n> On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano wrote:\n> > On the submitted patch itself, in decreasing order of importance:\n> >\n> > * The approach you took turns every \"-\" that appears as a command\n> > line argument into \"@{-1}\", but it does so without even checking\n> > where \"-\" appears on the command line is meant to take a branch\n> > name. This closes the door to later add an option that takes \"-\"\n> > as an argument that means something different (e.g. one common\n> > use of \"-\" is to mean \"the standard input\" when a filename is\n> > expected).\n> >\n> > * There is no explanation and justification in the proposed log\n> > message why you took a particular approach. Why is that a good\n> > approach? What are the possible downsides? What were the\n> > alternatives (if any), and why is the approach chosen is better\n> > than them?\n> >\n> > * We forbid declaration-after-statement in our codebase.\n> >\n> > * When you do not yet have the \"branch I was previously on\", I\n> > imagine that your implementation would give you this:\n> > [...]\n> >\n> > * You do not need the brace pairs around the body of these new\n> > \"for\" or \"if\" statements.\n>\n> Also for consideration: You'd probably also want to add a test or two\n> or three to verify that \"-\" works as intended, and a documentation\n> update may be warranted.\n"},{"id":"281346","messageId":"CAPig+cR0dvKUOVwd=Mt=4mzEDVE4NVanPJTdztzEQajzm71=pw@mail.gmail.com","threadId":"41738","inReplyTo":"CAJPOeMd-zeThDhx-OWW_6LwG9A+U3UU-oKPOZGcycR-xQRPcgg@mail.gmail.com","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T16:03:55Z","receivedAt":"2016-03-21T16:03:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 11:12 AM, elena petrashen\n wrote:\n> Hi Eric, Junio,\n>\n> thank you very much for your feedback! I honestly apologize I\n> got so many things wrong.\n\nNo apologies necessary. We understand that it takes time for newcomers\nto the project to absorb the codebase and become familiar with Git\ndevelopment. And, aside from gauging technical ability, an important\naspect of these microprojects is to expose the candidate to the Git\nreview process and see how the candidate absorbs and responds to\nreview comments.\n"},{"id":"281347","messageId":"xmqqy49bn7av.fsf@gitster.mtv.corp.google.com","threadId":"41738","inReplyTo":"CAJPOeMd-zeThDhx-OWW_6LwG9A+U3UU-oKPOZGcycR-xQRPcgg@mail.gmail.com","subject":"Re: [PATCH][Outreachy] branch: allow - as abbreviation of '@{-1}'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-21T16:07:36Z","receivedAt":"2016-03-21T16:07:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"elena petrashen writes:\n\n> Hi Eric, Junio,\n>\n> thank you very much for your feedback! I honestly apologize I\n> got so many things wrong.\n\nNothing to apologize. You are not expected to know all the local\nconventions from the beginning, and we appreciate your willingness\nto adjust ;-)\n\n> I'll try to minimize the scope a little bit for the next attempt to\n> make sure my approach is good first and then expand:\n> i.e will only teach git branch to delete \"-\" & give out an\n> error message in case there is no previous branch\n> to make sure I got the basics correct.\n\nSounds good.\n"}]}