git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Miriam R." <mirucam@gmail.com>, git <git@vger.kernel.org>
Subject: Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
Date: Thu, 24 Sep 2020 12:46:16 +0200	[thread overview]
Message-ID: <CAP8UFD3rLTLkbq+gYPSD3dmGituWr1g8ZtQDQmPYDdEBtHkU0Q@mail.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2009240952300.5061@tvgsbejvaqbjf.bet>

Hi Dscho,

On Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Thu, 24 Sep 2020, Christian Couder wrote:
>
> > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:

> > > Instead, those `eval` calls are required because the arguments are
> > > provided in quoted form. For example, during the execution of t6030.68,
> > > the `eval` would expand the call
> > >
> > >         eval "git bisect--helper --bisect-start $rev $tail"
> > >
> > > to
> > >
> > >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
> >
> > Yeah, that was also what I found (along with the bug I sent a patch for).
>
> I suspected that you had found that out, but I really wanted a record on
> the Git mailing list about our findings.
>
> It might be a good idea to add a paragraph to the respective patches,
> along these lines:
>
>         Note that the `eval` in the changed line of `git-bisect.sh` cannot be
>         dropped: it is necessary because the `rev` and the `tail`
>         variables may contain multiple, quoted arguments that need to be
>         passed to `bisect--helper` (without the quotes, naturally).

Yeah, sure. Hopefully Miriam will send this in the commit message of
the right patch which is in the subset of the patch series she hasn't
sent.

> > > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > > I had originally suggested to remove, for the same reason).
> > >
> > > I would still recommend appending `|| exit`, even if it just so happens
> > > that we will eventually abort when the `bisect--helper` command failed
> > > anyway, because the next command will then fail, and abort. But it's
> > > cleaner to abort already when this invocation failed rather than relying
> > > on that side effect.
> >
> > Yeah, I think it's a good solution.
>
> Excellent. I think we can actually move forward with the entire patch
> series now, not just the first subset, right?

Yeah, I think so too.

Thanks,
Christian.

  reply	other threads:[~2020-09-24 10:46 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 03/13] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
2020-09-03  5:50   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()' Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
2020-09-03  9:48   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
2020-09-03  9:56   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 08/13] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 09/13] bisect--helper: retire `--next-all` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
2020-09-03 12:03   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 11/13] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 12/13] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 13/13] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
2020-09-03 12:04 ` [PATCH v7 00/13] Finish converting git bisect to C part 2 Johannes Schindelin
2020-09-23  7:26   ` Miriam R.
2020-09-23 14:48     ` Johannes Schindelin
2020-09-23 21:23       ` Johannes Schindelin
2020-09-24  5:33         ` Christian Couder
2020-09-24  7:56           ` Johannes Schindelin
2020-09-24 10:46             ` Christian Couder [this message]
2020-09-24 12:53               ` Miriam R.
2020-09-24 12:56         ` Miriam R.

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

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

  git send-email \
    --in-reply-to=CAP8UFD3rLTLkbq+gYPSD3dmGituWr1g8ZtQDQmPYDdEBtHkU0Q@mail.gmail.com \
    --to=christian.couder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=mirucam@gmail.com \
    /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. 
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 NNTP newsgroup(s).