patch, 3 partsgit-bundle: die if a given ref is not included in bundle

The earlier patch tried to be nice by just warning, but it seems more likely that the user wants to adjust the parameters.

Also, it prevents a bundle containing _all_ revisions in the case when the user only gave one ref, but also rev-list options which excluded the ref.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	It really was a dumb idea not to error out there. Sorry.

 builtin-bundle.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

Johannes Schindelin wrote:

This patch makes git-bundle greatly complicates one of my primary uses: a nightly update generated and emailed to other users. With this, I need to write code to separately explore each branch in the repository to find what changed, then include only those. Without, I can just do git bundle ... <list of refs> and those which have been updated get included.

So, I would really like an option to error out only if the bundle would be empty.

Mark

Mark Levedahl <mlevedahl@gmail.com> writes:

I am not sure if the above is true. How are you preventing the command from bundling everything? You must have some limiter at the bottom, something like --since=25.hours (to account for cron schedule skew), not just <list of refs>.

I do not think the max-age limiter works as you are expecting, especially when merges and clock skew among committing machines are involved (just as you are distributing from central repo, I am assuming you are getting work back from worker-bee machines to the central repo at some point, and doing sneakernet implies to me that they are disconnected machines, not running ntp, but this is just me guessing).

In any case, the semantics of --since=25.hours limiter is not "show everything newer than 25.hours that are reachable from any of these refs"; it is "start digging from these tips, and stop exploring the path as soon as you hit something that is newer than 25.hours".

It appears that for the past few days we have been spending significant effort to make --max-count and --max-age work with bundles, but my honest impression is they do not play well in the real world, especially when clock skew is involved. On the other hand, revision ranges ("include these, exclude those") are always precise, and that is what you would want to be using, especially from an automated script.

If I were doing a nightly script, I would probably be doing something like this:

	#!/bin/sh
	yesterday=$(git bundle list-heads yesterday.bdl | sed -e 's/ .*//')
	git bundle create today.bdl --all --not $yesterday
	# mail it out

After sending today's bundle out, you will rotate it out to yesterday.bdl in order to prepare for the next round. It is likely that you would want to keep a few day's worth of bundles for other reasons _anyway_ (say, some project members might be out of office, or mail gets dropped, or whatever), I think the above is a reasonably clean and easy thing to arrange.

Junio C Hamano wrote:

Sorry, I trimmed too much. My typical command usage is something like
    git bundle create foo --since=10.days.ago --all

I include a very generous date range so that folks don't have to update daily, can cover a vacation with a single bundle, etc. I think this usage renders moot all practical issues with clock skew. I am being loose, if the bundle won't apply then get one from the previous week, apply, and go forward.

BTW, shouldn't git check for clock skew when creating a commit to assure the parents predate the child? Clock skew could allow this circumstance which would look suspicious when exploring a history.

I presume you mean "older than 2.5" hours in the above.

This is certainly a reliable method, but has the difficulty of how to get started and of course requires that history be kept for the entire range covered by each bundle. The first bundle is either a) the entire repository, or b) crafted by trying each and every ref to find which are legal to define. While all of this can be done, I think this cure is worse than the disease (seconds to minutes of clock skew).

I would prefer to just add a warning to the manual that when limiting by date, be generous to allow for all possible clock skew across the distributed set of computers.

Mark

Junio C Hamano wrote:

Thinking about this further, the above has a problem (or should, but see below). Consider a case where master is not updated since yesterday. Effectively, the above becomes

git bundle create today.bdl  master <other-refs> --not master <other-refs>

As ref master is excluded, the bundle creation should die because master cannot be included. Experimenting with next (299fcfbdcb5afd85) however, I get:

git>git bundle create t.bdl master --not master Generating pack... Done counting 0 objects. Writing 0 objects. Total 0 (delta 0), reused 0 (delta 0) git>git ls-remote t.bdl git>

e.g, an empty bundle is created without any error or warning. This is the one case I believe an error should result: there is no use to sending (or even creating) an empty bundle.

As a date limited bundle containing all updated refs is my basic use, I really want this case to not be hard, and it definitely should not require externally maintained history or scripting to create. Absent the "die if any ref wasn't updated in the given date range" logic, and adding always die if the resulting bundle is empty, git bundle in next accomplishes what I want.

Mark

A use case for git-bundle expected to be quite common is this:

	$ git bundle create daily.bundle --since=10.days.ago --all

The expected outcome is _not_ to error out if only a couple of the refs were not changed during the last 10 days.

This patch complains loudly about refs which are skipped due to the pack not containing the corresponding objects, but dies only if no objects would be in the pack _at all_.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

	On Fri, 9 Mar 2007, Mark Levedahl wrote:

	> Junio C Hamano wrote:
	> > If I were doing a nightly script, I would probably be doing
	> > something like this:
	> > 
	> > 	#!/bin/sh
	> > 	yesterday=$(git bundle list-heads yesterday.bdl | sed -e 's/ .*//')
	> > 	git bundle create today.bdl --all --not $yesterday
	> > 	# mail it out
	>
	> Thinking about this further, the above has a problem (or should, 
	> but see below). [...]

	I see another problem, too: if at least one ref was not updated 
	since yesterday, "create" would fail with the latest patches.

	This fixes it.

	BTW I had a little laugh when seeing what git-describe made of my 
	current version :-)

 builtin-bundle.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

Johannes Schindelin wrote:

This patch works for me... Thanks.

Mark

Mark Levedahl <mlevedahl@gmail.com> writes:

I agree that erroring on an empty output is a sensible _option_ just like pack-objects has --no-empty option.

The above is actually an interesting example in a different sense. When somebody did the following, what should be output?

	$ edit; git commit -a ;# on master
        $ git checkout -b side
        $ edit; git commit -a ;# on side
        $ git bundle create foo.bdl master side ^master
	$ git bundle verify foo.bdl

My answer is that it should list master and side as the available heads and master itself as also a prerequisite (which is not what the current code does).

I think unbundling foo.bdl should be the moral equivalent of fetching from the originating repository by somebody who has its prerequisites as tips of some branches. So I think foo.bdl should list refs/heads/master as one of the available heads to fetch/pull from, while requiring the same commit as prerequisite of the bundle. It is as if you tried "git fetch" and found out that you are up to date. Listing where the 'master' tip is, even though you did not have to include any commits from that branch, gives you a useful bit of information ("I am up to date with respect to that branch").

So I think if you did this instead in the above sequence: 

	$ git bundle create foo.bdl master ^master

it would be sensible to have an option to error out because of empty pack, but at the same time it would equally be sensible to have an option to still create a bundle with an empty pack contents. In either case, the head and prerequisite section should include the tip of the master. Earlier, I said we should error out if we do not find 'master' in the list of shown objects, but I think it is more sensible to add it to both the list of head _and_ prerequisites.

Junio C Hamano wrote:

So, practically speaking the proposal is that for each ref where git-bundle currently issues a warning, instead the ref should be added to the prerequisites list, and that all refs given on the command line are in the bundle's defined refs.

Sounds reasonable to me.

Mark

Hi,

On Fri, 9 Mar 2007, Junio C Hamano wrote:

IMHO saying "master ^master" should blow into the user's face. If she says "I want it" _and_ "I don't want it", she should sorta expect it not to work.

Ciao, Dscho

Johannes Schindelin wrote:

The command
    git-bundle create foo next ^master

is legitimate, even if next points to the same commit as master. The current logic would reject this, and should not as we might want to push out the base of a new development branch in this manner. Consider that git-fetch <url> would happily update next in this case, git bundle / git-fetch should as well.

I think we should think of the git-bundle command as accepting two lists of rev-args 1 - the list of heads to define in the bundle (possibly --all, should also accept refs/heads/*) 2 - the list of commits to require as prerequisites for applying the bundle (possibly defined as --since=, possibly defined as a list of commits, etc).

As long as the lists are syntactically acceptable (all exist), we should just create the bundle with the given refs and prerequisites. The resulting bundle will apply cleanly and conforms to general git semantics. So, I think git-bundle should error out only if a ref does not exist or if no refs are defined. An empty pack file is legitimate.

Mark

Hi,

On Sat, 10 Mar 2007, Mark Levedahl wrote:

So, what do you do if some of your users do, and some others do not, have the "blue-sky" branch? If you say "git bundle create new.bundle --all -10", your bundle will list "blue-sky" as a prerequisite.

Boom.

Some of your users -- those without "blue-sky" -- will _not_ be able to fetch _anything_ from the bundle. They are lacking the prerequisites.

The semantics of git-bundle used to be so clear and sensible, since they exactly reflected what git-pack-objects would do.

Now they are no longer?

Ciao, Dscho

Johannes Schindelin wrote:

Those who have the prerequisites can apply the bundle. Those who do not, cannot. This is unchanged, and completely unrelated to whether the bundle defines 0 objects or 10,000. If you do not have the prerequisites, you need a different bundle.

Mark

Hi,

On Sat, 10 Mar 2007, Mark Levedahl wrote:

Only that I suspect that you want to stick more than one ref into the bundle. And at some point you -- or any other user -- will expect the bundle to work _with several refs_, even if different recipients will pick _only one_ of them.

Basically, I am saying that this whole bundle concept is not thought through, that it is too loosely defined, and that it will result in unmet expectations sooner or later. (Which usually means sooner.)

So, either we have to rethink how to handle prerequisites (so that only those are checked which are strictly necessary for _the one_ ref you are updating), or we have to make it _very_ obvious to (human) users of git-bundle that you should _not_ bundle two unrelated -- or only remotely related -- refs into one bundle.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Earlier I thought you said that bundle had a clearly defined semantics, which I did not quite understand, but now you are agreeing with me...

I've been wondering if we can define prereqs per listed head.

Hi,

On Sat, 10 Mar 2007, Junio C Hamano wrote:

git-bundle (as is in "next") has clearly defined semantics.

But the bundle concept is not thought through. Obviously, the clearly defined semantics of git-bundle do not match what people want to use bundles for.

That is a substantial change, and it makes creating a bundle potentially very expensive. We would have to find out reachability for every boundary commit from each ref.

Granted, we could do that while finding the boundary objects, but we only have 28-16 bits left to store from which ref the commit is reachable, which means only 12 refs, and if you have more (which is likely when you say "--all" and have a reasonable number of tags) you need to allocate a bitfield for the remaining refs _for each_ traversed commit's parents.

So I wonder if there is not a better way.

Ciao, Dscho

Johannes Schindelin wrote:

git-bundle on next with the patch in <Pine.LNX.4.63.0703091726530.22628@wbgn013.biozentrum.uni-wuerzburg.de> works well enough for me, but absent that latter patch is too punishing.

Currently, a bundle has a single pack. The bundle's prerequisites are that pack's dependencies. Splitting prerequisites per head requires either creating one pack per head or unpacking at the receiving and and extracting only the objects needed for the selected head. I'm not sure that either is warranted, and my uses of bundle do not require this regardless.

I think we need to restate purpose here. git-bundle is an alternate transport mechanism: git-bundle + git-fetch over sneakernet allows doing what git-push or git-fetch can do when directly connected. However, there are limitations due to the lack of direct connect, specifically the user of git-bundle needs to specify the prerequisites as the protocol cannot negotiate these. The exchange needs to be robust in that git-bundle+git-fetch must never result in leaving a repository in a corrupted state: the current prerequisites list + use of git-fetch seem to satisfy this.

 From a given connected repo, I can do:

    git fetch -f <source url> master:master

and nothing complains, even if no update occurs (remote master is up to 
date). I can also do
        git fetch -f <source url> master:next

and the new ref is created without complaint even if no new objects need to be defined or if the new definition is completely unrelated to the old.

With appropriate remote settings in .git/config, I can have git-fetch get all branches, or all branches and tags, and never complain when no update is required for something.

What I desire is similar functionality across sneakernet, and this is where git-bundle steps in. I cannot know what is on the destination repository exactly, so I need an imprecise way to specify prerequisites (e.g., --since=10.days.ago): I *know* that this is not exactly correct and *must* be conservative so that the bundle likely can be used. As the system is distributed and I don't control the recipients of the bundle, there is *no* way to know exactly what exists, the previous bundle is not definitive, it might not have been applied, they might have received data by a side-channel communication, etc.

So, a date range is the best method I have found to specify a bundle's prerequisites. However, I should not have to know which refs have been updated within the date range: this is too punishing. Directly connected git-fetch does not abort when the refspec says "get everything" but some ref in "everything" has not changed, why should git-bundle complain? Absent the latest patch (i.e., what is now on next), git-bundle will error out which is extremely unfriendly and unhelpful.

The single disconnect for the above with latest git-bundle + patch is that we cannot package a ref whose commit object is directly a bundle prerequisite. (I cannot do the equivalent of "git fetch remote master:next", where I already have all the objects for master). These instead result in a string of warning messages with the latest patch: I can live with this limitation (though I don't think this should even be a warning, git-fetch/git-push do not warn here). Absent the latest patch, git-bundle errors out: this is too punishing to the user.

While it is possible to fetch a particular ref from the bundle rather than taking all, the monolithic pack structure and protocol dictates that you will get all objects regardless. I do not see this as a problem: the bundle came from a single repository, everything in the bundle is therefore related, excess is easily trimmed by git-repack. This is really just a limitation of the disconnected protocol that cannot optimize the pack for the exact transfer required.

At some point, we have to make a clear distinction between what rules the protocol should enforce for "correctness" vs what an "intelligent" use of bundle is, and not try to enforce the latter in the software. What practices are useful or good vary considerably from business to business (I have many times been told that things I find essential to my work are "bad practice," usually stated by people who didn't have to solve a problem given constraints I actually face). The only requests git-bundle/git-fetch should refuse are things that will corrupt a git-repository, and the pair should endeavor to enable any information transfer that can be done with git-push or git-fetch given direct connections.

Bottom line, I strongly advocate Dscho's last patch + what is on next be promoted to master. We can revisit how well it is working and refine it after it gets some usage from others defining additional use-cases.

Mark

Mark Levedahl <mlevedahl@gmail.com> writes:

I agree that is a very sensible thing to do.  Let's do that.

Hi,

On Sun, 11 Mar 2007, Mark Levedahl wrote:

I don't want to punish you! But I want to prevent "Huh?" moments early on.

You can just store the pack and update the ref (at least as long as the pack is not thin): we have a long-standing tradition of guaranteeing the integrity of commit chains _only_ when the commits are reachable by at least one ref.

But would that not mean that we expect the user of the bundle to update _all_ refs contained in the bundle?

Yes. I think we definitely need to ensure the integrity _before_ updating the refs (and not rely on the prereqs in the bundle). And yes, that means I changed my mind from when I argued against your use of fsck, and _for_ prereqs.

Makes sense.

... but a subsequent git-gc will strip them away.

Concur. But I think that we have to make sure that the "correctness" enforcing means that you do not end up with something that people want to use for what it cannot be used for.

FWIW my plans are to make the pack thin _only_ when there is only one prereq and/or ref in the bundle (this prevents a _wanted_ object being deltified against a not-wanted object).

Also, as mentioned above, I think that we have to check that "git rev-list --objects <new-refs> --not --all" does not result in missing objects.

Ciao, Dscho

Johannes Schindelin wrote:

I am not sure that this is really necessary or accomplishes additional safety. The prerequisites must exist and be well connected in the target repo before the pack file is indexed: presumably, the reference objects all exist if the checks hold, or there is a logic flaw in the thin-pack generation.

If the prereq test is removed, then avoiding a thin pack might allow the pack file to be applied to a repo that held only the prereqs for a single head out of many in the bundle, but there is no info for the user to understand how or when to do this and I don't really think that is a good practice to encourage. I suggest waiting for a well defined use-case that really demands being able to apply only part of a pack file before implementing.

This is certainly a good safety check: even though the prereqs are satisfied and all *should* be ok, some error might still exist and it is better to be safe.

A common use of bundle is to schedule nightly emailed updates to a remote system containing all updates made over a given period. We want bundle to complain only if the resulting bundle would be empty, but not if at least one of the given refs has an update resulting in a non-empty bundle.

Signed-off-by: Mark Levedahl <mdl123@verizon.net>
---
 builtin-bundle.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)