git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Pranit Bauva <pranit.bauva@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Christian Couder" <christian.couder@gmail.com>,
	"Christian Couder" <chriscool@tuxfamily.org>,
	"Lars Schneider" <larsxschneider@gmail.com>,
	"Torsten Bögershausen" <tboegi@web.de>
Subject: Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C
Date: Thu, 16 Jun 2016 14:55:01 -0400	[thread overview]
Message-ID: <CAPig+cQV3FTGJBvS0Kuc3CNPiwZMZtApT6r8+Ojhw3y7O2VTiw@mail.gmail.com> (raw)
In-Reply-To: <20160615140026.10519-7-pranit.bauva@gmail.com>

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `bisect_write` shell function in C and add a
> `bisect-write` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
>
> Using `--bisect-write` subcommand is a temporary measure to port shell
> function in C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
> from the global shell script thus we need to pass it to the subcommand
> using the arguments. After the whole conversion, we can remove the extra
> arguments and make the method use the two variables from the global scope
> within the C code.

You could do this now rather than waiting for later. Instead of
passing these arguments to bisect_write(), create global variables in
this patch and assign them in the BISECT_WRITE case of
cmd_bisect__helper() before calling bisect_write().

Not necessarily worth a re-roll, but would save you the effort of
having to explain it here and then change it in some later patch.

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int rev_nr)
> +static int bisect_write(const char *state, const char *rev,
> +                       const char *term_good, const char *term_bad,
> +                       int nolog)
> +{
> +       struct strbuf tag = STRBUF_INIT;
> +       struct strbuf commit_name = STRBUF_INIT;
> +       struct object_id oid;
> +       struct commit *commit;
> +       struct pretty_print_context pp = {0};
> +       FILE *fp;
> +
> +       if (!strcmp(state, term_bad))
> +               strbuf_addf(&tag, "refs/bisect/%s", state);
> +       else if(one_of(state, term_good, "skip", NULL))
> +               strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> +       else
> +               return error(_("Bad bisect_write argument: %s"), state);
> +
> +       if (get_oid(rev, &oid)) {
> +               strbuf_release(&tag);
> +               return error(_("couldn't get the oid of the rev '%s'"), rev);
> +       }

Minor: If you move the get_oid() conditional before the one above it,
then you won't have to worry about releasing 'strbuf tag' at this
point.

> +       if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
> +                      UPDATE_REFS_MSG_ON_ERR)) {
> +               strbuf_release(&tag);
> +               return -1;
> +       }

If you release 'strbuf tag' right here, after it's final use, then you
won't have to worry about releasing it anywhere below (particularly in
the error cases).

> +       fp = fopen(git_path_bisect_log(), "a");
> +       if (!fp) {
> +               strbuf_release(&tag);
> +               return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
> +       }
> +
> +       commit = lookup_commit_reference(oid.hash);
> +       format_commit_message(commit, "%s", &commit_name, &pp);
> +       fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
> +               commit_name.buf);
> +
> +       if (!nolog)
> +               fprintf(fp, "git bisect %s %s\n", state, rev);
> +
> +       strbuf_release(&commit_name);
> +       strbuf_release(&tag);
> +       fclose(fp);
> +       return 0;
> +}
> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> +       case BISECT_WRITE:
> +               if (argc != 4 && argc != 5)
> +                       die(_("--bisect-write requires either 4 or 5 arguments"));
> +               nolog = (argc == 5) && !strcmp(argv[4], "nolog");

This is minor and won't matter in the long run when this code goes
away later in the C conversion, but this differs from the shell code
which only cared that a (non-empty) fifth argument was provided but
didn't care about the actual value, whereas this code expects the
argument to be exactly "nolog".

> +               return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
>         default:
>                 die("BUG: unknown subcommand '%d'", cmdmode);
>         }

  reply	other threads:[~2016-06-16 18:55 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-07 20:54 [PATCH 1/4] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
2016-06-07 20:54 ` [PATCH 2/4] t6030: explicitly test for bisection cleanup Pranit Bauva
2016-06-07 23:21   ` Eric Sunshine
2016-06-08  8:07     ` Pranit Bauva
2016-06-08  8:17       ` Eric Sunshine
2016-06-08 10:20         ` Pranit Bauva
2016-06-07 20:54 ` [PATCH 3/4] dir: introduce file_size() to check the size of file Pranit Bauva
2016-06-08  7:37   ` Eric Sunshine
2016-06-08  7:57     ` Pranit Bauva
2016-06-08  8:13       ` Eric Sunshine
2016-06-08 10:03         ` Christian Couder
2016-06-08  8:17       ` Torsten Bögershausen
2016-06-08 13:08         ` Pranit Bauva
2016-06-12 10:44           ` Torsten Bögershausen
2016-06-13  6:21             ` Pranit Bauva
2016-06-07 20:54 ` [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C Pranit Bauva
2016-06-08  7:59   ` Eric Sunshine
2016-06-08  9:51     ` Christian Couder
2016-06-08  9:53       ` Christian Couder
2016-06-08 17:50       ` Eric Sunshine
2016-06-08 13:20     ` Pranit Bauva
2016-06-08 17:53       ` Eric Sunshine
2016-06-08 18:04         ` Pranit Bauva
2016-06-07 22:31 ` [PATCH 1/4] bisect--helper: `bisect_clean_state` " Eric Sunshine
2016-06-08  1:51   ` Eric Sunshine
2016-06-08  7:46   ` Pranit Bauva
2016-06-08  8:02     ` Eric Sunshine
2016-06-08  8:09       ` Pranit Bauva
2016-06-08  9:41       ` Christian Couder
2016-06-08 17:59         ` Eric Sunshine
2016-06-08 18:04           ` Pranit Bauva
2016-06-15 14:00 ` [PATCH v2 0/6] convert various shell functions in git-bisect to C Pranit Bauva
2016-06-15 14:00   ` [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
2016-06-15 18:04     ` Eric Sunshine
2016-06-15 18:47       ` Pranit Bauva
2016-06-15 20:22         ` Eric Sunshine
2016-06-15 14:00   ` [PATCH v2 2/6] t6030: explicitly test for bisection cleanup Pranit Bauva
2016-06-15 14:00   ` [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c Pranit Bauva
2016-06-15 18:12     ` Junio C Hamano
2016-06-15 18:15       ` Pranit Bauva
2016-06-15 18:22     ` Eric Sunshine
2016-06-15 18:40       ` Pranit Bauva
2016-06-15 14:00   ` [PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C Pranit Bauva
2016-06-15 21:05     ` Eric Sunshine
2016-06-16 19:06       ` Pranit Bauva
2016-06-15 14:00   ` [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` " Pranit Bauva
2016-06-15 21:14     ` Eric Sunshine
2016-06-16 19:05       ` Pranit Bauva
2016-06-16 19:16         ` Eric Sunshine
2016-06-16 19:25           ` Pranit Bauva
2016-06-16 20:47             ` Christian Couder
2016-06-17 12:49               ` Pranit Bauva
2016-06-15 14:00   ` [PATCH v2 6/6] bisect--helper: `bisect_write` " Pranit Bauva
2016-06-16 18:55     ` Eric Sunshine [this message]
2016-06-16 19:01       ` Pranit Bauva
2016-06-16 20:38         ` Christian Couder
2016-06-17 13:10           ` Pranit Bauva
2016-06-15 17:53   ` [PATCH v2 0/6] convert various shell functions in git-bisect to C Eric Sunshine
2016-06-15 18:09     ` Pranit Bauva
2016-06-26 12:23 ` [PATCH v3 " Pranit Bauva
2016-07-06 20:25   ` [PATCH v4 " Pranit Bauva
2016-07-06 20:25     ` [PATCH v4 1/6] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
2016-07-06 20:25     ` [PATCH v4 2/6] t6030: explicitly test for bisection cleanup Pranit Bauva
2016-07-11 19:16       ` Junio C Hamano
2016-07-06 20:25     ` [PATCH v4 3/6] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file() Pranit Bauva
2016-07-06 20:25     ` [PATCH v4 4/6] bisect--helper: `bisect_reset` shell function in C Pranit Bauva
2016-07-06 20:25     ` [PATCH v4 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` " Pranit Bauva
2016-07-06 20:25     ` [PATCH v4 6/6] bisect--helper: `bisect_write` " Pranit Bauva
2016-07-11 19:19     ` [PATCH v4 0/6] convert various shell functions in git-bisect to C Junio C Hamano
2016-06-26 12:23 ` [PATCH v3 1/6] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
2016-06-26 12:23 ` [PATCH v3 2/6] t6030: explicitly test for bisection cleanup Pranit Bauva
2016-06-26 12:23 ` [PATCH v3 3/6] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file() Pranit Bauva
2016-06-26 12:23 ` [PATCH v3 4/6] bisect--helper: `bisect_reset` shell function in C Pranit Bauva
2016-06-26 12:23 ` [PATCH v3 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` " Pranit Bauva
2016-06-26 12:23 ` [PATCH v3 6/6] bisect--helper: `bisect_write` " Pranit Bauva

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=CAPig+cQV3FTGJBvS0Kuc3CNPiwZMZtApT6r8+Ojhw3y7O2VTiw@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=larsxschneider@gmail.com \
    --cc=pranit.bauva@gmail.com \
    --cc=tboegi@web.de \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).