For now, don't "bup get" more than one target in a single command

[Rob Browning]

While using get --repair to repair my own repositories I just discovered
a bug that causes it to omit intermediate commits from the final ref.
They're in the repo, but just not parents of the final commit (and so
also candidates for removal by a gc if they're not referred to
elsewhere).

My current plan is to make a 0.34.1 release very soon (and
0.33.11 if it's also affected) that rejects multiple targets, and then
fix the underlying issue in a subsequent release.

For example, don't:

bup get -s SRC ... --pick X --pick Y

Instead:

bup get -s SRC ... --pick X
bup get -s SRC ... --pick Y

Though this will be much slower than the combined command if you're
repairing.

--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Greg Troxel]

Rob Browning <r...@defaultvalue.org> writes:

> While using get --repair to repair my own repositories I just discovered
> a bug that causes it to omit intermediate commits from the final ref.
> They're in the repo, but just not parents of the final commit (and so
> also candidates for removal by a gc if they're not referred to
> elsewhere).

I am not understanding.

- Is this with --repair only (and if so, only if actually repair=ed)

- I would expect the most recent commit to have as first-parent the
previous commit on the same ref. Are you saying that the new
latest commit omits that first-parent, and what does it have
instead?

- What is unreferenced? Just a previous commit, or also trees/blobs
referenced by it, if they aren't referenced by latest?

> My current plan is to make a 0.34.1 release very soon (and
> 0.33.11 if it's also affected) that rejects multiple targets, and then
> fix the underlying issue in a subsequent release.

That sounds fine to me.

Would there also be a new test to detect the bug?

> For example, don't:
>
> bup get -s SRC ... --pick X --pick Y
>
> Instead:
>
> bup get -s SRC ... --pick X
> bup get -s SRC ... --pick Y

Does this affect get --rewrite?

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> I am not understanding.
>
> - Is this with --repair only (and if so, only if actually repair=ed)

It's any invocation of "bup get" with multiple targets, i.e. multiple
--picks, --appends, etc. For example, If the destination has a "branch"
HASH and you --pick branch/save-1 --pick branch/save-2 --pick
branch/save-3 from somewhere, I believe it will create three new
commits, each with HASH as the parent, and then update the destination
branch to be the last one. Assuming that's right, the intermediate
commits (and their attached data) are in the repository, but detached.

Internally, this is due to a problem with bup get's attempt to make the
invocation more atomic with respect to refs. Right now it attempts to
defer all ref updates until the end, and only if all operations succeed.

One question we're wondering about right now, is how important is it
that bup get have deferred ref updates. It would be fairly easy to
"fix" the problem by changing get to just update the refs as it goes.

Fixing the problem while preserving the deferral is going to be more
complicated, and has higher risk of getting something wrong now, or in
future changes (because bup has to simulate the ref changes during the
process, to be careful with respect to the VFS (and its caching)), but
it is a nice behavior when you need it. Of course you could also handle
that yourself, outside of bup, by capturing the relevant refs with "git
show-ref ...", and then rolling back on error via "git branch -f ...".

I still have some investigation to do, but if someone were to want a
quick local fix, then putting something like this at the top of
lib/bup/cmd/get.py get_everything() may work:

if len(opt.target_specs > 1):
misuse('only one method allowed per invocation until a bug is fixed')

That will also cause some of the tests to fail.

> That sounds fine to me.
>
> Would there also be a new test to detect the bug?

Definitely. (I'm unpleasantly surprised none of the existing tests
caught this, either intentionally or incidentally.)

> Does this affect get --rewrite?

Yes (at least) any invocation of bup get that should be "chaining" more
than one commit(ish) to a target branch, and unfortunately, the
workaround is likely to make --rewrite/--repair of multiple saves (on
the same or related branches) much slower because the database of
"previous rewrites" won't be available across the saves (it's per bup
invocation).

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> I still have some investigation to do, but if someone were to want a
> quick local fix, then putting something like this at the top of
> lib/bup/cmd/get.py get_everything() may work:
>
> if len(opt.target_specs > 1):
> misuse('only one method allowed per invocation until a bug is fixed')

Or rather "if len(opt.target_specs) > 1:".

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> While using get --repair to repair my own repositories I just discovered
> a bug that causes it to omit intermediate commits from the final ref.
> They're in the repo, but just not parents of the final commit (and so
> also candidates for removal by a gc if they're not referred to
> elsewhere).
>
> My current plan is to make a 0.34.1 release very soon (and
> 0.33.11 if it's also affected) that rejects multiple targets, and then
> fix the underlying issue in a subsequent release.

The patches I just posted for review should actually fix the problem for
main. I also tried backporting them to 0.33.x, and while I didn't
finish, I believe I got about half way thorough the "harder parts", and
so it may be feasible. But I'm not sure whether I think that would be a
good idea, or if we should just reject multiple targets in 0.33.x.

Some places like Debian and its derivatives, people will probably be
running 0.33.x for a long while yet.

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> The patches I just posted for review should actually fix the problem for
> main. I also tried backporting them to 0.33.x, and while I didn't
> finish, I believe I got about half way thorough the "harder parts", and
> so it may be feasible. But I'm not sure whether I think that would be a
> good idea, or if we should just reject multiple targets in 0.33.x.
>
> Some places like Debian and its derivatives, people will probably be
> running 0.33.x for a long while yet.

Oh, and all the candidate changes I posted for 0.34.1 are also here for
anyone who wants to try them:

https://github.com/bup/bup/tree/tmp/review/main
https://codeberg.org/bup/bup/src/branch/tmp/review/main

Thanks

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> I believe I got about half way thorough the "harder parts", and
> so it may be feasible. But I'm not sure whether I think that would be a
> good idea, or if we should just reject multiple targets in 0.33.x.
>
> Some places like Debian and its derivatives, people will probably be
> running 0.33.x for a long while yet.

OK, I have the fixes working fine on 0.33.x now, so we have the option,
but let's settle main before worrying about that.

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> Oh, and all the candidate changes I posted for 0.34.1 are also here for
> anyone who wants to try them:
>
> https://github.com/bup/bup/tree/tmp/review/main
> https://codeberg.org/bup/bup/src/branch/tmp/review/main

Rebased to add the bup get destination prefix checking I just posted,
and a few other minor fixes.

I think that's about everything I had pending for a release. If nothing
comes up, I'll probably push those to main in a while, and then call for
testing for 0.34.1.