git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Pranit Bauva <pranit.bauva@gmail.com>
To: Siddharth Kannan <kannan.siddharth12@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Matthieu.Moy@imag.fr, Jeff King <peff@peff.net>,
	Junio C Hamano <gitster@pobox.com>,
	Duy Nguyen <pclouds@gmail.com>,
	"Brian M. Carlson" <sandals@crustytoothpaste.ath.cx>
Subject: Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
Date: Sun, 5 Feb 2017 20:25:25 +0530	[thread overview]
Message-ID: <CAFZEwPOFPPyui=9mnccbOc-79q0URYhdGHSkcd0YyR6qe-c_zQ@mail.gmail.com> (raw)
In-Reply-To: <1486299439-2859-1-git-send-email-kannan.siddharth12@gmail.com>

Hey Siddharth,

On Sun, Feb 5, 2017 at 6:27 PM, Siddharth Kannan
<kannan.siddharth12@gmail.com> wrote:
> Search and replace "-" (written in the context of a branch name) in the argument
> list with "@{-1}". As per the help text of git rev-list, this includes the following four
> cases:
>
>   a. "-"
>   b. "^-"
>   c. "-..other-branch-name" or "other-branch-name..-"
>   d. "-...other-branch-name" or "other-branch-name...-"
>
> (a) and (b) have been implemented as in the previous implementations of
> this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add
> docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as
> abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a
> short-hand for "previous branch", 2011-04-07)
>
> (c) and (d) have been implemented by using the strbuf API, growing it to the
> right size and placing "@{-1}" instead of "-"
>
> Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
> ---
> This is a patch for one of the microprojects of SoC 2017. [1]
>
> I have implemented this using multiple methods, that I have re-written again and
> again for better versions ([2]). The present version I feel is the closest that
> I could get to the existing code in the repository. This patch only uses
> functions that are commonly used in the rest of the codebase.
>
> I still have to write tests, as well as update documentation as done in 696acf45
> (checkout: implement "-" abbreviation, add docs and tests, 2009-01-17).
>
> I request your comments on this patch. Also, I have the following questions
> regarding this patch:
>
> 1. Is the approach that I have used to solve this problem fine?
> 2. Is the code I am writing in the right function? (I have put it right
> before the revisions data structure is setup, thus these changes affect only
> git-log)
>
> [1]: https://git.github.io/SoC-2017-Microprojects/
> [2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses
> strbufs for the starting 4 characters, and last 4 characters and compares those
> to the appropriate strings for case (c) and case (d). I edited this patch to use
> strstr instead, which avoids all the strbuf declarations)
>
>  builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 55d20cc..a5aac99 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>                          struct rev_info *rev, struct setup_revision_opt *opt)
>  {
>         struct userformat_want w;
> -       int quiet = 0, source = 0, mailmap = 0;
> +       int quiet = 0, source = 0, mailmap = 0, i = 0;
>         static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
>
>         const struct option builtin_log_options[] = {
> @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>
>         if (quiet)
>                 rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
> +
> +       /*
> +        * Check if any argument has a "-" in it, which has been referred to as a
> +        * shorthand for @{-1}.  Handles methods that might be used to list commits
> +        * as mentioned in git rev-list --help
> +        */
> +
> +       for(i = 0; i < argc; ++i) {
> +               if (!strcmp(argv[i], "-")) {
> +                       argv[i] = "@{-1}";
> +               } else if (!strcmp(argv[i], "^-")) {
> +                       argv[i] = "^@{-1}";
> +               } else if (strlen(argv[i]) >= 4) {
> +
> +                       if (strstr(argv[i], "-...") == argv[i] || strstr(argv[i], "-..") == argv[i]) {
> +                               struct strbuf changed_argument = STRBUF_INIT;
> +
> +                               strbuf_addstr(&changed_argument, "@{-1}");
> +                               strbuf_addstr(&changed_argument, argv[i] + 1);
> +
> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
> +
> +                               argv[i] = strbuf_detach(&changed_argument, NULL);
> +                       }
> +
> +                       /*
> +                        * Find the first occurence, and add the size to it and proceed if
> +                        * the resulting value is NULL
> +                        */
> +                       if (!(strstr(argv[i], "...-") + 4)  ||
> +                                       !(strstr(argv[i], "..-") + 3)) {
> +                               struct strbuf changed_argument = STRBUF_INIT;
> +
> +                               strbuf_addstr(&changed_argument, argv[i]);
> +
> +                               strbuf_grow(&changed_argument, strlen(argv[i]) + 4);
> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
> +
> +                               strbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, "@{-1}", 5);
> +
> +                               argv[i] = strbuf_detach(&changed_argument, NULL);
> +                       }
> +               }
> +       }
> +
>         argc = setup_revisions(argc, argv, rev, opt);
>
>         /* Any arguments at this point are not recognized */
> --


It is highly recommended to follow the pre existing style of code and
commits. In the micro project list, I think it is mentioned that this
similar thing is implemented in git-merge so you should try and dig
the commit history of that file to find the similar change.

If you do this, then you will find out that there is a very short and
sweet way to do it. I won't directly point out the commit.

strbuf API should be used when you need to modify the contents of the
string. I think you have a little confusion.

If you declare the string as,

const char *str = "foo";

then, you can also do,

str = "bar";

But you can't do,

str[1] = 'z';

I hope you get what I am saying, if not, search for it.

Regards,
Pranit Bauva

  reply	other threads:[~2017-02-05 14:55 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-05 12:57 Siddharth Kannan
2017-02-05 14:55 ` Pranit Bauva [this message]
2017-02-06  0:15 ` Junio C Hamano
2017-02-06  2:27   ` Siddharth Kannan
2017-02-06 18:10   ` Siddharth Kannan
2017-02-06 23:09     ` Junio C Hamano
2017-02-07 19:14       ` Siddharth Kannan
2017-02-08 14:40         ` Matthieu Moy
2017-02-08 17:23           ` Siddharth Kannan
2017-02-09 12:25             ` Matthieu Moy
2017-02-09 18:21               ` Siddharth Kannan

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFZEwPOFPPyui=9mnccbOc-79q0URYhdGHSkcd0YyR6qe-c_zQ@mail.gmail.com' \
    --to=pranit.bauva@gmail.com \
    --cc=Matthieu.Moy@imag.fr \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kannan.siddharth12@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.ath.cx \
    --subject='Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).