Re: Question about .git/objects/info/alternates

On Wed, Mar 24, 2010 at 12:58 PM, Junio C Hamano [off-list ref] wrote:

Chris Packham [off-list ref] writes:

quoted

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.

quoted

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

quoted

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".

quoted

+    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.

quoted

+        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.

quoted

+        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).

quoted

+# 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
               ...

quoted

+        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.

quoted

+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.

quoted

+    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?

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 ...

quoted

+    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?

quoted

+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.

quoted

+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.

quoted

+    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.