Re: [PATCH 06/10] fast-export: when using paths, avoid corrupt stream with non-existent mark

From: Elijah Newren <newren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	Lars Schneider <larsxschneider@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Taylor Blau <me@ttaylorr.com>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH 06/10] fast-export: when using paths, avoid corrupt stream with non-existent mark
Date: Mon, 12 Nov 2018 07:36:36 -0800	[thread overview]
Message-ID: <CABPp-BHLEtXe-2OTHNxHe=vypvbd-kFQ3G1FaVGnQ-Gc4+z1uA@mail.gmail.com> (raw)
In-Reply-To: <20181112124547.GG3956@sigill.intra.peff.net>

On Mon, Nov 12, 2018 at 4:45 AM Jeff King <peff@peff.net> wrote:
> On Sun, Nov 11, 2018 at 12:01:43AM -0800, Elijah Newren wrote:
>
> > > It does seem funny that the behavior for the earlier case (bounded
> > > commits) and this case (skipping some commits) are different. Would you
> > > ever want to keep walking backwards to find an ancestor in the earlier
> > > case? Or vice versa, would you ever want to simply delete a tag in a
> > > case like this one?
> > >
> > > I'm not sure sure, but I suspect you may have thought about it a lot
> > > harder than I have. :)
> >
> > I'm not sure why you thought the behavior for the two cases was
> > different?  For both patches, my testcases used path limiting; it was
> > you who suggested employing a negative revision to bound the commits.
>
> Sorry, I think I just got confused. I was thinking about the
> documentation fixup you started with, which did regard bounded commits.
> But that's not relevant here.
>
> > Anyway, for both patches assuming you haven't bounded the commits, you
> > can attempt to keep walking backwards to find an earlier ancestor, but
> > the fundamental fact is you aren't guaranteed that you can find one
> > (i.e. some tag or branch points to a commit that didn't modify any of
> > the specified paths, and nor did any of its ancestors back to any root
> > commits).  I hit that case lots of times.  If the user explicitly
> > requested a tag or branch for export (and requested tag rewriting),
> > and limited to certain paths that had never existed in the repository
> > as of the time of the tag or branch, then you hit the cases these
> > patches worry about.  Patch 4 was about (annotated and signed) tags,
> > this patch is about unannotated tags and branches and other refs.
>
> OK, that makes more sense.
>
> So I guess my question is: in patch 4, why do we not walk back to find
> an appropriate ancestor pointed to by the signed tag object, as we do
> here for the unannotated case?
>
> And I think the answer is: we already do that. It's just that the
> unannotated case never learned the same trick. So basically it's:
>
>   1. rewriting annotated tags to ancestors is already known on "master"
>
>   2. patch 4 further teaches it to drop a tag when that fails
>
>   3. patch 6 teaches both (1) and (2) to the unannotated code path,
>      which knew neither
>
> Is that right?

Ah, now I see where the slight disconnect was.  And yes, you are correct.

> > > This hunk makes sense.
> >
> > Cool, this was the entirety of the code...so does this mean that the
> > code makes more sense than my commit message summary did?  ...and
> > perhaps that my attempts to answer your questions in this email
> > weren't necessary anymore?
>
> No, it only made sense that the hunk implemented what you claimed in the
> commit message. ;)
>
> I think your responses did help me understand that what the commit
> message is claiming is a good thing.