Re: [PATCH] Add git-bundle: move objects and references by archive.
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 19, 2007, 01:07 UTC
- Message-ID
- <Pine.LNX.4.63.0702190126220.22628@wbgn013.biozentrum.uni-wuerzburg.de>
- In-Reply-To
- <1171838852534-git-send-email-mdl123@verizon.net>
Hi,
Sorry to be such a PITA, but I really, really think that it is wrong to make a tar dependency here. You said your cygwin has problems with binary files. Could you please try this:
$ echo -ne '\x1a\x1b\x15\x10\0abc' | cat | wc
If it returns anything else than "<tab>0<tab>1<tab>8", then your setup works differently from mine. I fit returns what I expect it to, then we can use cat directly and do not have to move the tar bloat around (it is inherently block based, so it wastes a lot of space, and it also stores other things we'll never use, like permissions for all files).
On Sun, 18 Feb 2007, Mark Levedahl wrote:
> diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl > old mode 100755 > new mode 100644
This is unintended, right?
Show 15 quoted lines
> diff --git a/git-bundle.sh b/git-bundle.sh > new file mode 100755 > index 0000000..19ac137 > --- /dev/null > +++ b/git-bundle.sh > @@ -0,0 +1,174 @@ > +#!/bin/sh > +# Basic handler for bundle files to connect repositories via sneakernet. > +# Invocation must include action. > +# This function can create a bundle or provide information on an existing bundle > +# supporting git-fetch, git-pull, and git-ls-remote > + > +USAGE='[--create bundle <git-rev-list args>] | > +[--verify|--list-heads|--unbundle bundle] <--tar tarspec> > + where bundle is the name of the bundle file.'
We seem to prefer another (more consistent?) way to describe things: If the argument is not an option, we embrace it with "<>", for example:
<bundle>
if it is an optional option, we put it in "[]", for example:
[--tar <tarfile>]
and if one of several mutual exclusive options has to be passed, we put it in "()" delimited with "|". Also, we put no explanation in the output of "-h", since you should read the man page if you don't know what the options mean. So, it is
git-bundle ( --create <bundle> [<rev-list-args>...] | --verify | --list-heads | --unbundle <bundle> ) [--tar <tarspec>]
> +verify() {This function is nicely done, AFAICT!
Show 5 quoted lines
> +list_heads() {
> + if test -z "$*" ; then
> + $TAR -xf "$bfile" -O references 2>/dev/null || exit 1
> + else
> + ($TAR -xf "$bfile" -O references 2>/dev/null || exit 1) |AFAICT this will not work as expected: "()" opens a subshell, and "exit 1" will only exit that subshell. Correct me if I'm wrong.
Show 10 quoted lines
> + while read sha1 ref ; do
> + for arg in $* ; do
> + if test "${ref%$arg}" != "$ref" ; then
> + echo "$sha1 $ref"
> + break
> + fi
> + done
> + done
> + fi
> +}The complexity here is unnecessarily high (O(N*M)), but I guess we cannot do better in shell. Maybe constructing a regexp from the args, and piping the output from tar through a grep would help here. Dunno. Maybe it doesn't matter much in reality, anyway.
> +SUBDIRECTORY_OK=1 > +. git-sh-setup
We tend to write this part at the very top of a git script.
Show 6 quoted lines
> +while test -n "$1" ; do
> + case $1 in
> + --create|--list-heads|--unbundle|--verify)
> + action=${1#--}
> + shift
> + bfile=$1This can contain spaces (you are working on Windows, right? Windows users _love_ spaces in their filenames).
Show 8 quoted lines
> + test -z "$bfile" && die "$action requires filename";;
> + --tar=*)
> + TAR=${1##--tar=};;
> + --tar)
> + shift
> + TAR=$1;;
> + *)
> + args="$args $1";;I know you want to mix the --tar option with the rev-list options (which I still find totally confusing, and unnecessary at best), but you _have_ to quote "$1" in the args=... part, then. Even refs can contain spaces these days.
> +# what tar to use, prefer gtar, then tar. > +if test -z "$TAR" ; then > + GTAR=$(which gtar 2>/dev/null)
Somebody on this list said that "which" should not be relied upon IIRC. I forgot why, but I obey that hint.
> + gitrevargs=$(git-rev-parse --symbolic --revs-only $args) || exit 1
Here, you rely again on the refs not containing spaces.
> + # git-rev-list cannot determine edge objects if a date restriction is > + # given... we do things a slow way if max-age or min-age are given
Might make sense to teach max-age about boundary commits instead...
> + prereqs=$(git-rev-list --objects-edge $fullrevargs | sed -ne 's/^-//p');;
Since you are not interested in non-commits at all, why not use "--boundary" instead?
> + # create refs and pack > + tmp="$GIT_DIR/bundle_tmp$$"
Should we really rely on $GIT_DIR being writable? For unbundling, yes, but for bundling? Given the great confusion with git-status trying to write into $GIT_DIR, and people _demanding_ that it does not do so, I recommend against that.
In the following part, refnames must not contain spaces again:
Show 6 quoted lines
> + (for p in $prereqs ; do > + git-rev-list --pretty=one --max-count=1 $p > + done) > "$prerequisites" && > + git-show-ref $refs > "$references" && > + git-rev-list --objects $fullrevargs | cut -b-40 | > + git pack-objects --all-progress --stdout > "$pack" &&
pack-objects can take rev-list arguments itself these days, using "--revs" and being piped the rev-list arguments.
Show 5 quoted lines
> +verify) > + verify && echo "$bfile is ok";; > + > +list-heads) > + list_heads $args;;
I like this abstraction (defining a function, and calling that). Why don't you do it with all commands?
Rest looks fine.
Given the problems you had with "cat" on Cygwin, and the quoting stuff, I think it might make sense to make this a builtin right away, before the script stage (which is meant to speed things up, not slow them down).
Ciao, Dscho