Re: Question about .git/objects/info/alternates
- From
Chris Packham <judge.packham@gmail.com>
- Date
- Mar 24, 2010, 20:35 UTC
- Message-ID
- <a038bef51003241335l1623ade4i4a9b7269546739d4@mail.gmail.com>
- In-Reply-To
- <7vljdhh4po.fsf@alter.siamese.dyndns.org>
On Wed, Mar 24, 2010 at 12:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Chris Packham <judge.packham@gmail.com> writes: > >> git repack -a did the correct thing. >> >> It occurs to me that the UI around alternates is a bit lacking i.e. >> there isn't a git command to display the alternates in use or to add >> them to an existing repository (or at least I couldn't find one >> skimming the docs or googling). > > That's an understatement. Thanks for starting the effort.
Thanks for the comments (and lessons in portable shell scripting). I'll re-roll shortly. Some responses to specific questions below.
> I will likely to have quite a few style issues with the script > implementation, and am undecided if this should be a new command > or an option to some existing command, but it's time we have a > management facility for alternates.
Fair enough about style issues, I tend to code everything like its C I guess some habits are hard to break. I was wondering about the justification for having a separate command but I couldn't think of anywhere else it'd fit. Also calling the command alternates when we create them with git clone --reference seems a bit funny.
Show 16 quoted lines
>> diff --git a/Makefile b/Makefile >> index 3a6c6ea..1a7b084 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -334,6 +334,7 @@ TEST_PROGRAMS_NEED_X = >> unexport CDPATH >> >> SCRIPT_SH += git-am.sh >> +SCRIPT_SH += git-alternates.sh >> SCRIPT_SH += git-bisect.sh >> SCRIPT_SH += git-difftool--helper.sh >> SCRIPT_SH += git-filter-branch.sh > > You probably need one entry in the command-list.txt to classify where in > the main manual page git(1) for this command to appear. I would suggest > imitating "git config".
Yes will do when I write the documentation
Show 30 quoted lines
>> diff --git a/git-alternates.sh b/git-alternates.sh
>> new file mode 100755
>> index 0000000..74ec707
>> --- /dev/null
>> +++ b/git-alternates.sh
>> @@ -0,0 +1,159 @@
>> +#!/bin/sh
>> ...
>> +#
>> +# Runs through the alternates file calling the callback function $1
>> +# with the name of the alternate as the first argument to the callback
>> +# any additional arguments are passed to the callback function.
>> +#
>> +walk_alternates()
>> +{
>> + local alternates=$GIT_DIR/objects/info/alternates
>> + local callback=$1
>
> We want to use this on platforms with ksh and dash, so please refrain from
> bash-ism features. "local" does not mix well with "#!/bin/sh".
>
>> + shift
>> +
>> + if [ -e $alternates ]; then
>
> We tend to avoid '[' and write it like this:
>
> if test -f "$alternates"
> then
> ...I think there were a few of these I missed before submitting. Easy to fix.
> Also notice that an indent is one tab and one tabstop is 8 places, and > alternates need to be quoted in case $GIT_DIR has IFS whitespace in it.
I must have used the wrong thing as an example. I actually started with tabs and converted it to spaces after looking at git-mergetool.sh.
Show 16 quoted lines
>> + while read line >> + do >> + $callback $line $* > > As the path to an alternate object store can contain IFS whitespace, line > needs to be quoted. Also you call walk_alternates with things like "$dir" > that could also be a path with IFS whitespace, it needs to be quoted > properly. I suspect callback is only your own shell function, so it does > not have to be quoted, but it is Ok to quote it for consistency. I.e. > > "$callback" "$line" "$@" > > You are loooooose in quoting throughout your script, so I won't bother to > point all of them out in the rest of the message. You also are loose in > checking error returns (what you failed to write $newalternates file, for > example), which needs to be fixed before the final version.
I'll fix these.
Show 34 quoted lines
>> + done < $alternates
>
> This is correct per POSIX even if $alternates itself has IFS whitespace in
> it, but newer bash on some platforms have an obnoxious "safety feature"
> (see 3fa7c3d work around an obnoxious bash "safety feature" on OpenBSD,
> 2010-01-26) that can cause it to barf on this. It is unfortunate but we
> need to quote it like this:
>
> done <"$alternates"
>
> Also notice that there is no SP after < or > redirection (it is just a
> style thing).
>
>> +# Walk function to display one alternate object store and, if the user
>> +# has specified -r, recursively call show_alternates on the git
>> +# repository that the object store belongs to.
>> +#
>> +show_alternates_walk()
>> +{
>> + say "Object store $1"
>> + say " referenced via $GIT_DIR"
>> +
>> + local new_git_dir=${line%%/objects}
>> + if [ "$recursive" == "true" -a "$GIT_DIR" != "$new_git_dir" ]
>> + then
>
> if test "$recursive" = true && test "$GIT_DIR" != "$new_git_dir"
> then
> ...
>
>> + GIT_DIR=$new_git_dir show_alternates
>
> This is probably depending on a bug in bash and is not portable. See
> 76c9c0d (rebase -i: Export GIT_AUTHOR_* variables explicitly, 2010-01-22)OK I thought that was a standard thing. Exporting could play havoc with recursion, will have to look for a solution for that.
Show 14 quoted lines
>> +add_alternate()
>> +{
>> + if test ! -d $dir; then
>> + die "fatal: $dir is not a directory"
>> + fi
>
> This will not work with relative alternates. In two repositories A and B,
> where B borrows from A:
>
> A/.git/objects/
> B/.git/objects/info/alternates
>
> the alternates file in B can point at A's .git/objects, relative to its
> own .git/objects/.So would the best approach be not to validate the input or to validate it relative to $PWD and .git/objects/. I think the answer ties into my use of readlink.
Show 11 quoted lines
>> + walk_alternates check_current_alternate_walk $dir >> + >> + # At this point we know that $dir is a directory that exists >> + # and that its not already being used as an alternate. We could >> + # go further and verify that $dir has valid objects. >> + >> + # if we're still going we can safely add the alternate >> + touch $GIT_DIR/objects/info/alternates >> + echo "$(readlink -f $dir)" >> $GIT_DIR/objects/info/alternates > > What is this touch for?
I wasn't 100% sure >> would work if the file didn't exist. I just tried on a bash shell and it works. Does anyone know of any supported shells that behave differently w.r.t the >> operator?
Show 7 quoted lines
> Is readlink(1) portable enough across platforms? A more fundamental > question is if resolving symbolic link like this behind user is a sensible > thing to do, especially as you are ... > >> + say "$dir added as an alternate" > > ... lying to the user here, if $dir indeed is a symbolic link.
Using readlink was my hack around converting a user specified relative path to an absolute one. I actually would prefer it if it didn't interpret a symlink. I also had my doubts about portability its probably not going to exist on platforms that don't have real symlinks (windows). What I really wanted to do was something like "abspath $dir" but I was bitterly disappointed to find that was a gnu make-ism. Any suggestions?
Show 8 quoted lines
>> +rewrite_alternates()
>> +{
>> + if test "$1" != "$2"; then
>> + echo $2 >> $3
>> + fi
>> +}
>
> That's a misleading name for this helper function.I think this is made redundant by your grep suggestion below.
Show 20 quoted lines
>> +del_alternate()
>> +{
>> + if test ! $force = "true"; then
>> + say "Not forced, use"
>> + say " 'git repack -a' to fetch missing objects, then "
>> + say " '$dashless -f -d $dir' to remove the alternate"
>> + die
>> + fi
>> +
>> + local alternates=$GIT_DIR/objects/info/alternates
>> +
>> + new_alts_file=$(mktemp $alternates-XXXXXX)
>> + touch $new_alts_file
>> +
>> + walk_alternates rewrite_alternates $dir $new_alts_file
>
> I think this is a more expensive way to say:
>
> grep -v -F "$dir" <"$alternates" >"$new_alternates"
>Much easier. I'll do that.
Show 19 quoted lines
>> + mv $new_alts_file $alternates >> + >> + # save the git from repeatedly reading a 0 length file >> + if test $(stat -c "%s" $alternates) -eq 0; then >> + rm $alternates >> + fi > > Good point, but it would be better to do something like this: > > grep -v -F "$dir" <"$alternates" >"$new_alternates" > if test -s "$new_alternates" > then > mv "$new_alternates" "$alternates" > else > rm -f "$alternates" > fi > > without making it less portable by using "stat". >
Yes. Will change.