Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them

From: Elijah Newren <newren@gmail.com>
Date: May 27, 2019, 23:27 UTC
Message-ID: <CABPp-BFkqyGT_YHReKPJKy-nX5eWYhb9AVE2UeD7M+nhKT0dqg@mail.gmail.com>
In-Reply-To: <CAH_=xoZ5Lt2nn50cfDbTA-ZVeshi8HZ91Od4btuUXpBGCanGng@mail.gmail.com>

Hi Eric,

On Mon, May 27, 2019 at 3:46 PM Eric Rannaud <e@nanocritical.com> wrote:

Show 7 quoted lines

> On Mon, May 27, 2019 at 2:12 PM Elijah Newren <newren@gmail.com> wrote:
> > On Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:
>
> I think the test will not fail without the patch (and therefore won't
> catch a regression): while checkpoint doesn't currently check the
> failure flag, it will be checked when cmd_main() returns so
> exit_status will be 1.

Ooh, good catch.

Show 8 quoted lines

> You either want to:
> - (a) add more independent commands after the checkpoint and check
> that they were not run,
> - or (b) run with --done, do not include a done command, and check
> that fast-import does exit (but it's racy),
> - or (c) you can reuse background_import to have a more explicit
> sequence of events (in which case improvements to background_import
> from my patch would have to be committed first).

That sounds good...though it's taking my short patch and just about amounts to completely rewriting it. Would you like to take it over including authorship, and just add either a "Original-patch-by:" or "Based-on-patch-by:" for me in the commit message (these two tags appear to be the two most common attribution mechanism used in git.git's history when someone does this)?

Show 8 quoted lines

> > +       cat >input <<-INPUT_END &&
> > +       feature done
> > +       commit refs/heads/V3
> > +       mark :3
> > +       committer Me My <self@and.eye> 1234567890 +0123
>
> You likely want to use:
> committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE

I see other tests in that testsuite using this, and using it here certainly wouldn't hurt; I'm not opposed to it. But I'm curious...other than "other tests in the same testcase use it a lot" I don't see why the choice of committer name/email/date matters at all. Is there an actual reason for this that I just missed?

Cheers, Elijah