{"thread":{"id":"45053","subject":"[PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","startedAt":"2017-02-05T12:58:08Z","lastAt":"2017-02-09T18:32:55Z","messageCount":11,"participants":["Siddharth Kannan","Pranit Bauva","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310874","messageId":"1486299439-2859-1-git-send-email-kannan.siddharth12@gmail.com","threadId":"45053","inReplyTo":null,"subject":"[PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-05T12:57:19Z","receivedAt":"2017-02-05T12:58:08Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Search and replace \"-\" (written in the context of a branch name) in the argument\nlist with \"@{-1}\". As per the help text of git rev-list, this includes the following four\ncases:\n\n  a. \"-\"\n  b. \"^-\"\n  c. \"-..other-branch-name\" or \"other-branch-name..-\"\n  d. \"-...other-branch-name\" or \"other-branch-name...-\"\n\n(a) and (b) have been implemented as in the previous implementations of\nthis abbreviation. Namely, 696acf45 (checkout: implement \"-\" abbreviation, add\ndocs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow \"-\" as\nabbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow \"-\" as a\nshort-hand for \"previous branch\", 2011-04-07)\n\n(c) and (d) have been implemented by using the strbuf API, growing it to the\nright size and placing \"@{-1}\" instead of \"-\"\n\nSigned-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\n---\nThis is a patch for one of the microprojects of SoC 2017. [1]\n\nI have implemented this using multiple methods, that I have re-written again and\nagain for better versions ([2]). The present version I feel is the closest that\nI could get to the existing code in the repository. This patch only uses\nfunctions that are commonly used in the rest of the codebase.\n\nI still have to write tests, as well as update documentation as done in 696acf45\n(checkout: implement \"-\" abbreviation, add docs and tests, 2009-01-17).\n\nI request your comments on this patch. Also, I have the following questions\nregarding this patch:\n\n1. Is the approach that I have used to solve this problem fine?\n2. Is the code I am writing in the right function? (I have put it right\nbefore the revisions data structure is setup, thus these changes affect only\ngit-log)\n\n[1]: https://git.github.io/SoC-2017-Microprojects/\n[2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses\nstrbufs for the starting 4 characters, and last 4 characters and compares those\nto the appropriate strings for case (c) and case (d). I edited this patch to use\nstrstr instead, which avoids all the strbuf declarations)\n\n builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 55d20cc..a5aac99 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\t\t struct rev_info *rev, struct setup_revision_opt *opt)\n {\n \tstruct userformat_want w;\n-\tint quiet = 0, source = 0, mailmap = 0;\n+\tint quiet = 0, source = 0, mailmap = 0, i = 0;\n \tstatic struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};\n\n \tconst struct option builtin_log_options[] = {\n@@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n\n \tif (quiet)\n \t\trev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n+\n+\t/*\n+\t * Check if any argument has a \"-\" in it, which has been referred to as a\n+\t * shorthand for @{-1}.  Handles methods that might be used to list commits\n+\t * as mentioned in git rev-list --help\n+\t */\n+\n+\tfor(i = 0; i < argc; ++i) {\n+\t\tif (!strcmp(argv[i], \"-\")) {\n+\t\t\targv[i] = \"@{-1}\";\n+\t\t} else if (!strcmp(argv[i], \"^-\")) {\n+\t\t\targv[i] = \"^@{-1}\";\n+\t\t} else if (strlen(argv[i]) >= 4) {\n+\n+\t\t\tif (strstr(argv[i], \"-...\") == argv[i] || strstr(argv[i], \"-..\") == argv[i]) {\n+\t\t\t\tstruct strbuf changed_argument = STRBUF_INIT;\n+\n+\t\t\t\tstrbuf_addstr(&changed_argument, \"@{-1}\");\n+\t\t\t\tstrbuf_addstr(&changed_argument, argv[i] + 1);\n+\n+\t\t\t\tstrbuf_setlen(&changed_argument, strlen(argv[i]) + 4);\n+\n+\t\t\t\targv[i] = strbuf_detach(&changed_argument, NULL);\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * Find the first occurence, and add the size to it and proceed if\n+\t\t\t * the resulting value is NULL\n+\t\t\t */\n+\t\t\tif (!(strstr(argv[i], \"...-\") + 4)  ||\n+\t\t\t\t\t!(strstr(argv[i], \"..-\") + 3)) {\n+\t\t\t\tstruct strbuf changed_argument = STRBUF_INIT;\n+\n+\t\t\t\tstrbuf_addstr(&changed_argument, argv[i]);\n+\n+\t\t\t\tstrbuf_grow(&changed_argument, strlen(argv[i]) + 4);\n+\t\t\t\tstrbuf_setlen(&changed_argument, strlen(argv[i]) + 4);\n+\n+\t\t\t\tstrbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, \"@{-1}\", 5);\n+\n+\t\t\t\targv[i] = strbuf_detach(&changed_argument, NULL);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \targc = setup_revisions(argc, argv, rev, opt);\n\n \t/* Any arguments at this point are not recognized */\n--\n2.1.4\n\n"},{"id":"310876","messageId":"CAFZEwPOFPPyui=9mnccbOc-79q0URYhdGHSkcd0YyR6qe-c_zQ@mail.gmail.com","threadId":"45053","inReplyTo":"1486299439-2859-1-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2017-02-05T14:55:25Z","receivedAt":"2017-02-05T14:55:32Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Siddharth,\n\nOn Sun, Feb 5, 2017 at 6:27 PM, Siddharth Kannan\n<kannan.siddharth12@gmail.com> wrote:\n> Search and replace \"-\" (written in the context of a branch name) in the argument\n> list with \"@{-1}\". As per the help text of git rev-list, this includes the following four\n> cases:\n>\n>   a. \"-\"\n>   b. \"^-\"\n>   c. \"-..other-branch-name\" or \"other-branch-name..-\"\n>   d. \"-...other-branch-name\" or \"other-branch-name...-\"\n>\n> (a) and (b) have been implemented as in the previous implementations of\n> this abbreviation. Namely, 696acf45 (checkout: implement \"-\" abbreviation, add\n> docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow \"-\" as\n> abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow \"-\" as a\n> short-hand for \"previous branch\", 2011-04-07)\n>\n> (c) and (d) have been implemented by using the strbuf API, growing it to the\n> right size and placing \"@{-1}\" instead of \"-\"\n>\n> Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\n> ---\n> This is a patch for one of the microprojects of SoC 2017. [1]\n>\n> I have implemented this using multiple methods, that I have re-written again and\n> again for better versions ([2]). The present version I feel is the closest that\n> I could get to the existing code in the repository. This patch only uses\n> functions that are commonly used in the rest of the codebase.\n>\n> I still have to write tests, as well as update documentation as done in 696acf45\n> (checkout: implement \"-\" abbreviation, add docs and tests, 2009-01-17).\n>\n> I request your comments on this patch. Also, I have the following questions\n> regarding this patch:\n>\n> 1. Is the approach that I have used to solve this problem fine?\n> 2. Is the code I am writing in the right function? (I have put it right\n> before the revisions data structure is setup, thus these changes affect only\n> git-log)\n>\n> [1]: https://git.github.io/SoC-2017-Microprojects/\n> [2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses\n> strbufs for the starting 4 characters, and last 4 characters and compares those\n> to the appropriate strings for case (c) and case (d). I edited this patch to use\n> strstr instead, which avoids all the strbuf declarations)\n>\n>  builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n>  1 file changed, 46 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 55d20cc..a5aac99 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>                          struct rev_info *rev, struct setup_revision_opt *opt)\n>  {\n>         struct userformat_want w;\n> -       int quiet = 0, source = 0, mailmap = 0;\n> +       int quiet = 0, source = 0, mailmap = 0, i = 0;\n>         static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};\n>\n>         const struct option builtin_log_options[] = {\n> @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>\n>         if (quiet)\n>                 rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n> +\n> +       /*\n> +        * Check if any argument has a \"-\" in it, which has been referred to as a\n> +        * shorthand for @{-1}.  Handles methods that might be used to list commits\n> +        * as mentioned in git rev-list --help\n> +        */\n> +\n> +       for(i = 0; i < argc; ++i) {\n> +               if (!strcmp(argv[i], \"-\")) {\n> +                       argv[i] = \"@{-1}\";\n> +               } else if (!strcmp(argv[i], \"^-\")) {\n> +                       argv[i] = \"^@{-1}\";\n> +               } else if (strlen(argv[i]) >= 4) {\n> +\n> +                       if (strstr(argv[i], \"-...\") == argv[i] || strstr(argv[i], \"-..\") == argv[i]) {\n> +                               struct strbuf changed_argument = STRBUF_INIT;\n> +\n> +                               strbuf_addstr(&changed_argument, \"@{-1}\");\n> +                               strbuf_addstr(&changed_argument, argv[i] + 1);\n> +\n> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);\n> +\n> +                               argv[i] = strbuf_detach(&changed_argument, NULL);\n> +                       }\n> +\n> +                       /*\n> +                        * Find the first occurence, and add the size to it and proceed if\n> +                        * the resulting value is NULL\n> +                        */\n> +                       if (!(strstr(argv[i], \"...-\") + 4)  ||\n> +                                       !(strstr(argv[i], \"..-\") + 3)) {\n> +                               struct strbuf changed_argument = STRBUF_INIT;\n> +\n> +                               strbuf_addstr(&changed_argument, argv[i]);\n> +\n> +                               strbuf_grow(&changed_argument, strlen(argv[i]) + 4);\n> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);\n> +\n> +                               strbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, \"@{-1}\", 5);\n> +\n> +                               argv[i] = strbuf_detach(&changed_argument, NULL);\n> +                       }\n> +               }\n> +       }\n> +\n>         argc = setup_revisions(argc, argv, rev, opt);\n>\n>         /* Any arguments at this point are not recognized */\n> --\n\n\nIt is highly recommended to follow the pre existing style of code and\ncommits. In the micro project list, I think it is mentioned that this\nsimilar thing is implemented in git-merge so you should try and dig\nthe commit history of that file to find the similar change.\n\nIf you do this, then you will find out that there is a very short and\nsweet way to do it. I won't directly point out the commit.\n\nstrbuf API should be used when you need to modify the contents of the\nstring. I think you have a little confusion.\n\nIf you declare the string as,\n\nconst char *str = \"foo\";\n\nthen, you can also do,\n\nstr = \"bar\";\n\nBut you can't do,\n\nstr[1] = 'z';\n\nI hope you get what I am saying, if not, search for it.\n\nRegards,\nPranit Bauva\n"},{"id":"310894","messageId":"xmqqtw882n08.fsf@gitster.mtv.corp.google.com","threadId":"45053","inReplyTo":"1486299439-2859-1-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-06T00:15:03Z","receivedAt":"2017-02-06T00:15:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n\n> @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>\n>  \tif (quiet)\n>  \t\trev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n> +\n> +\t/*\n> +\t * Check if any argument has a \"-\" in it, which has been referred to as a\n> +\t * shorthand for @{-1}.  Handles methods that might be used to list commits\n> +\t * as mentioned in git rev-list --help\n> +\t */\n> +\n> +\tfor(i = 0; i < argc; ++i) {\n> +\t\tif (!strcmp(argv[i], \"-\")) {\n> +\t\t\targv[i] = \"@{-1}\";\n> +\t\t} else if (!strcmp(argv[i], \"^-\")) {\n> +\t\t\targv[i] = \"^@{-1}\";\n> +\t\t} else if (strlen(argv[i]) >= 4) {\n> +\n> +\t...\n> +\t\t}\n> +\t}\n> +\n>  \targc = setup_revisions(argc, argv, rev, opt);\n\n\"Turn '-' to '@{-1}' before we do the real parsing\" can never be a\nreasonable strategy to implement the desired \"'-' means the tip of\nthe previous branch\" correctly.  To understand why, you only need to\nimagine what happens to this command:\n\n    $ git log --grep '^-'\n\nTurning it into \"git log --grep '^@{-1}'\" obviously is not what the\nend-users want, so that is an immediate bug in the version of Git\nwith this patch applied.\n\nEven if this were not a patch for the \"log\" command but for some\nother command, a change with the above approach is very much\nunwelcome, even if that other command does not currently have any\noption that takes arbitrary string the user may want to specify\n(like \"find commit with a line that matches this pattern\" option\ncalled \"--grep\" the \"log\" command has).  That is because it will\nmake it impossible to enhance the command by adding such an option\nin the future.  So it is also adding the problems to future\ndevelopers (and users) of Git.\n\nA correct solution needs to know if the argument is at the position\nwhere a revision (or revision range) is expected and then give the\ntip of the previous branch when it sees \"-\" (and other combinations\nthis patch tries to cover).  In other words, the parser always knows\nwhat it is parsing, and if and only if it is parsing a rev, react to\n\"-\" and think \"ah, the user wants me to use the tip of the previous\nbranch\".\n\nBut the code that knows that it expects to see a revision already\nexists, and it is the real parser.  In the above snippet,\nsetup_revisions() is the one that does the real parsing of argv[].\nThe code there knows when it wants to see a rev, and takes argv[i]\nand turns into an object to call add_pending_object().  That codepath\nmay not yet know that \"-\" means the tip of the previous branch, and\nthat is where the change needs to go.\n\nSuch a properly-done update does not need to textually replace \"-\"\nwith \"@{-1}\" in argv[]; the codepath is where it understands what\nany textual representation of a rev the user gave it means, and it\nunderstands \"@{-1}\" there.  It would be the matter of updating it to\nalso understand what \"-\" means.\n\nA correct solution will be a lot more involved, of course, and I\nthink it will be larger than a reasonable microproject for people\nnew to the codebase.\n\nI didn't check the microproject ideas page myself; whether it says\nthat turning \"-\" unconditionally to \"@{-1}\" is a good idea, or it\nhints that supporting \"-\" as \"the tip of the previous branch\" in\nmore commands is a reasonable byte-sized microproject, I think it is\nmisleading and misguided.  Can somebody remove that entry so that we\nwon't waste time of new developers (which would lead to discouraging\nthem)?  Thanks.\n"},{"id":"310895","messageId":"20170206022705.GA3323@ubuntu-512mb-blr1-01.localdomain","threadId":"45053","inReplyTo":"xmqqtw882n08.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-06T02:27:05Z","receivedAt":"2017-02-06T02:27:15Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Junio,\nOn Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:\n> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n> \n> > @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n> >\n> >  \tif (quiet)\n> >  \t\trev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n> > +\n> > +\t/*\n> > +\t * Check if any argument has a \"-\" in it, which has been referred to as a\n> > +\t * shorthand for @{-1}.  Handles methods that might be used to list commits\n> > +\t * as mentioned in git rev-list --help\n> > +\t */\n> > +\n> > +\tfor(i = 0; i < argc; ++i) {\n> > +\t\tif (!strcmp(argv[i], \"-\")) {\n> > +\t\t\targv[i] = \"@{-1}\";\n> > +\t\t} else if (!strcmp(argv[i], \"^-\")) {\n> > +\t\t\targv[i] = \"^@{-1}\";\n> > +\t\t} else if (strlen(argv[i]) >= 4) {\n> > +\n> > +\t...\n> > +\t\t}\n> > +\t}\n> > +\n> >  \targc = setup_revisions(argc, argv, rev, opt);\n> \n> \"Turn '-' to '@{-1}' before we do the real parsing\" can never be a\n> reasonable strategy to implement the desired \"'-' means the tip of\n> the previous branch\" correctly.  To understand why, you only need to\n> imagine what happens to this command:\n> \n>     $ git log --grep '^-'\n> \n> Turning it into \"git log --grep '^@{-1}'\" obviously is not what the\n> end-users want, so that is an immediate bug in the version of Git\n> with this patch applied.\n> \n> Even if this were not a patch for the \"log\" command but for some\n> other command, a change with the above approach is very much\n> unwelcome, even if that other command does not currently have any\n> option that takes arbitrary string the user may want to specify\n> (like \"find commit with a line that matches this pattern\" option\n> called \"--grep\" the \"log\" command has).  That is because it will\n> make it impossible to enhance the command by adding such an option\n> in the future.  So it is also adding the problems to future\n> developers (and users) of Git.\n\nUnderstood!\n> \n> A correct solution needs to know if the argument is at the position\n> where a revision (or revision range) is expected and then give the\n> tip of the previous branch when it sees \"-\" (and other combinations\n> this patch tries to cover).  In other words, the parser always knows\n> what it is parsing, and if and only if it is parsing a rev, react to\n> \"-\" and think \"ah, the user wants me to use the tip of the previous\n> branch\".\n\nAh, okay. I will do another one of the suggestions as my micro project\nbut continue to look into this part of the code and try to find the\nright place to write the code to implement the present patch.\n\n> \n> But the code that knows that it expects to see a revision already\n> exists, and it is the real parser.  In the above snippet,\n> setup_revisions() is the one that does the real parsing of argv[].\n> The code there knows when it wants to see a rev, and takes argv[i]\n> and turns into an object to call add_pending_object().  That codepath\n> may not yet know that \"-\" means the tip of the previous branch, and\n> that is where the change needs to go.\n> \n> Such a properly-done update does not need to textually replace \"-\"\n> with \"@{-1}\" in argv[]; the codepath is where it understands what\n> any textual representation of a rev the user gave it means, and it\n> understands \"@{-1}\" there.  It would be the matter of updating it to\n> also understand what \"-\" means.\n> \n> A correct solution will be a lot more involved, of course, and I\n> think it will be larger than a reasonable microproject for people\n> new to the codebase.\n> \n> I didn't check the microproject ideas page myself; whether it says\n> that turning \"-\" unconditionally to \"@{-1}\" is a good idea, or it\n> hints that supporting \"-\" as \"the tip of the previous branch\" in\n> more commands is a reasonable byte-sized microproject, I think it is\n> misleading and misguided.  Can somebody remove that entry so that we\n> won't waste time of new developers (which would lead to discouraging\n> them)?  Thanks.\n\nThanks a lot for writing this detailed reply! I will definitely take\ninto account all of the points mentioned here in the future patches I\nsend.\n\n- Siddharth Kannan\n"},{"id":"310926","messageId":"20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain","threadId":"45053","inReplyTo":"xmqqtw882n08.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-06T18:10:26Z","receivedAt":"2017-02-06T18:10:35Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Junio, I did some more digging into the codepath:\n\nOn Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:\n> \n> A correct solution needs to know if the argument is at the position\n> where a revision (or revision range) is expected and then give the\n> tip of the previous branch when it sees \"-\" (and other combinations\n> this patch tries to cover).  In other words, the parser always knows\n> what it is parsing, and if and only if it is parsing a rev, react to\n> \"-\" and think \"ah, the user wants me to use the tip of the previous\n> branch\".\n> \n> But the code that knows that it expects to see a revision already\n> exists, and it is the real parser.  In the above snippet,\n> setup_revisions() is the one that does the real parsing of argv[].\n> The code there knows when it wants to see a rev, and takes argv[i]\n> and turns into an object to call add_pending_object().  That codepath\n> may not yet know that \"-\" means the tip of the previous branch, and\n> that is where the change needs to go.\n\nInside setup_revisions, it tries to parse arguments and options. In\nthere, is this line of code:\n\n    if (*arg == '-') {\n\nOnce control enters this branch, it will either parse the argument as\nan option / pseudo-option, or simply leave this argument as is in the\nargv[] array and move forward with the other arguments.\n\nSo, first I need to teach setup_revisions that something starting with\na \"-\" might be a revision or a revision range.\n\nAfter this, going further down the codepath, in\nrevision.c:handle_revision_arg: \n\nThe argument is parsed to find out if it is of the form\nrev1...rev2 or rev1..rev2 or just rev1, etc.\n\n(a) -> If `..` or `...` was found, then two pointers \"this\" and \"next\"\nnow hold the from and to revisions, and the function\nget_sha1_committish is called on them. In case both were found to be\ncommittish, then the char pointers now hold the sha1 in them, they are\nparsed into objects.\n\n(b) -> Else look for \"r1^@\", \"r1^!\" (This could be \"-^@\", \"-^!\") To\nget r1, again the function get_sha1_committish is called with only r1\nas the parameter.\n\n(c) -> Else look for \"r1^-\"\n\n(d) -> Else look for the argument using the same get_sha1_committish\nfunction (It any \"^\" was present in it, it has already been noted and\nremoved)\n\nCases (a), (b) and (d) can be handled by putting this inside\nget_sha1_committish. (Further discussion about that below)\n\nCase (c) is a bit confusing. This could be something like \"-^-\", and\nsomething like \"^-\" could mean \"Not commits on previous branch\" or it\ncould mean \"All commits on this branch except for the parent of HEAD\"\nPlease tell me if this is confusing or undesired altogether.\nPersonally, I feel that people who have been using \"^-\" would be\nvery confused if it's behaviour changed.\n\nSo, all the code till now points at adding the patch for \"-\" = \"@{-1}\"\ninside get_sha1_committish or downstream from there.\n\nget_sha1_committish \n-> get_sha1_with_context \n-> get_sha1_with_context_1\n-> get_sha1_1 \n  -> peel_onion -> calling get_sha1_basic again with the ref\n  only (after peeling) \n  -> get_sha1_basic -> includes parsing of \"@{-N}\" type revs. So, \n  this indicates that if we can convert the \"-\" appropriately \n  before this point, then it would be good.\n  -> get_short_sha1\n\nSo, this patch reduces to the following 2 tasks:\n\n1. Teach setup_revisions that something starting with \"-\" can be an\nargument as well\n2. Teach get_sha1_basic that \"-\" means the tip of the previous branch\nperhaps by replacing it with \"@{-1}\" just before the reflog parsing is\ndone\n\n> A correct solution will be a lot more involved, of course, and I\n> think it will be larger than a reasonable microproject for people\n> new to the codebase.\n\nSo true :) I had spent a fair bit of time already on my previous patch,\nand I thought I might as well complete my research into this, and send\nthis write-up to the mailing list, so that I could write a patch some\ntime later. In case you would prefer for me to not work on this\nanymore because I am new to the codebase, I will leave it at this.\n\n- Siddharth Kannan\n"},{"id":"310953","messageId":"xmqqtw86zzk4.fsf@gitster.mtv.corp.google.com","threadId":"45053","inReplyTo":"20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-06T23:09:47Z","receivedAt":"2017-02-06T23:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n\n> Hey Junio, I did some more digging into the codepath:\n> ...\n> In case you would prefer for me to not work on this anymore\n> because I am new to the codebase, I will leave it at this.\n\nThe above is nicely analized and summarized.\n\nThe earlier mention of \"those new to the codebase\" by me was \"this\nis an inappropriate topic as a GSoC microproject for people new to\nthe codebase\" and it wasn't meant to say \"this part of the code is\ntoo precious to let unknown folks touch it.\"\n\nThe focus of GSoC being mentoring those who are new to the open\nsource development, and hopefully retain them in the community after\nGSoC is over, we do expect microprojects to be suitable for those\nwho are new to the codebase.\n\nThe focus of microprojects are twofold.  It is a way for new people\nto learn the way in which they will be interacting with the\ncommunity once they become Git developers, sending their patches\n(which includes analyzing and explaining the problem they are trying\nto solve and their solution to it) and receiving and responding to\nreview comments.  We also want to find out which candidates are\nwilling to learn and which ones are difficult to work with during\nthe process.  And its primary focus is not about solving the real\nissues the project has with its code---something \"bite-sized\" is\nsufficient (and desirable) for microprojects for both GSoC student\ncandidates and GSoC mentors and reviewers to work with.\n\n> (c) -> Else look for \"r1^-\"\n> ...\n> Case (c) is a bit confusing. This could be something like \"-^-\", and\n> something like \"^-\" could mean \"Not commits on previous branch\" or it\n> could mean \"All commits on this branch except for the parent of HEAD\"\n\nDo you mean:\n\n    \"git rev-parse ^-\" does not mean \"git rev-parse HEAD^-\", but we\n    probably would want to, and if that is what is going to happen,\n    \"^-\" should mean \"HEAD^-\", and cannot be used for \"^@{-1}\"?\n\nIt's friend \"^!\" does not mean \"HEAD^!\", and \"^@\" does not mean\n\"HEAD^@\", either (the latter is somewhat borked, though, and \"^@\"\ntranslates to \"^HEAD\" because confusingly \"@\" stands for \"HEAD\"\nsometimes).  \n\nSo my gut feeling is that it is probably OK to make \"^-\" mean\n\"^@{-1}\"; it may be prudent to at least initially keep \"^-\" an error\nlike it currently is already, though.\n\n\n"},{"id":"310993","messageId":"20170207191450.GA5569@ubuntu-512mb-blr1-01.localdomain","threadId":"45053","inReplyTo":"xmqqtw86zzk4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-07T19:14:50Z","receivedAt":"2017-02-07T19:15:08Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"On Mon, Feb 06, 2017 at 03:09:47PM -0800, Junio C Hamano wrote:\n> The focus of GSoC being mentoring those who are new to the open\n> source development, and hopefully retain them in the community after\n> GSoC is over, we do expect microprojects to be suitable for those\n> who are new to the codebase.\n\nOkay, understood! Since I have spent time here anyway, I guess I will\ncontinue on this instead of going over to a new micro project.\n\n> \n> > (c) -> Else look for \"r1^-\"\n> > ...\n> > Case (c) is a bit confusing. This could be something like \"-^-\", and\n> > something like \"^-\" could mean \"Not commits on previous branch\" or it\n> > could mean \"All commits on this branch except for the parent of HEAD\"\n> \n> Do you mean:\n> \n>     \"git rev-parse ^-\" does not mean \"git rev-parse HEAD^-\", but we\n>     probably would want to, and if that is what is going to happen,\n>     \"^-\" should mean \"HEAD^-\", and cannot be used for \"^@{-1}\"?\n> \n> It's friend \"^!\" does not mean \"HEAD^!\", and \"^@\" does not mean\n> \"HEAD^@\", either (the latter is somewhat borked, though, and \"^@\"\n> translates to \"^HEAD\" because confusingly \"@\" stands for \"HEAD\"\n> sometimes).  \n\nYes, I meant that whether we should use ^- as ^@{-1} or HEAD^-.\n\nOh! So, that's why running `git log ^@` leads to an empty set!\n> \n> So my gut feeling is that it is probably OK to make \"^-\" mean\n> \"^@{-1}\"; it may be prudent to at least initially keep \"^-\" an error\n> like it currently is already, though.\n\nI agree with your gut feeling, and would like to _not_ exclude only\nthis case. This way, across the code and implementation, there\nwouldn't be any particular cases which would have to be excluded.\n\n> > So, this patch reduces to the following 2 tasks:\n> > \n> > 1. Teach setup_revisions that something starting with \"-\" can be\n> > an\n> > argument as well\n> > 2. Teach get_sha1_basic that \"-\" means the tip of the previous\n> > branch\n> > perhaps by replacing it with \"@{-1}\" just before the reflog\n> > parsing is\n> > done\n\nMaking a change in sha1_name.c will touch a lot of commands\n(setup_revisions is called from everywhere in the codebase), so, I am\nstill trying to figure out how to do this such that the rest of the\ncodepath remains unchanged.\n\nI hope that you do not mind this side-effect, but rather, you intended\nfor this to happen, right? More commands will start supporting this\nshorthand, suddenly.  (such as format-patch, whatchanged, diff to name\na very few).\n\nBest Regards,\n\nSiddharth.\n"},{"id":"311066","messageId":"vpqh944eof7.fsf@anie.imag.fr","threadId":"45053","inReplyTo":"20170207191450.GA5569@ubuntu-512mb-blr1-01.localdomain","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-02-08T14:40:28Z","receivedAt":"2017-02-08T15:58:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n\n> Making a change in sha1_name.c will touch a lot of commands\n> (setup_revisions is called from everywhere in the codebase), so, I am\n> still trying to figure out how to do this such that the rest of the\n> codepath remains unchanged.\n\nChanging sha1_name.c is the way to go *if* we want all commands to\nsupport this. Just like other ways to name a revision...\n\n> I hope that you do not mind this side-effect, but rather, you intended\n> for this to happen, right? More commands will start supporting this\n> shorthand, suddenly.  (such as format-patch, whatchanged, diff to name\n> a very few).\n\n... but: the initial implementation of this '-' shorthand was\nspecial-casing a single command (IIRC, \"git checkout\") for which the\nshorthand was useful.\n\nIn a previous discussion, I made an analogy with \"cd -\" (which is the\nsource of inspiration of this shorthand AFAIK): \"-\" did not magically\nbecome \"the last visited directory\" for all Unix commands, just for\n\"cd\". And in this case, I'm happy with it. For example, I never need\n\"mkdir -\", and I'm happy I can't \"rm -fr -\" by mistake.\n\nSo, it's debatable whether it's a good thing to have all commands\nsupport \"-\". For example, forcing users to explicitly type \"git branch\n-d @{1}\" and not providing them with a shortcut might be a good thing.\n\nI don't have strong opinion on this: I tend to favor consistency and\nsupporting \"-\" everywhere goes in this direction, but I think the\ndownsides should be considered too. A large part of the exercice here is\nto write a good commit message!\n\nAnother issue with this is: - is also a common way to say \"use stdin\ninstead of a file\", so before enabling - for \"previous branch\", we need\nto make sure it does not introduce any ambiguity. Git does not seem to\nuse \"- for stdin\" much (most commands able to read from stdin have an\nexplicit --stdin option for that), a quick grep in the docs shows only\n\"git blame --contents -\" which is OK because a revision wouldn't make\nsense here anyway.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"311127","messageId":"CAN-3QhoZN_wYvqbVdU_c1h4vUOaT5FOBFL7k+FemNpqkxjWDDA@mail.gmail.com","threadId":"45053","inReplyTo":"vpqh944eof7.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-08T17:23:05Z","receivedAt":"2017-02-09T00:11:25Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hello Matthieu,\n\nOn 8 February 2017 at 20:10, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n> In a previous discussion, I made an analogy with \"cd -\" (which is the\n> source of inspiration of this shorthand AFAIK): \"-\" did not magically\n> become \"the last visited directory\" for all Unix commands, just for\n> \"cd\". And in this case, I'm happy with it. For example, I never need\n> \"mkdir -\", and I'm happy I can't \"rm -fr -\" by mistake.\n>\n> So, it's debatable whether it's a good thing to have all commands\n> support \"-\". For example, forcing users to explicitly type \"git branch\n> -d @{1}\" and not providing them with a shortcut might be a good thing.\n\nbuiltin/branch.c does not call setup_revisions and remains unaffected\nby this patch :)\n\nIn my original patch post, I was not explicit about what files call\nsetup_revisions.\nI would like to rectify that with this (grep-ed) list:\n\n* builtin/blame.c\n* builtin/diff.c\n* builtin/diff-files.c\n* builtin/diff-index.c\n* builtin/diff-tree.c\n* builtin/log.c\n* builtin/rev-list.c\n* builtin/shortlog.c\n* builtin/fast-export.c\n* builtin/fmt-merge-msg.c\nbuiltin/add.c\nbuiltin/checkout.c\nbuiltin/commit.c\nbuiltin/merge.c\nbuiltin/pack-objects.c\nbuiltin/revert.c\n\n* marked commands only show information, and don't change anything.\n\nAs you might notice, in this list, most commands are not of the `rm` variety,\ni.e. something that would delete stuff.\n\nIn the next version of this patch, I will definitely include the list\nof commands\nwhich are \"rm-ish\" and affected by this patch.\n\n>\n> I don't have strong opinion on this: I tend to favor consistency and\n> supporting \"-\" everywhere goes in this direction, but I think the\n> downsides should be considered too. A large part of the exercice here is\n> to write a good commit message!\n\nYes, I prefer consistency very much as well! Having \"-\" mean the same thing\nacross a lot of commands is better than having that shorthand only in a few\ncommands, as it is now. Unless there is a specific confusion that might arise\nbecause of this shorthand inclusion, I think that this shorthand\nshould be supported\nacross the board.\n(I especially like typing `git checkout - <filename>` which is very handy!)\n\n>\n> Another issue with this is: - is also a common way to say \"use stdin\n> instead of a file\", so before enabling - for \"previous branch\", we need\n> to make sure it does not introduce any ambiguity. Git does not seem to\n> use \"- for stdin\" much (most commands able to read from stdin have an\n> explicit --stdin option for that), a quick grep in the docs shows only\n> \"git blame --contents -\" which is OK because a revision wouldn't make\n> sense here anyway.\n\nYes, just to jog your memory, this was discussed here [1]\n\nJunio said:\n\n    As long as the addition is carefully prepared so that we know it\n    will not conflict (or be confused by users) with possible other uses\n    of \"-\", I do not think we would mind \"git branch -D -\" and other\n    commands to learn \"-\" as a synonym for @{-1}.\n\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\nThanks a lot for the review on this patch, Matthieu!\n\n-- \n\nBest Regards,\n\n- Siddharth.\n\n[1]: https://public-inbox.org/git/7vmwpitb6k.fsf@alter.siamese.dyndns.org/\n"},{"id":"311179","messageId":"vpqwpczlfe5.fsf@anie.imag.fr","threadId":"45053","inReplyTo":"CAN-3QhoZN_wYvqbVdU_c1h4vUOaT5FOBFL7k+FemNpqkxjWDDA@mail.gmail.com","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-02-09T12:25:54Z","receivedAt":"2017-02-09T14:23:43Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n\n> Hello Matthieu,\n>\n> On 8 February 2017 at 20:10, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n>> In a previous discussion, I made an analogy with \"cd -\" (which is the\n>> source of inspiration of this shorthand AFAIK): \"-\" did not magically\n>> become \"the last visited directory\" for all Unix commands, just for\n>> \"cd\". And in this case, I'm happy with it. For example, I never need\n>> \"mkdir -\", and I'm happy I can't \"rm -fr -\" by mistake.\n>>\n>> So, it's debatable whether it's a good thing to have all commands\n>> support \"-\". For example, forcing users to explicitly type \"git branch\n>> -d @{1}\" and not providing them with a shortcut might be a good thing.\n>\n> builtin/branch.c does not call setup_revisions and remains unaffected\n> by this patch :)\n\nRight, I forgot this: in some place we need any revspec, but \"branch -d\"\nneeds a branch name explicitly.\n\n> [...]\n> As you might notice, in this list, most commands are not of the `rm` variety,\n> i.e. something that would delete stuff.\n\nOK, I think I'm convinced.\n\nKeep the arguments in mind when polishing the commit message.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"311198","messageId":"CAN-3QhpWQ8qt7Bza=c1v4FkTigW127sqFc7qj_m3_tQ0vfbbxA@mail.gmail.com","threadId":"45053","inReplyTo":"vpqwpczlfe5.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC] WIP: log: allow \"-\" as a short-hand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-09T18:21:37Z","receivedAt":"2017-02-09T18:32:55Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"On 9 February 2017 at 17:55, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n>\n>> [...]\n>> As you might notice, in this list, most commands are not of the `rm` variety,\n>> i.e. something that would delete stuff.\n>\n> OK, I think I'm convinced.\n\nI am glad! :)\n\n>\n> Keep the arguments in mind when polishing the commit message.\n\nI will definitely do that. I am working on a good commit message for\nthis by looking at some past changes to sha1_name.c which have\naffected multiple commands.\n\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\n-- \n\nBest Regards,\n\n- Siddharth.\n"}]}